PSA: Connections can memory leak Instances!

TheAmazeman · April 18, 2018, 2:44am

What would be better?

stravant · April 18, 2018, 3:01am

The first one still leaks memory (Probably… if the Player object is Destroy’ed then neither would leak but I don’t think it is). You actually have to disconnect the reference to the connection that you save in the table, just saving the reference to it alone doesn’t accomplish anything:

for name, connection in pairs(playerStuffTable[plr]) do
    connection:disconnect()
end
playerStuffTable[plr] = nil

The second version will work since the Destroy disconnects the connection.

A better way to accomplish the same thing would be to put all of the stats inside of a “Stats” Folder and just Destroy the whole Folder so you don’t have to loop over each individual stat.

ChadTheCreator · April 18, 2018, 10:12pm

For a function like this which is just in a main game script and can never be disconnected,

,would I have to dispose of the “plr” and “itemName” variables somehow? Or is only the connection to the CraftedItem event what matters?

Seiiyo · April 21, 2018, 4:01am

As long as the two arguments aren’t being stored elsewhere they should be GC’d automatically (plr, itemName). So for your question, yes only the event matters because it has a reference and is connected to be listened too.

colbert2677 · June 8, 2018, 9:10pm

I require clarification on this. I still destroy my top-level object so in essence everything below is fine, but this gears more towards permanent scripts.

Assign table to variable “Tweens” in a do-end block
Insert Tweens into those tables via table.insert
Call Destroy on the Tween once it fires Tween.Completed

I called destroy on the tween, but I print #Tweens and it still reads 3. Are these hanging references that I need to dispose of or will the do-end block take care of that for me?

Corecii · June 9, 2018, 2:20am

This depends on if your Tweens table is inside or outside of the do end block and if Tweens gets referred to by any other part of the script.

This, for example, is fine, but Tween will only be garbage-collected up once the outer do end block is done:

do
	local Tweens = {}

	do
		local Tween = MakeTween()
		table.insert(Tweens, Tween)
		Tween.Completed:Connect(function()
			Tween:Destroy()
		end)
	end  -- `Tween` loses one of its references, the `Tween` variable, but it still has another in the `Tweens` table, so it is not garbage-collected
end  -- `Tweens` loses its only reference, so it is garbage-collected, so it no longer holds any reference to `Tween`, so `Tween` loses its only reference, so it is garbage-collected

If you want Tween garbage-collected before that (for example, when the tween is completed), then you need to manually remove the reference from inside the Tweens table:

do
	local Tweens = {}

	do
		local Tween = MakeTween()
		table.insert(Tweens, Tween)
		Tween.Completed:Connect(function()
			Tween:Destroy()
			for Index, Value in next, Tweens do
				if Value == Tween then
					table.remove(Tweens, Index)  -- remove reference inside `Tweens`
					break
				end
			end
		end)
	end
end

Or, alternatively, you could use a dictionary. This is easier and perfect for this use case:

do
	local Tweens = {}

	do
		local Tween = MakeTween()
		Tweens[Tween] = true
		Tween.Completed:Connect(function()
			Tween:Destroy()
			Tweens[Tween] = nil
		end)
	end
end

The thing to take away from this is that the tween will not be garbage-collected until all references – including the reference in the Tweens table – are gone. You either need to manually remove that reference or wait until the Tweens table gets garbage-collected itself.

colbert2677 · June 9, 2018, 6:53pm

My current code looks like the first block of code you posted. Thanks for the answer!

SirMing · June 20, 2018, 9:29pm

If I reuse the same variable, would that remove the old reference?

For example, would this cause memory leak?

local tween = nil

v.MouseButton1Down:Connect(function()
    if tween then
        tween:Cancel()
    end
    tween = tweenService:Create(...)
    tween:Play()
end)

XAXA · June 21, 2018, 8:25am

Yes.

-- Create a table that has weak values
-- (Values in the table can be garbage collected.)
local refCounter = {}
setmetatable(refCounter, {__mode = "v"})

-- Thing to be garbage collected. In this case, a Part.
local testPart = Instance.new("Part")

