Fix loop scheduler (#1)

lib/Loop.lua

@@ -132,7 +132,9 @@ type System = (...any) -> () | { system: (...any) -> (), event: string?, priorit
 ]=]
 function Loop:scheduleSystems(systems: { System })
 	for _, system in ipairs(systems) do
-		self._systems[system] = system
+		if table.find(self._systems, system) == nil then
+			table.insert(self._systems, system)
+		end
 		self._systemState[system] = {}
 
 		if RunService:IsStudio() then
@@ -143,7 +145,6 @@ function Loop:scheduleSystems(systems: { System })
 
 	self:_sortSystems()
 end
-
 --[=[
 	Schedules a single system. This is an expensive function to call multiple times. Instead, try batch scheduling
 	systems with [Loop:scheduleSystems] if possible.
@@ -162,11 +163,14 @@ end
 	@param system System
 ]=]
 function Loop:evictSystem(system: System)
-	if self._systems[system] == nil then
+	local systemIndex = table.find(self._systems, system)
+
+	if systemIndex == nil then
 		error("Can't evict system because it doesn't exist")
 	end
 
-	self._systems[system] = nil
+	table.remove(self._systems, systemIndex)
+
 	self._systemErrors[system] = nil
 
 	topoRuntime.start({
@@ -187,12 +191,15 @@ end
 	@param new System
 ]=]
 function Loop:replaceSystem(old: System, new: System)
-	if not self._systems[old] then
+	local systemIndex = table.find(self._systems, old)
+
+	if not systemIndex then
 		error("Before system does not exist!")
 	end
 
-	self._systems[new] = new
-	self._systems[old] = nil
+	table.remove(self._systems, systemIndex)
+	table.insert(self._systems, new)
+
 	self._systemState[new] = self._systemState[old] or {}
 	self._systemState[old] = nil
 
@@ -201,7 +208,7 @@ function Loop:replaceSystem(old: System, new: System)
 		self._skipSystems[new] = true
 	end
 
-	for system in self._systems do
+	for _, system in self._systems do
 		if type(system) == "table" and system.after then
 			local index = table.find(system.after, old)
 
@@ -215,12 +222,21 @@ function Loop:replaceSystem(old: System, new: System)
 end
 
 local function orderSystemsByDependencies(unscheduledSystems: { System })
