X:WaitForChild(Y) leaks Lua thread if X is destroyed

Bug Reports Engine Bugs
#1

If you do x:WaitForChild(y), and y is never added under the object x, and subsequently Destroy is called on object x in another thread, then the thread which is yielded for the WaitForChild is not cleaned up. This leaks Lua threads because these threads can subsequently never resume from the WaitForChild yield.

How to reproduce:

  1. Open a baseplate.
  2. Add a Script to workspace, with following source:
local watch = {}
setmetatable(watch, {__mode='kv'})

spawn(function()
    local canary = {}
    watch[canary] = canary
    script.Folder:WaitForChild("Foo")
    print("hello")
end)

wait(0.5)
script.Folder:Destroy()
wait(1.0)

print(next(watch)) -- check whether thread cleaned up
  1. Create a Folder under the Script with name “Folder” (default).
  2. Press Run in Studio and watch output.

Alternatively, I have packaged up the script + folder here: WaitForChild_Leak_Repro.rbxm (802 Bytes)

Observed behavior:

The output says:

table: 38ECFD9C table: 38ECFD9C

Which is not nil, meaning the canary table was not cleared from the weak table, meaning the thread where WaitForChild is called is not cleaned up. The thread never resumes though, so the thread is leaked.

Expected behavior:

The thread should be cleaned up because the object that WaitForChild was called on is destroyed. In turn, “nil” should be printed, because the canary table in the weak table has no other references to it, so it should be garbage collected.

Additional information:

Credit to @Tiffnix for the repro code.

18 Likes
WaitForChild should stop if instance is destroyed
#2

In case anyone is having a problem with this in the future:

Here is a workaround that won’t leak a thread if the instance is destroyed:

local function watertightWaitForChild(object, name)
	local child = object:FindFirstChild(name)

	while not child do
		wait()

		if not object.Parent then
			coroutine.yield()
		end

		child = object:FindFirstChild(name)
	end

	return child
end
#3

Polling like that is bad practice. Here’s a better solution using events:

-- returns the first child with a given name, or waits for it to be added.
-- if the parent is destroyed while waiting, returns nil
local function safeWaitForChild(parent, childName)
    -- if the child's already there, we can return immediately
    local child = parent:FindFirstChild(childName)
    if child then
        return child
    end
    
    -- create a 'resolve' event that will fire with the result
    local resolveEvent = Instance.new "BindableEvent"
    
    -- if a child is added with the correct name, resolve with the child
    parent.ChildAdded:Connect(function(child)
        if child.Name == childName then
            resolveEvent:Fire(child)
        end
    end)

    -- if the parent is removed from the data model, resolve with nil
    parent.AncestryChanged:Connect(function()
        if not parent:IsDescendantOf(game) then
            resolveEvent:Fire(nil)
        end
    end)

    -- once the resolve event fires, clean it up
    resolveEvent.Event:Connect(function()
        resolveEvent:Destroy()
    end)
    
    -- wait for a result
    return resolveEvent.Event:Wait()
end

EDIT: updated based on feedback from @Quenty:

For reference, my previous solution: 
local function safeWaitForChild(parent, childName)
    local child = parent:FindFirstChild(childName)
    if child then
        return child
    end

    while parent.Parent do
        child = parent.ChildAdded:Wait()
        if child.Name == childName then
            return child
        end
    end
end

EDIT 2: alright I’ll stop editing my posts @Anaminus… wait, this is an edit, isn’t it?

oops!

3 Likes
#4

Hence the reason I always use the second parameter of WaitForChild when I’m not sure if the child will be added. I typically set it to 300 seconds.

Heads up: It appears Roblox doesn’t properly destroy some instances (like characters). Thus, even if the behaviour of WaitForChild was adjusted, waiting for a child inside a player character won’t yield different results if the player respawns / leaves.

1 Like
#6

Perhaps while true do? I’m not sure there’s a circumstance where we’d ever want this to return nil.

1 Like
#7

If you want, sure - I only wrote the code snippet as a quick demonstration, so it’s completely untested.

#8

Note, this may not work out for you if 2 children are added at once, or if the parent above that parent’s hierarchy is removed from the game.

It’s better to connect to an event, and then disconnect from them, returning, or passing in a cancellation token to cancel stuff.

Promises can wrap this in a safe way.

1 Like
#9

Can you explain this? From what I can tell, it works fine.

Adding a child fires ChildAdded immediately, setting the calling thread’s status to normal. The looping thread is then resumed, processing the child. Then it loops back around and yields, ready to process the next child. Skipping an added child might happen if the loop yielded some other way first, but that isn’t the case here.

I’m not sure what you mean about ancestors. Is WaitForChild affected by ancestry changes somehow?

Wait, I wasn’t reading the wrong snippet. Stop editing your posts @Elttob >:(

1 Like
#10

I actually think I might be mistaken here. I had issues in my require system before using this sort of loop, but I think maybe the require() were blocking, so this would cause issues.

Basically, I prefer to connect and disconnect the events instead of using a loop here, due to having this sort of code fail in the past.

I suspect this was mostly my fault, and not an actual issue with the core logic. This does feel slightly harder to do safely, but I think you’d be fine.

No, this was also a miscommunication, but a loop like this is unsafe. We want a slightly different condition than parent.Parent!

Instead, we want to do child:IsDescendantOf(game) or something like that.