Destroying a ModuleScript doesn't stop execution

[acreol]

Issue Type: Other
Impact: High
Frequency: Often
Date First Experienced:
Date Last Experienced:

Reproduction Steps:
In a ModuleScript required via Client,Server,or command bar, this runs indefinitely. But in a Client or Server script execution stops after 3 seconds as expected.

game:GetService'Debris':AddItem(script,3)
while wait() do
	print'hi'
end

return nil

Expected Behavior:
The ModuleScript should stop execution after being destroyed.

Actual Behavior:
The ModuleScript runs indefinitely despite being destroyed—there is no way to stop execution.

Workaround:

[EssenceExplorer]

EDIT - some small spelling corrections.

Hi! I wouldn’t say this is a bug, but rather expected behaviour. I can not confirm how require() works internally, however, I can provide some observations and try to prove them. See the steps taken below.

My hypothesis:

1. Once the module script is required, it is strongly referenced, so there is no way for it to be clear from memory by lua garbage collector, unless the script that holds the reference is destroyed as well. Module is available to the script at all times.

2. When the module is required, itscontent is loaded into the script and is thus part of that script, so strong reference applies.

Attempt 1

Module script code

game:GetService('Debris'):AddItem(script, 3)

while (true) do
	game:GetService('RunService').Heartbeat:Wait()
	print("Hi")
end

return nil

Script code

require(script.Module)

Hierarchy:

Findings: Module code continues to run after the module is destroyed.

Important detail is that Module is removed from the game, although it remains in memory. Proof:

game:GetService('Debris'):AddItem(script, 3)

while (true) do
	game:GetService("RunService").Heartbeat:Wait()
	print(script.Parent)
end

return nil

-- OUTPUT BEFORE REMOVING:
-->> Script
-- OUTPUT AFTER REMOVING:
-->> nil

Attempt 2

Same module script.

Script code

require(script.Module)
print("EXECUTED") -- never runs, same hierarchy.

Attempt 3

Wrapper code

wait(5)
script.Script:Destroy()

Script code

require(script.Module)
print("EXECUTED") -- doesn't run.

Same module script.

Hierarchy:

Findings: Module script doesn’t run anymore, neither does script (parent). No values printed in the output. print(“EXECUTED”) doesn’t run either.

Attempt 4

Module script code

game:GetService('Debris'):AddItem(script, 3)

local module = {}

function module.sayHello()
	while (true) do
		game:GetService("RunService").Heartbeat:Wait()
		print(script.Parent)
	end
end

return module

Script code

local Module = require(script.Module)

wait(5) -- until destroyed
Module.sayHello()

Code runs successfuly!

Conclusion

As previously said, I don’t know how require works exactly, but can confirm that at least one of above stated hypothesis is true and valid. There is perhaps the difference between

require(script.Module)

and

local Module = require(script.Module)

But ultimately, it isn’t important. Former is a standard way of requiring a module, and direct call is used when the script has to be ran immediately. If we try to destroy the module script before requiring it, that also doesn’t make sense, because module is not accessible, probably scheduled for removal from memory, and no game paths are valid.

This doesn’t seem like a bug. No workaround is needed, because it all comes down to how code is written. Infinite loops in module scripts behave normally, but are not the best practice. A better way would be wrapping them in functions, which are then called by other scripts in various ways. To my knowledge, module scripts that remain idle in memory are pretty performance friendly, unless they are running and doing heavy tasks.

[VegetationBush (FartedAtLightspeed)]

I believe it’s because requiring a module is just like an addon to a script(it’s cached), for example, if the module script had this code:

local module = {}
function module.Whatever()
    while wait() do end
end
return module

And the script had this code:

local A = "a"
local B = "b"
local C = "c"
print(A) -- Prints
local module = require(script.module)
print(B) -- Prints
module.Whatever()
print(C) -- Does not print

The module would just be a part of the script, so the script above is the same as this:

local A = "a"
local B = "b"
local C = "c"
print(A) -- Prints
local module = {
    function module.Whatever()
        while wait() do end
    end
}
print(B) -- Prints
module.Whatever()
print(C) -- Does not print

I try to simplify I by thinking the module is just a storage item that is embedded in whatever script requires it.

[acreol]

