Properly destroy player on leave

[acreol]

The destroy wiki explains that destroy “Sets the Instance.Parent property to nil, locks the Instance.Parent property, disconnects all connections and calls Destroy on all children”

As a Roblox developer, it is currently too hard to manually unlink the player signal connections (such as CharacterAdded) to prevent memory leaks and other bugs that may rely on the player connections being disconnected (because you expect certain items to be garbage collected, but they persist indefinitely)

If Roblox is able to address my issue, it would make developing a lot less painful because I would not have to track all player connections to disconnect them when the player leaves

Proof that the player is improperly destroyed (connections are not disconnected)

The following code shows that the table x persists indefinitely because the CharacterAdded connection is not disconnected

local t=setmetatable({},{__mode='k',})

game:GetService'Players'.PlayerAdded:Connect(function(plr)
	local x={}
	t[x]=true
	plr.CharacterAdded:connect(function(char)
		print(x)
	end)
end)

while wait()do
	local n=0
	for k,v in next,t do n=n+1 end
	print(n)
end

If you comment out the print(x) inside of CharacterAdded, lua properly garbage collects the table x

Additionally, if you manually disconnect the player connections (what we unfortunately have to do now), then lua also properly garbage collects the table x:

local t=setmetatable({},{__mode='k',})
local cons={}

game:GetService'Players'.PlayerAdded:Connect(function(plr)
	local x={}
	t[x]=true
	cons[plr]=plr.CharacterAdded:connect(function(char)
		print(x)
	end)
end)

game:GetService'Players'.PlayerRemoving:Connect(function(plr)
	cons[plr]:Disconnect()
	cons[plr]=nil
end)
while wait()do
	local n=0
	for k,v in next,t do n=n+1 end
	print(n)
end

I want to clarify that this doesn’t just happen for the CharacterAdded connection, I believe it happens for ALL RBXScriptSignals in the player object (I tested CharacterAdded, Chatted, and DescendantRemoving)

Hopefully this thread clarifies the issue better

[Dekkonot]

To add onto this, the expected behavior for leaving players is that their Player instance is destroyed and not simply parented to nil. This is a problem that’s come up in my work and it can cause unexpected memory leaks to new developers. If the Player instance was properly destroyed, it would make the behavior of Players more obvious and expected, and decrease the effort required clean up after a player leaves the game.

[TheRings0fSaturn (SaturianNightmare)]

Personally, I’m not sure how this is not classified as a bug, or how it was related to RemoteEvent memory leaks - they’re two entirely different problems. Anything that leaks memory should be classified as a bug, regardless if it’s undocumented behavior or not.

Anyways, this is something that makes developing when it comes to players quite confusing to newer Developers, since everything else is destroyed, besides players. I’m not sure why Players aren’t properly destroyed when they leave the game, as DataStores still have to reload everything when a player rejoins the game, so there’s really no reason to keep the current behavior of players leaving a game and should be changed to where the Player instance is destroyed 10 seconds after the Player’s client closes, or sooner, unless there is a datastore that needs to be updated.

[acreol]

The player seems to GC for me, but only after >13-15 seconds pass since the player leaves the game
It’s also unaffected by whether I spam create tables to make the GCer more aggressive, but if I manually disconnect the connection it GCs for me in <.5 seconds
So maybe Roblox is doing something with the player in the background

edit:::

local t=setmetatable({},{__mode='k',})

game:GetService'Players'.PlayerAdded:Connect(function(plr)
	local x={}
	t[x]=true
	plr.CharacterAdded:connect(function(char)
		print(plr)
		print(x)
	end)
end)

while wait()do
	local n=0
	for k,v in next,t do n=n+1 end
	print(n)
end

The repro in the op is wrong because it will gc over time if no references are stored on the player; there needs to be a circular reference to confuse the lua gc, so the repro in this post properly shows the bug
At least now we know that Roblox doesn’t hold any lua references to the player object (after some time passes since the player leaves)

[acreol]

it’d be really nice to fix this because i have a 250 MB leak from relying on this being fixed in a server that’s been running for 24 hours

[magnalite]

Can confirm that players are not destroyed properly and will result in memory leaks. Players not being destroyed is unintuitive and if it is intended then this needs to be documented!

Currently players appear to be parented to nil instead of destroyed, and in my testing never actually get destroyed. This means all associated events are not disconnected and all descendant objects are not destroyed. As events are not disconnected this means any reference to a player object inside of a player event will cause it to never get gc’d.

As far as I can tell this simple script is technically a memory leak.

Players.PlayerAdded:Connect(function(player) 
    player.CharacterAdded:Connect(function()
        print(player)
    end)
end)

Seeing as this kind of script is likely very common, this is scary.

This has nothing to do with lua’s garbage collector as it will collect the reference to the player object, but the player object will still exist in a non destroyed state. You can test this with the following script.

local Players = game:GetService("Players")
local t = setmetatable({}, {__mode='k'})
local part, timer

Players.PlayerAdded:Connect(function(player)
	t[player]=true
	part = Instance.new("Part", player)
end)

Players.PlayerRemoving:Connect(function(player)
	timer = tick() 
end)

spawn(function()
	repeat wait() until timer
	print("Player left, timing...")
	repeat wait() until not part.Parent
	print("It took", tick() - timer, "seconds for the player to be destroyed")
end)

spawn(function()
	
	repeat wait() until part
	
	print("Player joined")
	
	while wait() do
		local n = 0
		for i, v in pairs(t) do
			n = n + 1
		end
		
		if n == 0 then
			print("Player lua gc'd after", tick() - timer, "seconds")
			break
		end
	end
end)

