Is there any memory leaks in this code?

Help and Feedback Code Review
#1

(This is my first post here, please correct me if I do anything wrong)

I recently started rewriting my footsteps script, and I came up with this simple code:

    local function keyFrameReached(frameName)
		if frameName == "WalkSound" then
			-- do footstep stuff here
		end
	end
	
	humanoid.Animator.AnimationPlayed:Connect(function(anim)		
		anim.KeyframeReached:Connect(keyFrameReached)
	end)

This code is inside a CharacterAdded event.

Can this piece of code create any memory leaks? Or is it all fine?

EDIT: As @Pxxm said, this code does indeed leak memory, so I came up with this:

    local function keyFrameReached(frameName)
		if frameName == "WalkSound" then
			-- do footstep stuff here
		end
	end
	
	humanoid.Animator.AnimationPlayed:Connect(function(anim)	
        local connection	

		connection = anim.KeyframeReached:Connect(keyFrameReached)

        spawn(function()
            anim.Stopped:Wait()
            connection:Disconnect()
        end)
	end)

Would that fix the problem?

#2

The anim.KeyframeReached event in your AnimationPlayed event will most likely cause a memory leak. Make sure to disconnect that when you don’t need the event anymore.

#3

Thanks!
Sadly there isnt an AnimationStopped event, so I will have to mess around and try to figure something out

#4

Came up with something, would this fix the memory leak?

    local function keyFrameReached(frameName)
		if frameName == "WalkSound" then
			-- do footstep stuff here
		end
	end
	
	humanoid.Animator.AnimationPlayed:Connect(function(anim)	
        local connection	

		connection = anim.KeyframeReached:Connect(keyFrameReached)

        spawn(function()
            anim.Stopped:Wait()
            connection:Disconnect()
        end)
	end)
#5

Yeah that would work. Consider this a micro-optimization, but coroutines seem like a better option here.

1 Like
#6

I don’t know why you would need the spawn() that just seems uncessary unless you’re missing out some code?

Since :Connect() doesn’t yield the code when its called it just creates a seperate thread

Also you don’t need to do Global variables for :Disconnect()

	humanoid.Animator.AnimationPlayed:Connect(function(anim)

		local connection = anim.KeyframeReached:Connect(keyFrameReached)

        anim.Stopped:Wait()
        connection:Disconnect()
	end)
1 Like
#7

I just thought that anim.Stopped:Wait() would prevent the KeyframeReached event from running more than once, thanks

#8

Oh then just do

humanoid.Animator.AnimationPlayed:Connect(function(anim)

	local frameName = anim.KeyframeReached:Wait()

    keyFrameReached(frameName)
end)

Cause this will fire once

#9

Thats not what I meant.
I added the spawn because I thought that anim.Ended:Wait() would actually wait until the animation stopped, preventing the keyframeReached event from running more than 1 time when the animation played.

My intended use was that the event fired more than once.
But if anim.Ended:Wait() does NOT pause the code, then I dont need the spawn

#10

When an event is fired, it creates a new thread. If an event is connected on the (now yielded) thread, it will still run the callback code.

TL;DR: You do not need spawn in this code. The event will still fire several times.

1 Like
#11

@Pxxm The event it self won’t cause any memory leaks, as you aren’t using any up values which are of reference types. Even if you don’t disconnect the event, it still isn’t a memory leak (because all it does is run the call back assigned to the event) since most people on Roblox have the wrong concept of how events work.

Event:Connect(callBack) just inserts callback to the call back list of the event, and when the event is invoked, Roblox will call all the call backs in the call back list of the event, passing the valid arguments. Event:Connect(callBack) also returns a connection object (calling Disconnect on it) will not DISCONNECT the entire event it self, but rather remove callback from the call back list of the event so that it isn’t called. Internally it happens differently but I’m going to assume something similar to this, as its efficient and makes complete sense.

Pseudo code:

-- NOTE: THIS IS ALL DONE ENTIRELY IN C, JUST A LUAU PSEUDO CODE
-- OF HOW IT MAY LOOK:

function Signal:Connect(callBack)
      -- error checking
      table.insert(signalCallBackList, callBack)
      local index = #signalCallBackList

     return {Disconnect = function()
             table.remove(signalCallBackList, index) 
      end
end

This also proves you can do the following:

local event = Instance.new("BindableEvent")

local e = even.Event:Connect(function()

end)

local d = even.Event:Connect(function()

end)

e:Disconnect()
-- event d still runs but not e
#12

I don’t know if I completely understood your post but I’m not talking about the outer event, I’m talking about the event within the outer event. From what you’ve told me the callback gets inserted into a list where the event will call after being fired. My question is, if you stack up on unneeded call backs won’t that cause a memory leak?

#13

Memory leak is when a program is using more memory (even a slight amount that won’t impact anything) than needed. It depends on the situation of what the call backs do, if empty callbacks, then no. If the call backs are relatively expensive, then yes it can cause memory leak ONLY when the event is fired, but that leak would end after those call backs finish.

For example, a code like this would cause a memory leak:

local event = Instance.new("BindableEvent")

local t = {}

 event.Event:Connect(function()
     print(t)
end)

Since the call back of the event uses the up value t, and gcable objects cannot get collected if they have more than 0 references. Because of the function connected to the event, there is +1 reference as the function uses the up value t, so technically 2 references (The value of t also has 1 reference, which is t it self). When the scope ends (of the main stack), the value of t loses only 1 reference, which is t it self as it is removed from the stack when it end, but the function still uses the up value t, so that’s a +1 reference.

t would only be collected if it has 0 references, but it will always have 1 EXTRA reference in this case. When the event is disconnected, the call back is available for collection since the event won’t use the call back anymore and dereferences it. (The callback will be collected assuming you don’t have a reference to it, most of time you dont).

When closures using up values are garbage collected, they no longer reference those up values obviously.

3 Likes