:LoadCharacter() leaks memory if you don't manually :Destroy() the old character

If you call LoadCharacter without manually destroying the old character, you get memory leaks on both client and server.

CharacterAutoloads == false

--This leaks
while(true) do
	
	for key,player in game.Players:GetPlayers() do
		--Spawn!
		player:LoadCharacter()
	end
        wait(0.2)
end
--This doesnt
while(true) do
	
	for key,player in game.Players:GetPlayers() do
		
		if (player.Character) then 
			player.Character:Destroy()
			player.Character = nil
		end
		player:LoadCharacter()
	end
	

	wait(0.2)

end

Expected behavior

I would expect LoadCharacter() to correctly clean up the old character. It does not.

Sample

Empty baseplate + this script showing the leak:
MemoryLeakBaseplate.rbxl (52.7 KB)

64 Likes

Oh no :frowning:

I did another test with CharacterAutoLoads = true

while(true) do
	
	for key,player in game.Players:GetPlayers() do
		
		if (player.Character) then 
			local humanoid = player.Character:FindFirstChild("Humanoid")
			if (humanoid) then
				humanoid.Health = 0
 			end
		end
	end
	
	wait(0.2)
end

This leaks.

This is a big deal, because this is what happens when you just let characters get handled by roblox normally.

I don’t even know how to stop this case from leaking, none of my experiments worked.

13 Likes

Wow. That’s a major issue. Have you tried contacting roblox directly about this? There’s no obvious way to even fix the second one.

4 Likes

I posted here - hopefully this’ll be enough. They know how to get ahold of me :smiley:

4 Likes

does the weak table method work?

local weakCharacters = setmetatable({}, { __mode = "v" }) -- Create a weak table

for _, player in pairs(game.Players:GetPlayers()) do
    player.CharacterAdded:Connect(function()
        -- Keep a weak reference to the old character since it's weak, shouldnt prevent the garbage collector from cleaning it up?
        table.insert(weakCharacters, character)
    end)
end
game.Players.PlayerAdded:Connect(function(player)
    player.CharacterAdded:Connect(function()
        table.insert(weakCharacters, character)
    end)
end)

while true do
    for _, player in pairs(game.Players:GetPlayers()) do
        player:LoadCharacter()
    end
    wait(0.2)
end

It probably doesnt work, because I’m assuming that player:LoadCharacter() doesn’t hold onto other strong references of the old character internally. Worth a shot.

4 Likes

It might.

It’s worth mentioning again that the second example doesn’t use LoadCharacter, it just allows spawning via a regular spawnpoint.

3 Likes

I have seen this bug reported a few months ago, can’t find the post.
I thought it would be fixed by now, but eh?

4 Likes

Yeah I think I’ve seen this get “Fixed” before, too.

4 Likes
local Players = game:GetService("Players")

local function cleanupCharacter(player)
    if player.Character then
        -- Cleanup logic; we know it works
        player.Character:Destroy()
        player.Character = nil
    end
end

local function overrideLoadCharacter(player)
    -- Ensure we're not doing this more than once per player
    if player:IsA('Player') and not player:FindFirstChild("LoadCharacterOverridden") then
        local originalLoadCharacter = player.LoadCharacter

        player.LoadCharacter = function(...)
            cleanupCharacter(player)
            return originalLoadCharacter(player, ...)
        end

        -- Add a tag
        local tag = Instance.new("BoolValue")
        tag.Name = "LoadCharacterOverridden"
        tag.Parent = player
    end
end

-- Immediately override for existing players
for _, player in pairs(Players:GetPlayers()) do
    overrideLoadCharacter(player)
end

-- Setup to override for any players that join in the future
Players.PlayerAdded:Connect(overrideLoadCharacter)

Make sure no other script in your game is trying to call LoadCharacter before this script, no idea what determines the order in which scripts run in serverscriptservice, assuming it’s sorted by name prefix the script with $

5 Likes

Unless I’m misunderstanding, no the weak table won’t work

4 Likes

This arleady is a solid solution.

3 Likes

No, I was replying to your weak table question. Just informing that roblox instances do not get garbage collected, even in weak tables.

Though you’re right, checking for a character and destroying before calling LoadCharacter is a very great solution.

4 Likes

Understandable :smile: Pointed out that it’s an outdated comment.

Good to know Roblox instances do not get garbage collected, but I heavily doubted from the start that weak tables would work, considering I don’t know the internal code haha

4 Likes

Bonus bug!
I think there might be something broken with the way player.Character:Destroy() works.

player.Character:Destroy()
player.Character = nil

does NOT behave the same way as just calling

player.Character:Destroy()

Full example:

CharacterAutoLoads = false

while(true) do
	for key,player in game.Players:GetPlayers() do
		if (player.Character) then
			player.Character:Destroy()
			--player.Character = nil   --Comment this line out
		end
		player:LoadCharacter()
	end
		
	wait(0.2)
end

The following code causes the following console error:

which does not happen if you set Character = nil explicitly.

5 Likes

I assume that’s the core movement scripts likely thinking that the character still exists, but it doesn’t. I don’t particularly think that’s a bug, as it’s only throwing a warning. There should be some sort of method though to destroy the character without that workaround.

3 Likes

It is weird that this is still happening after being reported and “fixed” many times. This is a major issue for games that make the player die a lot and do not use custom respawning systems

3 Likes

Would you have to call

player.Character:Destroy()
player.Character = nil

when the player leaves?

4 Likes

This isn’t an issue as long as scripts aren’t holding references to the character after it respawns.

local Character = Player.Character
Player:LoadCharacter()
-- ...Leak!
local Character = Player.Character
Player.CharacterRemoving:Connect(function()
    Character = nil
end)
Player.CharacterAdded:Connect(function(newCharacter)
    Character = newCharacter
end)
Player:LoadCharacter()
-- ...No leak!

Instance:Destroy() is a handy way to remove existing connections and references so that garbage collection can occur, but garbage collection still won’t occur if care isn’t taken to manually discard of old references held in scripts.

References to Instances are never weak, even in weak tables!

3 Likes

That’s what the bug report is about. Opening @MrChickenRocket 's repro file, this is the only script in the baseplate:
image
It does not hold any references to the old character; merely calling ::LoadCharacter.

4 Likes

Oh no! Sorry, I misunderstood the bug report. I thought the OP was reporting that Instance:Destroy() isn’t implicitly called on old characters. If there’s still a leak when there’s no scripts holding references to the old character, that’s a whole other kettle of fish - and it’s quite disappointing that some people mention this happening months ago yet we still don’t have a fix.

2 Likes