Client-sided RemoteEvent memory leak

Bug Reports Engine Bugs
#1

Make sure to see the full place file, but when you create a remote on the server and destroy it a bit after, the client doesn’t gc the references

As demonstrated in the last example of PSA: Connections can memory leak Instances!, this should not be happening

Here is the client side snippet:

local t={}
do
	local checker=setmetatable({t},{__mode='v'})
	coroutine.wrap(function()
		while checker[1] do
			print'exists'
			wait()
		end
		print'gced'
	end)()
end
local ev=workspace.RemoteEvent

ev.OnClientEvent:Connect(function()
	_=t
end)
t.x=ev

ev.AncestryChanged:Wait()
print'c got kill'

Server side:

local ev=Instance.new("RemoteEvent",workspace)
wait(3)
ev:Destroy()
print("s destroyed")

Full place:
roblox broke.rbxl (13.1 KB)

Interestingly, if you manually disconnect the event on the client side when ancestry changes (bc no destroyed event :frowning: ) it successfully gc-es

local t={}
do
	local checker=setmetatable({t},{__mode='v'})
	coroutine.wrap(function()
		while checker[1] do
			print'exists'
			wait()
		end
		print'gced'
	end)()
end
local ev=workspace.RemoteEvent

local disc=ev.OnClientEvent:Connect(function()
	_=t
end)
t.x=ev

ev.AncestryChanged:Wait()
print'c got kill'
disc:Disconnect()
MEMORY LEAK: Player connections are not disconnected on player leave
#2

Please do not abuse the critical tag. See here for guidelines on when to use it:

#3

The reference to the table t inside the connection prevents the RemoteEvent to get garbage collected.

#4

I am confused about some items from your script:

  1. local checker=setmetatable({t},{__mode=‘v’}), this sets the metatable on an anonymous table that contains t, not on t
  2. due to the way we bridge objects from the engine to lua, you cannot use weak table keys to establish if an instance is alive or dead, it is possible to get false positives (the key may leave the table before the instance has been deleted. The only reliable way to know if there is an instance leak is to check the instance count (game:GetService(“Stats”).InstanceCount)
  3. There is a dependency cycle. The remote event will hold a reference to the callback for as long as the script thread that created the callback is still alive. The callback holds a strong reference to “t” for use in the callback. “t” holds a strong reference to the remote event (creating a cycle). This is why disconnecting the callback allows the object to be collected.
#5
  1. it’s to check if the table t has been gced
  2. i’m not really checking if an instance is gced though, the ‘checker’ weak table has table t inside of it
  3. the wiki for destroy explicitly states Instance:Destroy “disconnects all connections”

& as an elaboration for #3, the example I was talking about in the OP’s link is

do -- Also also all good, as Destroy() implicitly disconnects all connections
    local p = Instance.new('Part')
    p.Touched:connect(function() print(p) end)
    p:Destroy()
end

which if you test for gc, it successfully disconnects&gces (as the wiki explains should happen)

another clarification:
I’m talking about the table not gcing, not the RemoteEvent instance
but as a consequence of the table not gcing and still holding a reference to the instance, [at least part of] the RemoteEvent instance won’t gc either

#6

You cannot force a lua table to become gc’ed by putting in to some other table with a weak reference. All callbacks contain strong references to all closed variables, so

local disc=ev.OnClientEvent:Connect(function()
	_=t
end)

holds a strong reference to t for as long as the callback exists. One other part that may be confusing here: due to legacy reasons, if an object is deleted on the server, we do not call :Destroy on it, we only call .Parent = nil, meaning that de-parenting on the server will not clean up connections for you.

3 Likes
#7

dang well that is the problem

why don’t you call destroy?

#8

In the last year we transitioned to a purely server-authoritative model; before that point, games were allowed to let clients perform significant game changes client side. In a heavily distributed environment like that, being more gentle with Parent = nil allowed for more flexibility.

Then, because we have maintained that behavior for so long up to this point, there is a lot of potential to break games if we change this behavior today. Some games specifically leverage network de-sync on client to create local effects, and those kinds of effects can break if we forcibly Destroy everything that the server releases. We try very hard to avoid breaking changes like that in general as a product directional thing.

1 Like