Can someone double check this for memory leaks?

Help and Feedback Code Review
#1 
local api = {}

local function on(lamp)
	lamp.DynamicLight.Enabled = true
	lamp.Lamp.Enabled = true
end

local function off(lamp)
	lamp.DynamicLight.Enabled = false
	lamp.Lamp.Enabled = false
end

function api:analyzeSignalInputCommand(command,config,signalSet)
	if command[1] == "on" then
		local group = command[2]
		for _,trafficSignalName in pairs(group) do
			local signalObject = signalSet.Signals:FindFirstChild(trafficSignalName)
			on(signalObject.Lights:FindFirstChild(command[3]))
		end
	elseif command[1] == "off" then
		local group = command[2]
		for _,trafficSignalName in pairs(group) do
			local signalObject = signalSet.Signals:FindFirstChild(trafficSignalName)
			off(signalObject.Lights:FindFirstChild(command[3]))
		end
	elseif command[1] == "wait" then
		if tonumber(command[2]) then
			wait(command[2])
		end
	end
end

local function handleModeChange(signalSetCycles,signalSetState,newState,signalSetConfiguration,signalSet)
	local cycle = signalSetCycles[signalSetState.Value]
	for _,thread in pairs(cycle) do
		coroutine.resume(coroutine.create(function()
			while wait() do
				for _,command in pairs(thread) do
					if signalSetState.Value == newState then
						api:analyzeSignalInputCommand(command,signalSetConfiguration,signalSet)
					else
						break
					end
				end
			end
		end))
	end
end

function api:initTrafficSignals()
	local signalFolder = workspace:WaitForChild("Traffic Signals")
	
	for _,signalSet in pairs(signalFolder:GetChildren()) do
		local initializedValue = signalSet.Initialized
		local signalSetState = signalSet.State
		
		if initializedValue.Value == false then
			initializedValue.Value = true
			
			local signalSetConfiguration = require(signalSet.Configuration)
			local signalSetCycles = signalSetConfiguration.cycles
			
			handleModeChange(signalSetCycles,signalSetState,signalSetState.Value,signalSetConfiguration,signalSet)
			signalSetState.Changed:Connect(function(newState)
				handleModeChange(signalSetCycles,signalSetState,newState,signalSetConfiguration,signalSet)
			end)
		end
	end
end

return api

This code will run all of the traffic signals in my game when the :init function is called. Considering I don’t use coroutines very often, I want to make sure there is no bad practice here, specifically in the “handleModeChange” function. Can someone just look through that function and double check for any memory leaks/bad practice?

TL;DR - I want to confirm that coroutines automatically gc when the code inside of them is done, assuming that the coroutine has not been saved in a variable anywhere.

1 Like
#2

Don’t know about memory leaks, but instead of making two functions to enable/disable the lamp, just make one function:

local function toggle(boolean,lamp)
    lamp.DynamicLight.Enabled = boolean
    lamp.Lamp.Enabled = boolean
end

-- To enable
toggle(true)

-- To disable
toggle(false)
1 Like
#3

Thanks, but this wasn’t entirely what i’m looking for. It is like that because in the future, the logic for both of those functions will become a bit more in depth and having them separate would be beneficial.

#4

I suggest wrapping the

			signalSetState.Changed:Connect(function(newState)
				handleModeChange(signalSetCycles,signalSetState,newState,signalSetConfiguration,signalSet)
			end)

Inside of a _maid:GiveTask() function. Since its object that can be destroyed, and if that object is destroyed the code will be trying to run this function and since that object isn’t there it wouldn’t be garbage collected.

1 Like
#5

Don’t use a : in functions unless you’re calling self, which you aren’t, I suggest switching api:init to api.init.

1 Like
#6

There are no memory leaks. However to keep things tidy, you should disconnect signalSetState when it isn’t needed.

As a word of advice, make sure that functions who access their upvalues are garbage collected when needed. Here’s a quick and easy to understand example:

In ths code down below; upVal would never get garbage collected because the function holds a reference to it as an up value, so in order for upVal to be garbage collected, the function it self needs to be garbage collected. In this case, the function is never garbage collected because it is connected to a bindable event and thus the connection needs to be disconnected to release the internal reference of the function by the event.

local upVal = {}
local event = Instance.new("BindableEvent")

event:Connect(function()
    print(upVal) -- uh oh, memory leak!
end)
3 Likes
#7

Judging from the first impression, this script itself is really a sensitive victim to Memory Leaks.

1 Like
#8

Not a memory leak, but inefficient. Create a dictionary containing functions, that way, you can just do MyDictionaryName[command[1]](), instead of many if statements.

1 Like