Bug requests can always be interpreted as feature requests and vice versa. I don’t think developers should have to debug and speculate on why undocumented, unintuitive behavior occurs.

If your theory about ModuleScripts continuing to run while strong references exist is true, then I see no reason why Module:Destroy() must not disconnect connections—unless Roblox deliberately chose to do this to prevent the script from somehow only partially running instead of fixing the underlying issue e.e so I guess maybe this is deliberate behavior. (The .parent property, however, is locked upon script:Destroy() call).

game:GetService'Debris':AddItem(script,3)
workspace.Changed:Connect(function()--destroy should disconnect all script connections, but this persists
	print'con not disconnected'
end)
while wait() do
	
end

return nil

But fwiw I believe the following code should GC—and if it did, then Roblox might be able to cleanly patch this bug report as well

@acreol

local x={}
coroutine.wrap(function()
	local t=setmetatable({x},{__mode='v'})
	repeat wait()until not t[1]
	print'gced'
end)()

[EssenceExplorer]

You are right, bug reports can also represent feature requests, and yes, there definitely is a lack of thorough documentation.

As for connections, vanilla lua provides more flexibility and freedom when it comes to garbage collection. As you already know, Roblox engine manages collection by itself, does it routinely, and limits the use of GC by developers. Now, :Destroy() - as opposed to :Remove() - does disconnect connections, as long as, again, there is no reference to the script. Right now, I couldn’t tell why module script connections are not disconnected. Even when wrapping and destroying the script that requires your module, connection indeed still “resists”. This is not the case with any other type of scripts. Is this behaviour deliberate? I don’t know, you had probably better ask a Roblox intern.

@VegetationBush yes, that’s looks plausible.

It’s still a very good practice to disconnect events and functions when they are no longer needed. It’s reliable (luckly, when a server or local script is destroyed, connections are disconnected too). Hard to see yourself what is garbage collected and what not, because you might somehow be referencing the object unintentionally. We could draw an analogy to Greek mythology, which is literally epic , and Orpheus (reference to positive part, because the end of his story is sincerely sad).

Anyway, I’d love to learn more about memory clearning.

[metatablecatmaid (metatablecat)]

TL:DR: the returned table gets strongly referenced, meaning the GC doesn’t delete it

[popeeyy (pop)]

ModuleScripts should stay the way they are as many people rely off this behavior for “security” in various ways to hide their scripts. Doing this would break a lot of modules and produce a new unexpected behavior for many developers. I’m highly against a change like this as there are actually use cases for destroying your ModuleScripts and having them continuing to run, and I can not see why you are destroying your scripts to stop their execution rather than properly implementing a more efficient system that would not involve destroying any ModuleScripts and expecting them to stop their execution.

[buildthomas]

@popeeyy

ModuleScripts should stay the way they are as many people rely off this behavior for “security” in various ways to hide their scripts

Just FYI but I think that use case completely falls through, considering exploiters have full access to the client’s memory and so it is pointless to try and hide them using methods like this. It’d be naive to think reparenting a module stops an exploiter from accessing them. Therefore that alone should not be considered as a valid blocker for requests like this.

[popeeyy (pop)]

There are different ways you can trick exploits into thinking that the object has been destroyed, but it takes more skill for exploiters to actually find your module when you use hacky methods. It’s a basic security defense that I’ve seen catches a lot of exploiters who don’t know what they’re doing and requires them to learn the specific functions of their exploit rather than basic LUA knowledge. Also, if we allowed these modules to be destroyed and stop executing, that would be an easy task to stop an anti exploit anyways. There are also methods that will destroy your module and your module will show in a weird state where exploiters cannot see your module unless they go through very specific steps, invisible to game downloaders and not in nil instances.

[buildthomas]

Security through obscurity (e.g. these stopgap tricks to break certain exploits) should never be a blocker for actual developer features! What you are describing is totally not intended functionality of parenting things to nil, etc. The reason why these tricks are used in practice is because developers do not go through the effort to implement proper server authority mechanisms in their games.

[popeeyy (pop)]

