Problem with trying to disconnect maid class 'tasks'

Help and Feedback Scripting Support
#1

Hi!

Explanation

So basically I’m working on trying to minimize memory leaks by using my own module inspired by the ‘Maid Class’ that was explained about here.

I was able to make it work nicely up until I found out that if I never manually finish the tasks the task will hang around in memory forever. An example is if a UI gets deleted on the client and the script inside of it created a task via the Maid module but was never able to finish the task when it was meant to because that script is now gone.

There’s a module in my game called ‘Maid’ that is shared between the server and the client for garbage collection on many things like temporary events. Outside scripts require the Maid module and can either create a task (Maid:CreateTask(Task, Script) or “finish” a task (Maid:FinishTask(TaskIndex)).
There is a table inside of the Maid module that holds all of the tasks with an index that is returned by CreateTask and then the same index can be used to finish the same task by FinishTask.

Problem

For some unknown reason line 52 of the Maid module (The AncestryChanged event) is not firing for me and it seems like it is mysteriously getting automatically disconnected, (Perhaps because when the Instance is being removed it also disconnects everything with a connection in CreateTask?).

I notice at the same time right below it if I delete the script after it outputs “A” and before it outputs “B”, it will never print B. It prints B if I don’t delete it before the time ends.

Snippet from the Maid module:

local Connection
Connection = Script.AncestryChanged:Connect(function()
	warn("Removed.") --This is never printing when removing the script passed.
	if not Script.Parent then
		Maid:FinishTask(Index)
		Script:Destroy()
		Connection:Disconnect()
		Connection = nil
	end
end)

coroutine.wrap(function()
	print("A")
	wait(5)
	print("B", Connection)
end)()

Code

Maid module:

-- Mirror
local Maid = {}

-- Variables
Maid.Tasks = {}

-------------------------------

--[[
	This module is used for basic disconnection and destroyal of both [Instances] and [Connections] when needed to keep 
	the game clean.
	
	'Tasks' is the word used for values that are limited in lifetime and eventually need to be disconnected by the script.
	These tasks can either be a single value (like :CreateTask(Instance or Connection)) or a table of multiple values
	(like :CreateTask({Instance, Instance, Connection}). Single values can be used for single copies of items that need
	to be disconnected in the future and tables can be used for groups of values that all need to be disconnected at once,
	usually related to each other in some way.
	
	NOTE: Only values directly inside of the table given will be disconnected. Any values nested under a table inside of
	the primary table will be completely ignored.
	
	Example usage:
	
	local Task = Maid:CreateTask({
		OnItemAdded = Instance.new("BindableEvent").Event,
		OnItemRemoved = Instance.new("BindableEvent").Event
	}
	
	Item.AncestryChanged:Wait()
	
	if not Item.Parent then
		Maid:FinishTask(Task) -- Task in this case is actually just the index of the task. Looks much better.
	end
]]

function Maid:GetTaskCount()
	return #Maid.Tasks
end

function Maid:CreateTask(Task, Script)
	assert(Task, "A task value is required to create a new task.")
	assert(Script, "A reference to the script is required to create a new task.")
	
	local Index = Maid:GetTaskCount() + 1
	
	Maid.Tasks[Index] = Task
	
	print("Task created: ".. Index ..".")
	print(Script:GetFullName())
	
	local Connection
	Connection = Script.AncestryChanged:Connect(function() -- PSA: AncestryChanged isn't reliable.
		warn("Removed.")
		if not Script.Parent then
			Maid:FinishTask(Index)
			Script:Destroy()
			Connection:Disconnect()
			Connection = nil
		end
	end)
	
	coroutine.wrap(function()
		print("A")
		wait(5)
		print("B", Connection)
	end)()
	
	return Index
end

function Maid:FinishTask(Index)
	local Task = Maid.Tasks[Index]
	
	if Task then
		if typeof(Task) == "table" then
			for _Index, Value in pairs(Task) do
				if typeof(Value) == "RBXScriptConnection" then
					Maid.Tasks[Index][_Index]:Disconnect()
				elseif typeof(Value) == "Instance" then
					Maid.Tasks[Index][_Index]:Destroy()
				end
				Maid.Tasks[Index][_Index] = nil
			end
		elseif typeof(Task) == "RBXScriptConnection" then
			Maid.Tasks[Index]:Disconnect()
		elseif typeof(Task) == "Instance" then
			Maid.Tasks[Index]:Destroy()
		end
		
		Maid.Tasks[Index] = nil
		print("Task finished: ".. Index ..".")
	end
end

coroutine.wrap(function() -- Temporary loop used to make sure it is all being GCed properly.
	while true do
		print("Tasks in memory: ".. Maid:GetTaskCount())
		wait(10)
	end
end)()

return Maid

Script used to test out the module:

-- Services
local ReplicatedStorage = game:GetService("ReplicatedStorage")

-- Modules
local Maid = require(ReplicatedStorage.Maid)

---------------------------

local Tasks = {}

for i = 1, 5 do
	local Task = Maid:CreateTask(workspace:GetPropertyChangedSignal("DistributedGameTime"):Connect(function()
		print("Task set 1 running")
	end), script)
	
	table.insert(Tasks, Task)
end

--for _, Task in pairs(Tasks) do
--	Maid:FinishTask(Task)
--end

Extra question if it’s not too much to ask: What are the benefits of using metatables/OOP over not using it like shown above with a Maid object? Every other implementation of the Maid object that I’ve seen is always using metatables so I’m wondering if it’s just a preference or if there’s more use to it that can’t be pulled off without metatables.

Thank you!

#2

First, an event connection is already bound to the script that created it, so if the script is removed, that connection is automatically disconnected. Note that this association depends on the stack, rather than where the connection code is located. For example, a ModuleScript defines a function that connects to an event. When a separate Script calls this function, the created connection is associated with the Script rather than the ModuleScript.

Anyway, a Maid is used to manage a context of event connections. For example, when displaying a dialog with buttons, you might assign all the button click events of that dialog to a Maid object. When the dialog is closed, that Maid disconnects all of its assigned connections. Then that particular Maid object, having served its purpose, is discarded along with the rest of the dialog. (how cruel!) This is why Maid is an instantiable class rather than a singleton as you’ve implemented; so that any number of Maids can be created to manage any number of contexts at a time.

You don’t have to use metatables specifically to implement classes, that’s just the preferred method.

2 Likes
#3

This post was originally written in response to the following message, and is being reposted here for informational purposes:

A singleton in the context of classes refers to a class that can have only one instance. To start, consider how a class might be declared. I’ll use the most common method with metatables:

local Class = {}
Class.__index = Class

function Class.new()
	local instance = setmetatable({
		foo = "bar",
	}, Class)
	return instance
end

function Class:Foo()
	return self.foo
end

return Class

To make this a singleton, the constructor could be limited to allow only one instance:

local Class = {}
Class.__index = Class

local instance
function Class.new()
	if instance then
		-- Return the singleton if it already exists.
		return instance
	end

	-- The singleton does not yet exist; create it.
	instance = setmetatable({
		foo = "bar",
	}, Class)
	return instance
end

function Class:Foo()
	return self.foo
end

return Class

It might be done this way to use the singleton pattern while also retaining the class pattern for consistency with other classes (it’s using the class pattern, so it must be a class). If this aspect isn’t important, then we could just skip the constructor and return an “instance” directly:

local Class = {
	foo = "bar",
}

function Class:Foo()
	return self.foo
end

return Class

If the usual pattern is to have classes, then this might be referred to as an “instance” only because of convention. It could also just be referred to as a table with some keys. I chose to refer to your implementation of Maid as a singleton to make an association with the class pattern, which is how Maids are usually implemented.

Aside: you can also implement classes without metatables:

local Class = {}

function Class.new()
	local instance = {
		foo = "bar",
	}

	function instance:Foo()
		return self.foo
	end

	return instance
end

return Class

The only difference is that the methods are placed directly in the instance table rather than the metatable. Metatables are usually preferred because they have more benefits, such as grouping the methods of a class in one location to reduce memory usage, or implementing operators via metamethods (such as __add, allowing things like Class + Class).

This was a mistake on my part. A connection is bound to a thread rather than a stack. But I can explain stacks anyway:

Explanation of stacks

The stack, or more specifically the call stack in this case, is the list of functions that are currently executing. When a function is called, it is added to the top of the stack. When it returns, it is removed from the top of the stack. This allows the code to know where to go back to when a function returns.

Consider this code:

function C()
	print("STACK:")
	-- Print the current call stack.
	print(debug.traceback())
end

function B()
	C()
end

function A()
	B()
end

A()

Running this code prints the following:

STACK:
ServerScriptService.Script:4 function C
ServerScriptService.Script:8 function B
ServerScriptService.Script:12 function A
ServerScriptService.Script:15

The script runs, calling function A, which calls B, which calls C, which prints a stack trace. This result shows that the current function is C, which was called by B, which was called by A, which was called by the script.

Anyway, when a script runs in Roblox, a thread is created and associated with that script. This is how the code within the script actually runs. coroutine.running() can be used to get the thread in which the code is currently running:

print(coroutine.running())
--> thread: 0x67497c82a1058230

When Event:Connect() is called, the resulting connection is bound to the script associated with the running thread. When a script leaves its container[1], such as being destroyed, any bound connections are automatically disconnected.

Now that you know more about connections, you should reassess whether this module is necessary. For example, if you only ever plan on passing to CreateTask the same script that the connections are bound to, then the module itself is unnecessary, since Roblox already does it for you.

On the other hand, if you know that such a script will never be removed, but will need to connect and disconnect events over its lifetime, then the classic Maid pattern may be more suitable.

Finally, if, for some reason, you do need to associate connections with a different script, then it is possible, but nontrivial. Your module must be modified so that it creates a connection to the script within a thread associated with a different script.

To refresh, the reason that Script.AncestryChanged wont fire is because the connection is bound to Script. This happens because Script itself is calling Script.AncestryChanged:Connect(). Bound connections get disconnected when the bound script is removed, so AncestryChanged never gets a chance to fire. If we can call Script.AncestryChanged:Connect() from somwhere other than Script, then it will fire when Script is removed.

This turns out to be possible by using coroutines. The gist is that coroutine.create associates the new thread with the same script as the running thread.

-- NewThread is associated with the same script as CurrentThread.
CurrentThread = coroutine.running()
NewThread = coroutine.create(function() end)

This association will persist regardless of where the thread is resumed. So, all we have to do is wrap the call to Connect in a thread created by another script, such as the Maid ModuleScript.

As an example, here is your module with the following modifications:

  1. Creates thread “CallInModuleThread”. Each time this is resumed with a function passed as an argument, that function is called from within the thread, which is associated with the ModuleScript. The thread immediately yields back, so there are no delays to worry about.
  2. AncestryChanged is connected by using CallInModuleThread, causing the connection to be bound to the ModuleScript.
  3. The AncestryChanged listener no longer destroys the script. Destroying the script disconnects all events, preventing the remaining AncestryChanged listeners from running.
-- Mirror
local Maid = {}

-- Variables
Maid.Tasks = {}

-------------------------------

--[[
	This module is used for basic disconnection and destroyal of both [Instances] and [Connections] when needed to keep
	the game clean.

	'Tasks' is the word used for values that are limited in lifetime and eventually need to be disconnected by the script.
	These tasks can either be a single value (like :CreateTask(Instance or Connection)) or a table of multiple values
	(like :CreateTask({Instance, Instance, Connection}). Single values can be used for single copies of items that need
	to be disconnected in the future and tables can be used for groups of values that all need to be disconnected at once,
	usually related to each other in some way.

	NOTE: Only values directly inside of the table given will be disconnected. Any values nested under a table inside of
	the primary table will be completely ignored.

	Example usage:

	local Task = Maid:CreateTask({
		OnItemAdded = Instance.new("BindableEvent").Event,
		OnItemRemoved = Instance.new("BindableEvent").Event
	}

	Item.AncestryChanged:Wait()

	if not Item.Parent then
		Maid:FinishTask(Task) -- Task in this case is actually just the index of the task. Looks much better.
	end
]]

function Maid:GetTaskCount()
	return #Maid.Tasks
end

-- Calls function passed to coroutine.resume.
local CallInModuleThread = coroutine.create(function()
	while true do
		local func = coroutine.yield()
		func()
	end
end)
-- Initialize CallInModuleThread so that it is ready to receive functions.
coroutine.resume(CallInModuleThread)

function Maid:CreateTask(Task, Script)
	assert(Task, "A task value is required to create a new task.")
	assert(Script, "A reference to the script is required to create a new task.")

	local Index = Maid:GetTaskCount() + 1

	Maid.Tasks[Index] = Task

	print("Task created: ".. Index ..".")
	print(Script:GetFullName())

	coroutine.resume(CallInModuleThread, function()
		local Connection
		Connection = Script.AncestryChanged:Connect(function()
			if not Script.Parent then
				warn("Removed.")
				Maid:FinishTask(Index)
				Connection:Disconnect()
				Connection = nil
			end
		end)
		coroutine.wrap(function()
			print("A")
			wait(5)
			print("B", Connection)
		end)()
	end)

	return Index
end

function Maid:FinishTask(Index)
	local Task = Maid.Tasks[Index]

	if Task then
		if typeof(Task) == "table" then
			for _Index, Value in pairs(Task) do
				if typeof(Value) == "RBXScriptConnection" then
					Maid.Tasks[Index][_Index]:Disconnect()
				elseif typeof(Value) == "Instance" then
					Maid.Tasks[Index][_Index]:Destroy()
				end
				Maid.Tasks[Index][_Index] = nil
			end
		elseif typeof(Task) == "RBXScriptConnection" then
			Maid.Tasks[Index]:Disconnect()
		elseif typeof(Task) == "Instance" then
			Maid.Tasks[Index]:Destroy()
		end

		Maid.Tasks[Index] = nil
		print("Task finished: ".. Index ..".")
	end
end

coroutine.wrap(function() -- Temporary loop used to make sure it is all being GCed properly.
	while true do
		print("Tasks in memory: ".. Maid:GetTaskCount())
		wait(10)
	end
end)()

return Maid

Footnote [1]

In order to run, and continue running, a script must have an ancestor that is a “script container”, or an instance designated as being able to run scripts. Workspace and ServerScriptService are examples of such. A script’s thread will persist as long as the script is within any script container. The thread is killed if the script moves outside of any script containers, or if the script’s Disabled property is set to true.

4 Likes