PSA: Connections can memory leak Instances!

[Dev0mar (Omar)]

Interesting, thanks!

I recommend putting this in #tutorials

[edenojack]

I’ve been using this to disconnect one-time use connections;

local con
con = something.something:connect(function() 
print("works")
con:disconnect()
end)

That way once the connection has been used for its purpose, it should no longer exist. I can’t see any immediate issues with it, anything I’ve overlooked?

[davness]

Wouldn’t the :Wait() method be better for single usage events?

local passedArgs = something.something:Wait()
print("works")

[buildthomas]

That’s possible, but if “something” can be destroyed, that :Wait() will hang forever. And if you want to do code after it, you’ll have to wrap it in its own thread anyway, hence an event connection would be cleaner.

[ChadTheCreator]

Thanks for the reply! So just to reiterate, using :disconnect() on all connections you make (once you’re done with them) cleans up this issue?

[jewelycat]

Doesn’t ROBLOX’s default Health script cause a nasty memory leak every time a player dies?

-- Gradually regenerates the Humanoid's Health over time.

local REGEN_RATE = 1/100 -- Regenerate this fraction of MaxHealth per second.
local REGEN_STEP = 1 -- Wait this long between each regeneration step.

--------------------------------------------------------------------------------

local Character = script.Parent
local Humanoid = Character:WaitForChild'Humanoid'

--------------------------------------------------------------------------------

while true do
	while Humanoid.Health < Humanoid.MaxHealth do
		local dt = wait(REGEN_STEP)
		local dh = dt*REGEN_RATE*Humanoid.MaxHealth
		Humanoid.Health = math.min(Humanoid.Health + dh, Humanoid.MaxHealth)
	end
	Humanoid.HealthChanged:Wait()
end

Since characters never get :destroy() called on them, they just get parented to nil. This is also the case for Player instances.

[Quenty]

This has been my suspicion for quite a while. It’s nice to know that the closure is aware of usage underneath it. I’d highly recommend disconnecting all events you don’t intend to be permanent.

General notes:

• :Destroy() is recursive so you only have to call it on the parent hierarchy
• If your client scripts don’t persistent when the player dies, you may be fine. This isn’t a great assumption, but Roblox definitely disconnects scripts on death. Module scripts are an exemption to this, and don’t disconnect (for good reason)

@stravant

@Quenty has a very nice pattern for this with his “Maid” class that you can find in multiple of his projects if you need a way to fix it.

I stole this pattern from Anaminus, although my implementation does some nicer things when nested Maids and on-cleanup calls.

@Crazyman32

This is all a good example of how we can be learn to design our systems better. Lua falters from making all of this not very obvious. In Swift, you can explicitly mark variables as ‘weak’ to avoid this issue, which I’ve really enjoyed using.

Lua has weak tables, but as we’ve discussed before, these don’t work out too nicely for GC.

@jewelycat

Doesn’t ROBLOX’s default Health script cause a nasty memory leak every time a player dies?

No, these scripts in the character and definitely removed / destroyed in some manner.

while true do
	wait(0.5)
	print("Still running script", script:GetFullName())
end

Stops execution after death. This implies that signals are disconnected as well.

For players themselves, I’m not sure. I always disconnect events on players. I think scripts inside of them are also destroyed.

[jewelycat]

It seems scripts specifically within the character are disabled, but events connected to the character from elsewhere are not. So if you have a HealthChanged function (for example) connected to players OnCharacterAdded from the server, this connection does not disappear just because the character dies. The garbagecollection for this event then follows the rules posted in the OP. I’d imagine this is the same case for players.

[Noble_Draconian]

This post right here will fix a lot of lag issues in my game. Great post!

(You might want to consider moving this to the public tutorial section. This is seriously useful information.)

[sncplay42]

Could the core issue be worked around with a backport of Lua 5.2’s ephemeron weak tables?

[stravant]

Yes, I believe it could… you could use an ephemeron table to hold references to Lua functions in connection lists. However, it suspect that it would also make the Lua bridge much more complicated:

• The Lua bridge would have to be “aware” of something being passed over it is being used for, since the closure passed to connect would have to be associated with object whose connection list it is being stored in when being put in the ephemeron. Currently it’s just a flat system opaquely passing values across.

• You would probably have to hold a strong reference to the Lua-side “Ref” object associated with the instance while that instance is in the object hierarchy and it has any connections to Lua functions (Note: The connection interface is uniform between C++ and Lua, the engine can connect events to C++ functions too) so that the closure doesn’t get GCed while the object still exists on the C++ side (currently unless you get a reference to a given instance, that instance doesn’t exist on the Lua side at all—even though it may have connections to Lua functions which don’t reference it)

• There is probably additional complexity involved in getting RBXScriptSignal::Wait to function correctly in all circumstances with this ephemeron implementation.

TL;DR: It would definitely be a non-trivial project even if the backporting part were free.

[shayner32 (shayne)]

connect() is deprecated, you’re spreading false info!111!1!!

Jk, good guide and definitely something I never thought about.

[stravant]

@shayner32

connect() is deprecated

I’m an old fogey… so uppercase Workspace and lowercase connect it is.

[buildthomas]

I’ll probably keep using lowercase connect myself until they start throwing warnings for it in intellisense. It’d be too weird to switch otherwise.

[Kampfkarren (boyned)]

I use :connect because that way my eyes will focus on the PascalCased event name. Of course, we’re going off topic here, but…

[metatablecatmaid (metatablecat)]

I prefer :connect() in my code

[TaaRt]

The only issue with using :connect() versus :Connect() is being forced later to change it all. For now they are exactly the same, but if some kind of major issue is discovered/new method is applied, it will be applied to Connect and not to connect. Using the lowercase version is like sticking to an old OS; it still will work, but over time it can get more and more out of date, potentially exposing flaws or lacking features that are actually required. In the end it is a matter of change now with little effort, or change later with a ton of effort.

[Kampfkarren (boyned)]

Both Connect and connect point to the exact same function in memory.

[TaaRt]

@TaaRt

For now they are exactly the same, but if some kind of major issue is discovered/new method is applied, it will be applied to Connect and not to connect.

That is exactly what I said.

[Kampfkarren (boyned)]

No it won’t. If it applies to Connect, it’ll automatically apply to connect. They point to the same function in memory. Your statement would be true if they both pointed to different, identical functions.