I’m not implying that this should be a replacement to basic security on the server, but rather a supplement. There are a large amount of exploiters who don’t understand anything about what they’re doing and just execute any script under the sun, or are very bad at trying to make exploits. You can put certain things in these modules that will catch a lot of exploiters who don’t know what they’re doing, and this method is actually deployed in top games. I agree that security on the server is very important, but also weeding out the exploiters before they get a chance to do anything is a good idea in the sense that it will take them a LOT longer to develop exploits and is a good deterrent to not mess with your game as it becomes annoying to the point where it’s not worth it.

[tnavarts]

The solution to this problem is that your ModuleScripts should not be executing long-running code directly on require in the first place. Doing so is a very bad idea for a lot of reasons.

Do this in your modules:

local function runLongRunningTask()
    longRunningCall()
end
return {
    RunLongRunningTask = runLongRunningTask,
    DoOtherStuff = ...,
}

NOT this:

longRunningCall()
return {
    DoOtherStuff = ...,
}

[acreol]

I was basically trying to make my own plugin:reload() method, I let much of my framework run in edit mode by requiring the loader module through a plugin, and rather than adding special code to bloat my framework (which again, shares code between client,server,and edit contexts) ie a check to end all loops etc on a _G.reload() call[1], I would much much rather prefer to destroy the modulescripts (I already do clone them etc before requiring to prevent caching). But I understand that the destroy method cannot be fixed because of the legacy noop usage @popeeyy pointed out. So I look forward to plugin:reload().

[1] loops are sometimes necessary eg Roblox lacks a game.updated api, but I can check an asset’s latest version (with surprisingly very little throttling)

[tnavarts]

You still shouldn’t need long-running code to happen inside of a require.

Your “require graph” should be static, and always succeed, and then actual scripts that makes calls on the required modules are the thing that should need to be stopped, rather than the module loading itself containing non-trivial long running execution.

Doing otherwise creates problems like hidden race conditions in your require graph that just happen to work because of the order you wrote your requires in in an entirely different part of the program.

[acreol]

--[[
	event for when this game is updated
--]]
local ev=_G'event'()

coroutine.wrap(function()
	local version=_G.version
	if game.PlaceId==0 then game:GetPropertyChangedSignal'PlaceId':Wait()end
	local last=_G'retry'(nil,_G.insertservice.GetLatestAssetVersionAsync,_G.insertservice,game.PlaceId).finished:wait()
	while wait()and version==_G.version do
		local new=_G'retry'(nil,_G.insertservice.GetLatestAssetVersionAsync,_G.insertservice,game.PlaceId).finished:wait()
		if new~=last then
			last=new
			ev(new)
		end
	end
end)()

return _G.export(ev)

This is what my ‘updated’ module looks like. How would you make it fit your paradigm? I have no race conditions anywhere in my framework.

Edit: I guess I could actually have an event I bind loop code to that will destroy itself on calling _G.reload()—is this what you have in mind? It’s sort of awkward to combine with yielding code though like in this specific module (calling GetLatestAssetVersionAsync).

[tnavarts]

You should have a script which “bootstraps” the module, the module should not run the loop itself:

Bootstrap.lua:

local Updated = require(...Updated)
Updated:StartCheckLoop()

-- Any other long-running setup that has to be done.

Or something like that. Basically, every long-running process should be “owned” by a specific script, not by a module, because that way the module-graph itself remains entirely deterministic regardless of how you require what modules.

You can even batch together multiple different activities into the same loop to avoid scheduling overhead / give you greater control over scheduling that way.

This is a bit hairy if you want to distribute the Module as a re-usable package, but we’re working on nice solutions for that too.

[acreol]

Wow this behavior is really neat, I did not know about any of this (documenting for future):

Script:

local md=require(script.ModuleScript)
coroutine.wrap(md)()
wait(1)
script.ModuleScript:Destroy()
print'ModuleScript destroyed'
wait(1)
script:Destroy()
print'script destroyed'

Child module:

return function()
	while wait()do
		print'runs until caller script is destroyed'
	end
end

Also, in a separate place, this behavior:
Script:

coroutine.wrap(function()
	while wait()do
		print'until destroy1'
	end
end)()

wait(1)
script:Destroy()
print'destroyed1'
coroutine.wrap(function()
	while wait()do
		print'runs forever'
	end
end)()

wait(1)
script:Destroy()
print'destroyed2'