-	table.sort(unscheduledSystems, function(a, b)
+	local sortedUnscheduledSystems = table.clone(unscheduledSystems)
+
+	table.sort(sortedUnscheduledSystems, function(a, b)
 		local priorityA = systemPriority(a)
 		local priorityB = systemPriority(b)
 
 		if priorityA == priorityB then
-			return systemName(a) < systemName(b)
+			local nameA = systemName(a)
+			local nameB = systemName(b)
+
+			if nameA == nameB then
+				return table.find(unscheduledSystems, a) < table.find(unscheduledSystems, b)
+			end
+
+			return nameA < nameB
 		end
 
 		return priorityA < priorityB
@@ -229,43 +245,33 @@ local function orderSystemsByDependencies(unscheduledSystems: { System })
 	local scheduledSystemsSet = {}
 	local scheduledSystems = {}
 
-	local explore = 1
-	local visited = 2
-
-	while #scheduledSystems < #unscheduledSystems do
-		local index = 1
+	local visited, explored = 1, 2
 
-		while index <= #unscheduledSystems do
-			local system = unscheduledSystems[index]
+	local function scheduleSystem(system)
+		scheduledSystemsSet[system] = visited
 
-			if scheduledSystemsSet[system] == visited then
-				index += 1
-				continue
-			end
-
-			scheduledSystemsSet[system] = explore
-
-			local allScheduled = true
-
-			if type(system) == "table" and system.after then
-				for _, dependency in ipairs(system.after) do
-					if scheduledSystemsSet[dependency] == explore then
-						error("Unable to schedule systems due to cycle")
-					elseif scheduledSystemsSet[dependency] ~= visited then
-						allScheduled = false
-						break
-					end
+		if type(system) == "table" and system.after then
+			for _, dependency in system.after do
+				if scheduledSystemsSet[dependency] == nil then
+					scheduleSystem(dependency)
+				elseif scheduledSystemsSet[dependency] == visited then
+					error(
+						`Unable to schedule systems due to cyclic dependency between: \n{systemName(system)} \nAND \n{systemName(
+							dependency
+						)}`
+					)
 				end
 			end
+		end
 
-			if allScheduled then
-				scheduledSystemsSet[system] = visited
-				table.insert(scheduledSystems, system)
-				--Once this system is scheduled we want to start from the beginning to schedule systems that have a dependency on this system
-				break
-			end
+		scheduledSystemsSet[system] = explored
+
+		table.insert(scheduledSystems, system)
+	end
 
-			index += 1
+	for _, system in sortedUnscheduledSystems do
+		if scheduledSystemsSet[system] == nil then
+			scheduleSystem(system)
 		end
 	end
 
@@ -275,7 +281,7 @@ end
 function Loop:_sortSystems()
 	local systemsByEvent = {}
 
-	for system in pairs(self._systems) do
+	for _, system in pairs(self._systems) do
 		local eventName = "default"
 
 		if type(system) == "table" then
@@ -294,7 +300,7 @@ function Loop:_sortSystems()
 				end
 
 				for _, dependency in system.after do
-					if not self._systems[dependency] then
+					if not table.find(self._systems, dependency) then
 						error(
 							`Unable to schedule "{systemName(system)}" because the system "{systemName(dependency)}" is not scheduled.\n\nEither schedule "{systemName(
 								dependency

lib/Loop.spec.lua

@@ -164,6 +164,137 @@ return function()
 			connection.default:Disconnect()
 		end)
 
+		it("should not schedule systems more than once", function()
+			local loop = Loop.new()
+
+			local order = {}
+			local systemA = {
+				system = function()
+					table.insert(order, "a")
+				end,
+				t = 1,
+			}
+
+			local systemB = {
+				system = function()
+					table.insert(order, "b")
+				end,
+				t = 2,
+			}
+
+			loop:scheduleSystems({
+				systemA,
+				systemA,
+			})
+
+			loop:scheduleSystem(systemB)
+			loop:scheduleSystem(systemB)
+
+			local connection = loop:begin({ default = bindable.Event })
+
+			expect(#order).to.equal(0)
+
+			bindable:Fire()
+
+			expect(#order).to.equal(2)
+			expect(order[1]).to.equal("a")
+			expect(order[2]).to.equal("b")
+
+			connection.default:Disconnect()
+		end)
+
+		it("should schedule systems in order if dependencies are defined", function()
+			local loop = Loop.new()
+
+			local order = {}
+			local systemA = {
+				system = function()
+					table.insert(order, "a")
+				end,
+			}
+
+			local function namedSystemB()
+				table.insert(order, "b")
+			end
+
+			local systemB = {
+				system = namedSystemB,
+				after = { systemA },
+			}
+
+			--By naming the system functions this ensures that the initial table.sort call in the
+			--system scheduler returns the following order systemC -> systemB -> systemA
+			--and we want to schedule it in this order to ensure that the scheduler is able to complete
+			--without erroring
+			local function namedSystemA()
+				table.insert(order, "c")
+			end
+
+			local systemC = {
+				system = namedSystemA,
+				after = { systemA, systemB },
+			}
+
+			loop:scheduleSystems({
+				systemC,
+				systemB,
+				systemA,
+			})
+
+			local connection = loop:begin({ default = bindable.Event })
+
+			expect(#order).to.equal(0)
+
+			bindable:Fire()
+
+			expect(#order).to.equal(3)
+			expect(order[1]).to.equal("a")
+			expect(order[2]).to.equal("b")
+			expect(order[3]).to.equal("c")
+
+			connection.default:Disconnect()
+		end)
+
+		it("should schedule systems based on order passed into scheduleSystems", function()
+			local loop = Loop.new()
+
+			local order = {}
+			local systemA = {
+				system = function()
+					table.insert(order, "a")
+				end,
+			}
+			local systemB = {
+				system = function()
+					table.insert(order, "b")
+				end,
+			}
+			local systemC = {
+				system = function()
+					table.insert(order, "c")
+				end,
+			}
+
+			loop:scheduleSystems({
+				systemB,
+				systemC,
+				systemA,
+			})
+
+			local connection = loop:begin({ default = bindable.Event })
+
+			expect(#order).to.equal(0)
+
+			bindable:Fire()
+
+			expect(#order).to.equal(3)
+			expect(order[1]).to.equal("b")
+			expect(order[2]).to.equal("c")
+			expect(order[3]).to.equal("a")
+
+			connection.default:Disconnect()
+		end)
+
 		it("should throw error for systems with unscheduled depedencies", function()
 			local loop = Loop.new()