It will never find that the player has been destroyed. You can force the player object to be destroyed by adding this to the end of the script, which shows it will detect it being destroyed properly.

delay(20, function()
	print("Forcing destroy")
	part.Parent:Destroy()
end)

I have no doubt this behavior is causing many, albeit probably small, memory leaks. This needs to be changed to disconnect events and destroy descendants as expected. Even as an experienced developer this is incredibly odd behavior and having no obvious documentation on this is dangerous.

[cowsoncows]

Hate to bump a year-old old thread, but I just came across an article on event handling in the wiki that indicates that the player is properly destroyed, but as far as I can tell this still isn’t the case.

Trying all the tests throughout this thread it seems nothing has changed; the player still isn’t properly destroyed. So now not only is there no proper documentation on this, but there is instead incorrect documentation?

Edit: With the new docs out, it looks like that false statement about the Player object’s destruction has been removed.

[RuizuKun_Dev (Kitsune)]

I get The Parent property of Instance is locked error for calling Player:Destroy on Players.ChildRemoved

for anyone that runs into this issue you can use this as a work around

Debris:AddItem(player, 0)
Debris:AddItem(player.Character, 0)

I have do this simply because I hook functions to the player Character or Humanoid events. For example when the player leave I am uncertain that Roblox would disconnect the Humanoid.Died connection which will cause a memory leak.

[buildthomas]

@RuizuKun_Dev

For example when the player leave I am uncertain that Roblox would disconnect the Humanoid.Died connection which will cause a memory leak.

And you are sure that items added to Debris actually do disconnect the connections? How have you tested this?

[Luaction]

It does work.

game.Players.PlayerAdded:Connect(function(ply)
	local event = ply.CharacterAdded:Connect(function() end)
	game.Debris:AddItem(ply, 10)
	while true do
		wait(1)
		print(event.Connected)
	end
end)

[buildthomas]

Thanks for the sample code.

@RuizuKun_Dev This might be a little cleaner logic that doesn’t require an arbitrary yield: if you use task.defer you can avoid the property lock.

game.Players.PlayerRemoving:Connect(function(plr)
	task.defer(function() plr:Destroy() end)
end)

[RuizuKun_Dev (Kitsune)]

@Luaction thanks for the test!

thanks for the code!

why does task.defer prevent the error from happening?

@buildthomas

And you are sure that items added to Debris actually do disconnect the connections? How have you tested this?

;_; well, I assumed that it would do that because it’s expected, I mean… it would be pretty stupid if it didn’t and also the documentation

[buildthomas]

@RuizuKun_Dev

why does task.defer prevent the error from happening?

The property lock on Parent only holds while in the scope of the event handler.

Task.defer lets you postpone the work to the soonest possible moment in the same frame, without throttling. (unlike wait/spawn/delay/etc, which only happen a frame later at best, and possibly many more frames because of throttling) Since that is outside of the event handling thread the lock is no longer a problem, and you ensure the cleanup happens as fast as possible.

More detail here: Task Library - Now Available!

[buildthomas]

@RuizuKun_Dev

;_; well, I assumed that it would do that because it’s expected, I mean… it would be pretty stupid if it didn’t and also the documentation

I’ve been on the platform for a while and this seems relatively new (probably changed in the last 3-5 years or so), so my muscle memory may be off. It used to not actually cleanup event connections. If it’s in the documentation you can expect that to be the intended behavior yeah. But it’s still better to not use Debris if you don’t actually need an arbitrary delay in the cleanup of the instance, which you don’t really need here.

[Knineteen19 (Knine)]

Sorry to bump this topic after 2 years but from what I can tell (which isn’t much since I just found out about this issue after 5 years of scripting and am still learning about garbage collecting and weak tables) this issue still persists. Is there a reason this hasn’t been fixed after 4 years? It seems like something that would be a simple fix on Roblox’s end, but maybe I’m missing something. I would just make a post about this in #bug-reports:studio-bugs but I don’t have permission to.

This seems like something that would be a serious issue, but is it even being looked at at all? I couldn’t find any posts in #bug-reports:studio-bugs or anything talking about it either. I only ever heard about this issue just now because the documentation literally says otherwise.

As cowsoncows mentioned, the documentation states that the connections to CharacterAdded are disconnected. Cowsoncows also stated that it was removed from the documentation later on, but on the same page on events I found this:

Which basically states the same thing. Unless this isn’t an issue anymore and I’m just not testing it properly (since frankly I’m not entirely sure how the test scripts posted on this topic work).

[colbert2677 (ImagineerColbert)]

FWIW: it seems Roblox is aware of this and wants to change it but supposedly destroying the player might break some existing code and they’re looking for ways to make this change work across the board. I agree though, I’ve been developing for over 10 years on this platform and this issue evaded me entirely. I’m probably dealing with a lot of memory leaks.

Here’s the staff response to a question about destroying the player:

Disconnect Player on Removal from DataModel

Yeah, this is also something we’d like to change. Unfortunately, solutions we come up with could potentially be a breaking change (fixing this could change the behavior of scripts), so we’re still in the process of thinking through what the best solution would be.

[Knineteen19 (Knine)]

Ahh okay that makes sense. I wonder if it has to do with the PlayerRemoving event being able to return the player object itself possibly? Maybe properly destroying the player would make it hard to do things like saving data when a player leaves. Either way thanks for the updated information. I’ll just be wary of this issue as I go on.

[Rinpix]

It honestly took me by surprise that this isn’t already a thing. I was wondering what the instances leak was in my game and then I found out thru the dev console Luauheap that it’s player instances.