-- Insert it into refCounter, the size of refCounter is now 1.
table.insert(refCounter, testPart )
print(#refCounter) -- prints 1

-- Reuse the testPart variable. testPart is now a WedgePart
testPart = Instance.new("WedgePart")

-- Wait a short while for the next garbage collection cycle
wait(3)

-- The size of refCounter is now 0, which means that 
-- the old testPart was garbage collected.
print(#refCounter) -- prints 0

WrathOfTheElvhen · August 14, 2018, 8:08pm

A couple questions:

Will “:ClearAllChildren” work similar to Destroy()ing every child, or should I manually Destroy() each descendant?
Is it bad to have a connection to an event that will always exist? For example, a ClickDetector. I won’t have to worry about stuff like that, correct?
Do I have to worry about connections to instances that are part of the player object? For example, leaderstats values?
What about the Player object itself, should I disconnect all Chatted events and the like?
Should I disconnect any events in a player’s character? For example, Humanoid.ChildAdded or custom Humanoid.Died events, and so on? (Or alternatively, will :LoadCharacter() call Destroy() on a character model before loading it?)

sleitnick · August 14, 2018, 8:40pm

ClearAllChildren should Destroy all parts, so it should be fine
Not bad to have connections that always exist. This is common for many things in a game.
No, they are destroyed along with the Player object when the player leaves
No, Player object is destroyed, thus disconnecting all events
Again no; any object that gets destroyed will automatically disconnect events

If Roblox is handling the removal of items, assume that it is calling Destroy. If it’s not, that’s a bug and should be fixed.

WrathOfTheElvhen · August 14, 2018, 9:19pm

Alright, just what I wanted to hear. I did change it so that it destroys the humanoid and character right before calling LoadCharacter, since it seemed like UntrackedMemory was going up if I didn’t. I didn’t test it too much though, so it could just be me interpreting the results wrong, or it being some other problem. I’ll try doing more detailed tests later.

Anyways, old servers get UntrackedMemory up to around 1 GB, so a memory leak is definitely likely. Are there any other things I should look for, aside from reviewing all of my connections?

Kampfkarren · August 15, 2018, 1:24am

Question: I have a lot of code that looks like this

local connection; connection = BlaBlaBla.Event:connect(function()
    -- bla bla bla
    if something then
        connection:disconnect()
    end
end)

This leaks the connection, right? Is the easiest way to solve this just to connection = nil after disconnecting?

Corecii · August 15, 2018, 3:41am

connection will stick around for as long as some code that refers to it can be ran.

Assuming your code looks something like this:

do
	local connection; connection = BlaBlaBla.Event:connect(function()
	    -- bla bla bla
	    if something then
	        connection:disconnect()
	    end
	end)
end

then the only code that references connection is the function. The function there can only be ran as long as the signal is connected. Once it’s disconnected, the function has no references to it any more and is collected. Because the function no longer exists, there are no more references to connection, and it gets collected. Small cause-and-effect chart:

signal is fired
signal is disconnected
now-disconnected function is collected since it now has no references
connection is collected since it now has no references

In short:

It does not, assuming your only reference to connection is in the function and the only reference to the function is the event.

Kampfkarren · August 15, 2018, 3:44am

Always amazed at how smart Lua’s garbage collector is.

Tomarty · February 6, 2019, 5:15pm

If referencing instances from connected functions can cause serious memory problems, why aren’t the instances simply passed as arguments to their connected functions? Like this:

part.Touched:Connect(function(otherPart, part)
    print(part)
end)

This would also provide huge performance gains when connecting the same function to many parts:

local function onTouched(otherPart, part)
   print(part)
end
for i = 1, #parts do -- Connect to many parts
    parts[i].Touched:Connect(onTouched)
end

Connected functions could use less than half as much memory due to fewer upvalues

stravant · October 4, 2019, 6:54pm

Late reply, but, that doesn’t really solve the problem. It could solve the problem for the case of exactly solitary Roblox instances, but as soon as you had any sort of non-trivial data structures in your Lua code you’d end up back with the same issues.

You also can’t bodge around it by letting users “connect with” an additional userdata parameter that will get passed to the handler when it is invoked, like:

event:connect(function(args, userdata) ... end, userdata).

If you did you have exactly the same circular reference problem because the C++ side is holding a reference to that userdata but still doesn’t know anything about the contents of the userdata or what Roblox instances it may be referencing.

Tybearic · April 19, 2020, 9:01pm

Say I have an event for when my player receives more xp on the server, and I want to check to see if they have enough xp to level up. The IntValue that is stored inside the player fires an event when the value is updated. So when the player leaves, all the events connected to that IntValue will disconnect right?

nulllifeleft · April 19, 2020, 11:11pm

Yes, because when a player leaves or gets kicked, Roblox deletes the player instance. So unless you were just calling Remove() or setting player instances to nil, you should be good.

codyorr4 · April 20, 2020, 10:25am

what a great read and its actually kind of concerning that this hasn’t been mentioned much anywhere else…

I have always disconnected my events when they are no longer needed, even when I was just starting out on roblox scripting, because its a DUALITY (if you open a door then you can also close it and if you can connect to something then you can also disconnect from it) the fact that many people overlook it… is fascinating but definitely understandable.