Consider changing Signal class, possible memory leak

[1ForeverHD]

I haven't explored in full, but if someone was able to investigate and confirm, that'd be much appreciated 🙏

Will consider changing Signal classes if true.

Details by Outgram Roblox:

"Hello. I'm writing to report a memory leak within the GoodSignal module that TopbarPlus has been published with. Specifically, the leak occurs within the freeRunnerThread implementation. Assuming the helper thread has yielded for a previous task, and a new one comes in, runEventHandlerInFreeThread creates a new instance of the helper thread, however, the previous coroutine is never properly cleaned up due to the non-terminating loop. This quickly comsumes memory, and with just 10 samples of relatively simple computations, ~400 KB of memory were never freed. As HDAdmin and TopbarPlus use this module, and likely make use of the freeRunnerThread often, this could quickly accumulate to several megabytes of memory usage.

For further clarification, I pasted the affected code into a script in this demonstration. It has been modified to track the state of every coroutine created in an array.

As you can see from the image, the threads are left in a suspended state, originating from runEventHandlerInFreeThread's while true do loop that is intended to pass input to acquireRunnerThreadAndCallEventHandler through the use of coroutine.yield() passing input from resumptions.

The problem is the mishandling of threads that are unable to close, and memory leakage as a byproduct. If the module inside TopbarPlus was operating correctly, all these threads would indicate a 'dead' state, with the exception being the last thread being reused. I should mention that this bug only occurs when the current thread is yielded and a new task triggers coroutine replacement. If they're all tasks that return immediately then everything operates normally."

-- The currently idle thread to run the next handler on
local freeRunnerThread = nil

-- The array we use to track coroutine states
local threadTrack = {}

-- Function which acquires the currently idle handler runner thread, runs the
-- function fn on it, and then releases the thread, returning it to being the
-- currently idle one.
-- If there was a currently idle runner thread already, that's okay, that old
-- one will just get thrown and eventually GCed.
local function acquireRunnerThreadAndCallEventHandler(fn, ...)
	local acquiredRunnerThread = freeRunnerThread
	freeRunnerThread = nil
	fn(...)
	-- The handler finished running, this runner thread is free again.
	freeRunnerThread = acquiredRunnerThread
end

-- Coroutine runner that we create coroutines of. The coroutine can be 
-- repeatedly resumed with functions to run followed by the argument to run
-- them with.
local function runEventHandlerInFreeThread()
	-- Note: We cannot use the initial set of arguments passed to
	-- runEventHandlerInFreeThread for a call to the handler, because those
	-- arguments would stay on the stack for the duration of the thread's
	-- existence, temporarily leaking references. Without access to raw bytecode
	-- there's no way for us to clear the "..." references from the stack.
	while true do
		acquireRunnerThreadAndCallEventHandler(coroutine.yield())
	end
end

local function runInFreeThread(Func:(any)->(), ...:any):nil
	if not freeRunnerThread then
		freeRunnerThread = coroutine.create(runEventHandlerInFreeThread)
		
		-- Get the freeRunnerThread to the first yield and ready to accept
		-- input from further resumptions.
		coroutine.resume(freeRunnerThread)
		
		-- Insert every new thread into an array so
		-- we can track its state.
		table.insert(threadTrack, freeRunnerThread)
	end
	
	task.spawn(freeRunnerThread, Func, ...)
	
	return nil
end

-- The coroutine leakage doesn't happen when tasks return immediately
-- so we must trigger replacement of the coroutines by intentionally
-- waiting. We run this loop 10 times.
for i = 1, 10 do
	runInFreeThread(function()
		-- Small busy work to ensure Thread is nil during execution
		task.wait()
	end)

	-- Immediately call again to trigger replacement
	runInFreeThread(function() end)
end

for Index, Value in threadTrack do
	print('Coroutine #', Index..':', coroutine.status(Value))
end

DevForum post here: https://devforum.roblox.com/t/lua-signal-class-comparison-optimal-goodsignal-class/1387063/98?u=foreverhd

[I-Bat]

Hello. I’m aware of the reported memory leak in GoodSignal (used by TopbarPlus), specifically within the freeRunnerThread implementation.

I’ve been testing a modified version of the coroutine handler to see if it resolves the issue described — namely, the endless “suspended” coroutines created by runEventHandlerInFreeThreads infinite loop.

In this version, instead of nesting coroutine resumes, the user functions are executed via task.spawn. The coroutine itself only coordinates task dispatching and never yields based on user code. The goal is to allow a single reusable suspended coroutine to handle all function calls without creating additional ones.

From testing, this approach appears to reuse a single coroutine even when yielding functions are triggered, meaning the previously “suspended forever” coroutines are no longer generated. Memory usage stays stable across multiple executions.

However, I can’t guarantee that this fully resolves the underlying memory leak in every possible edge case. The change effectively sidesteps the yield-related issue by moving all yielding behavior into task contexts, which might have slightly different scheduling behavior than the original implementation.

In short: this fix reuses one coroutine instead of continuously spawning new ones, and preliminary testing suggests that the leak no longer occurs — though further verification across different use cases would still be needed.

--====================================================
-- Coroutine Reuse Test (GoodSignal style)
--====================================================

-- The currently idle thread to run the next handler on
local freeRunnerThread = nil

-- The array used to track coroutine states
local threadTrack = {}

-- Counters for analysis
local totalThreads = 0
local totalExecuted = 0

-- Runs the user function in task.spawn to prevent blocking the coroutine
local function acquireRunnerThreadAndCallEventHandler(fn, ...)
	local acquiredRunnerThread = freeRunnerThread
	freeRunnerThread = nil
	totalExecuted += 1

	task.spawn(fn, ...)

	freeRunnerThread = acquiredRunnerThread
end

-- Coroutine loop that waits for functions to execute
local function runEventHandlerInFreeThread()
	while true do
		local fn, args = coroutine.yield()
		acquireRunnerThreadAndCallEventHandler(fn, table.unpack(args or {}))
	end
end

-- Executes a function on the free thread or creates one if needed
local function runInFreeThread(Func: (any) -> (), ...: any): nil
	if not freeRunnerThread or coroutine.status(freeRunnerThread) == "dead" then
		totalThreads += 1
		freeRunnerThread = coroutine.create(runEventHandlerInFreeThread)
		coroutine.resume(freeRunnerThread)
		table.insert(threadTrack, freeRunnerThread)
	end

	local ok, err = coroutine.resume(freeRunnerThread, Func, table.pack(...))
	if not ok then
		warn("Thread error:", err)
		freeRunnerThread = nil
	end

	return nil
end

--====================================================
-- Test loop
--====================================================

print("Starting coroutine reuse test...")

for i = 1, 10 do
	runInFreeThread(function()
		task.wait()
	end)
	runInFreeThread(function() end)
end

task.wait(1)

--====================================================
-- Results
--====================================================

local suspended, dead = 0, 0

for i, thread in ipairs(threadTrack) do
	local status = coroutine.status(thread)
	print(string.format("Coroutine #%d: %s", i, status))

	if status == "suspended" then
		suspended += 1
	elseif status == "dead" then
		dead += 1
	end
end

print("----------------------------------")
print("Suspended:", suspended)
print("Dead:", dead)
print("Threads created:", totalThreads)
print("Functions executed:", totalExecuted)
print(string.format("Reuse ratio: %.1fx", totalExecuted / math.max(totalThreads, 1)))

if suspended == 1 and totalThreads == 1 then
	print("Test passed: 1 coroutine reused")
else
	print(string.format("Test failed: expected 1 coroutine, got %d", totalThreads))
end