Respawn is called when a player enters my game and Players.AutoCharacterLoad is set false.
I’m concerned that I’ve created an infinitely growing closure over many deaths.
What’s the correct pattern to use here?
function Respawn(player)
player:LoadCharacter()
local c = player.Character or player.CharacterAdded:Wait()
print("character loaded")
local h = c.Humanoid or c:WaitForChild("Humanoid")
h.Died:Connect(function() print("DEAD") wait (2) Respawn(player) end)
end
Yes, but not for the reason why you think.
Yielding functions such as Instance:WaitForChild() and Signal:Wait(), keep a reference to the thread in which they were called in. This is because they need to keep this reference in order to then resume the thread at a later date, when they’re intended to return.
The Connection you bound to Humanoid.Died, however; will be properly disconnected without an issue once the character despawns. This is because (assuming you’re properly deleting dead characters) :Destroy() will :Disconnect() all of the Connections, to every Signal, within every one of the Instance’s descendants. But ensure that you’re doing this with Destroy, as simply calling Remove, or Parenting the old Character to nil, will not solve this issue.
This all being said; there’s two theoretical conditions in which a memory leak will occur in this code:
:WaitForChild("Humanoid") never returns.I think only the first one could be feasible (unless something is swooping in and renaming the Humanoid before you get to it). And that would be if a Player leaves before their character finishes loading–which might (strong might) prevent CharacterAdded from firing.
Hello Shedletsky!
I have rewritten some of your code below (what LuckPersonified said is also true).
function respawn(player)
local character = player.Character
if not character then
character = player.CharacterAdded:Wait()
end
if character then
local humanoid = character:WaitForChild("Humanoid",10)
if humanoid then
humanoid.Died:Connect(function()
task.wait(2)
respawn(player)
end)
end
end
end
-- example below
game.Players.PlayerAdded:Connect(respawn)
I learned today.
By the way, :WaitForChild() has an optional 2nd parameter called timeOut that will make the function return nil instead of yielding forever. Based on that, would this be the best way to respawn a character?
--!strict
-- Script in ServerStorage
local Players = game:GetService("Players")
local function respawn(player: Player)
player:LoadCharacter()
end
Players.PlayerAdded:Connect(function(player: Player)
player.CharacterAdded:Connect(function(character: Model) -- Prevents Character:Wait() infinite yield
local humanoid = character:WaitForChild("Humanoid", 12) -- Prevents Humanoid infinite yield
if humanoid then
humanoid.Died:Connect(function()
print("DEAD")
task.wait(2)
respawn(player)
end)
end
end)
task.wait(4)
respawn(player)
end)
Yeah. I think using a Connection for CharacterAdded, and adding a timeout for WaitForChild(“Humanoid”) is definitely the way to go. Your Respawn code is fine, as I believe that LoadCharacter additionally cleans up the exiting character should one persist. Omitting a debounce for Humanoid.Died is also fine, as I believe there’s no longer a risk for it firing multiple times (like it used to when the Humanoid was healed and redamaged post-death).
I would however note, that it might be a good idea to respawn the character if WaitForChild() results in nil. Otherwise they’d be unable to respawn with no clue as to why. Stalling a full twelve seconds for the Humanoid might also be a bit overkill.
I’ve never had an encounter where the Humanoid wasn’t added into a Character when the character loads in, but that sounds like a nice solution to an edge-case that could possibly happen (but immediately respawning them could create an infinite-respawning cycle and crash the game if the bug was persistant. Maybe a 2 second delay is good Nevermind, the timeOut solves this).
Thanks for your input! I’m going to lower the timeOut to 6 seconds.
I don’t think the issue has anything to do with timeouts. I’m more concerned about the case where the character respawns 100 times.
I believe I’ve created a closure like this:
1st death: f(n)
2nd death: f(f(n))
3rd death: f(f(f(n)))
I just want to know the right way to do it. None of the languages I use all the time have closures, so I don’t know the patterns.
Hello Shedletsky!
It is referencing the same function in memory (respawn as defined) so you should theoretically be fine. I did make some changes in my message above that would lead to a more performant and safe respawn function (especially with the waitforchild timeouts).
WaitForChild issues aside…
The function call to Respawn wouldn’t get any of the upvalues from the calling function, so I don’t know why the closure would grow at all. Closures aren’t recursive like that, they’re made at parsing time not run time. (EDIT: I misspoke here—they’re lexically scoped but are runtime values. Still, because they’re lexically scoped, I don’t think it’s possible to create a recursive closure)
c is not accessible once we call another function)That’s my reasoning anyways. Someone can correct me if I misspoke.
Expanding on (amending?) my previous post:
You’re not creating wrapped closures.
The only closure that’s created is for the anonymous function inside the event handler which pulls in the upvalues player, c, and h.
The inner call to Respawn wouldn’t be transferred that closure or anything, because there’s no way for it to access those upvalues.
The initial closure created in the outer call is cleaned up by the gc once the event is disconnected and the player upvalue is no longer needed by anyone.
Edit: if quenty’s right that the humanoid isn’t destroyed then I guess this is a leak since the event’s never disconnected. Oops!
Destroy isn’t actually called on characters being removed from the game.
The correct way is to record each connection and manually disconnect the event when you’re done listening. So for example, disconnecting the CharacterAdded event when a player leaves, or disconnecting the .Died event whenever the character is removed or whenever a new character spawns in.
The pattern I end up using is a Maid object.
This is exactly the right way to do things. It’s the most stable and predictable solution, and maid objects make it easy. My projects became enormously more stable when I started using a maid for each player when they joined.
Is there a reason why this behavior exists and is it 100% guaranteed (for example, does it disconnect if there is no longer any reference to the character)? Also, I heard this happens with the Player class as well. Is this also true?
I was able to replicate the connection being kept for the character using this script:
-- Script in ServerScriptStorage
-- 1 player only
local Players = game:GetService("Players")
local connection = nil
Players.PlayerAdded:Connect(function(player)
player.CharacterRemoving:Connect(function()
print("Character removing: ", connection.Connected)
end)
player.CharacterAdded:Connect(function()
print("Character added: ", connection.Connected)
end)
local character = player.CharacterAdded:Wait()
connection = character.ChildAdded:Connect(function() end)
task.wait(4)
--character:Destroy() -- Adding this will clean the connection
player:LoadCharacter()
end)
You’re correct. This code actually creates duplicates of the Character, instead of erroring as one might expect. It’s completely ridiculous that this is the current functionality.
local Players = game:GetService("Players")
Players.PlayerAdded:Connect(function(Player)
Player.CharacterAdded:Connect(function(Character)
task.defer(Player.LoadCharacter, Player)
Character.AncestryChanged:Wait()
task.delay(1, function()
Character.Parent = workspace -- Perfectly valid.
end)
end)
end)
This also appears to be true.
game:GetService("Players").PlayerAdded:Connect(function(Player)
task.wait(1)
Player:GetPropertyChangedSignal("AutoJumpEnabled"):Connect(function()
print("Not disconnected.", Player.Parent) --> Not disconnected. nil
end)
task.defer(Player.Kick, Player)
Player.AncestryChanged:Wait()
Player.AutoJumpEnabled = not Player.AutoJumpEnabled
end)
This is especially dangerous considering that all of the API documentation I’ve found; either makes these assumptions, or outright incorrectly states that they’re Destroyed. So I know for a fact that these habits are extremely common. Developers just shouldn’t have to worry about Roblox not calling :Destroy() this late in. I’m going to go look for a Feature Request or Bug Report to like.
Here’s the boilerplate code that I use for any PlayerAdded/CharacterAdded stuff, just to make my life much much easier. Took me awhile to make, and I haven’t encountered any problems with it after 100’s of tests or in my games with tens of millions of visits. I haven’t been able to find any memory leaks within it, and I haven’t ran into any in my games, so I believe it’s safe from leaking memory. It works with custom characters/:LoadCharacter() as well (except for the CharacterAppearanceLoaded function if you’re using a custom character by setting player.Character). Hopefully it can make your life a lot easier as well, and let me know if you run into any issues. Enjoy!
----- On Player and Character added -----
function CharacterAppearanceLoaded(player, character) -- NOTE: This does not fire if a player spawns with a Custom Character
end
function OnCharacterDeath(player, character)
end
function OnFirstRespawn(player, character)
end
function OnCharacterRespawn(player, character)
end
function CharacterRemoving(player, character) -- Fires when the character's parts are REMOVED, not when they die
end
function PlayerAddedEvents(player)
end
function PlayerRemoving(player)
end
-----------------------------------------------------------------------------------------------------
-------- Do NOT change the PlayerAdded/CharacterAdded functions below unless a bug is found. --------
-----------------------------------------------------------------------------------------------------
-- Do NOT add anything new to this function. Use OnCharacterRespawn and OnCharacterDeath.
function CharacterAdded(player, character)
local humanoid = character:WaitForChild("Humanoid") -- WHEN THE CHARACTER DIES
humanoid.Died:Connect(function()
OnCharacterDeath(player, character)
-- ValueObject instead of Attributes on purpose
local JustDied = Instance.new("BoolValue")
JustDied.Name = "JustDied"
JustDied.Value = true
JustDied.Parent = player
end)
local JustDied = player:FindFirstChild("JustDied")
if JustDied then
JustDied:Destroy()
OnCharacterRespawn(player, character)
else
OnFirstRespawn(player, character)
end
end
-- Do NOT add anything new to this function. Use PlayerAddedEvents.
function PlayerAddedForEarlyJoinedPlayers(player) -- This function is a mess but handling early joined players/Play Solo is annoying.
PlayerAddedEvents(player)
local earlyCharacter = player.Character
if earlyCharacter ~= nil then
if player.HasAppearanceLoaded then
player.CharacterAppearanceLoaded:Connect(function(character) CharacterAppearanceLoaded(player, character) end)
CharacterAppearanceLoaded(player, earlyCharacter)
else
player.CharacterAppearanceLoaded:Connect(function(character) CharacterAppearanceLoaded(player, character) end)
end
player.CharacterAdded:Connect(function(character) CharacterAdded(player, character) end) -- Connect the event quickly before carrying out the actions
CharacterAdded(player, earlyCharacter)
else
-- Even though we do the CharacterAdded events once for early players, we still have to make these event connections for when the character respawns
player.CharacterAppearanceLoaded:Connect(function(character) CharacterAppearanceLoaded(player, character) end)
player.CharacterAdded:Connect(function(character) CharacterAdded(player, character) end)
end
player.CharacterRemoving:Connect(function(character) CharacterRemoving(player, character) end)
end
function PlayerAdded(player)
player.CharacterAppearanceLoaded:Connect(function(character) CharacterAppearanceLoaded(player, character) end)
player.CharacterAdded:Connect(function(character) CharacterAdded(player, character) end)
player.CharacterRemoving:Connect(function(character) CharacterRemoving(player, character) end)
PlayerAddedEvents(player)
end
-----------------------------------------------------------------------------------------------------
-----------------------------------------------------------------------------------------------------
-----------------------------------------------------------------------------------------------------
local Players = game:GetService("Players")
-- In case Players have joined the server earlier than this script ran:
for _, player in ipairs(Players:GetPlayers()) do
coroutine.wrap(PlayerAddedForEarlyJoinedPlayers)(player)
end
----- Connections -----
Players.PlayerAdded:Connect(PlayerAdded)
Players.PlayerRemoving:Connect(PlayerRemoving)