howmanysmall / Janitor

Janitor library.
https://howmanysmall.github.io/Janitor/
MIT License
107 stars 17 forks source link

No deterministic ordering of cleanup functions causes race conditions #26

Open JohnnyMorganz opened 2 years ago

JohnnyMorganz commented 2 years ago

Consider a janitor as such

local janitor = Janitor.new()
local camera = workspace.CurrentCamera

-- doing some work which requires custom camera control ...

-- if camera type changes (e.g. corescript), then change to scriptable it
janitor:Add(camera:GetPropertyChangedSignal("CameraType"):Connect(function()
    camera.CameraType = Enum.CameraType.Scriptable
end)

-- reset camera type to custom on cleanup
janitor:Add(function()
    camera.CameraType = Enum.CameraType.Custom
end)

-- stuff  ...

-- no longer need custom camera control
janitor:Cleanup()

depending on the ordering of the cleanup, camera type may end up as Scriptable if the reset function was called before the connection was disconnected. The ideal scenario is that it should always end up as Custom.

This cropped up when converting existing usage of Quenty's maid to janitor, which had deterministic ordering of cleanup.

Cleanup should ideally be applied using a queue or stack based approach. Or alternatively with a priority mechanism.

JohnnyMorganz commented 2 years ago

This isn't a major concern for me since I can add in checks for Janitor.CurrentlyCleaning, but the nondeterministic cleanup ordering caught me by surprise when migrating, and there may be subtle bugs because of this.

howmanysmall commented 2 years ago

I'll have to think about how to implement a cleanup ordering system, but yeah, I'll get right on that.

howmanysmall commented 2 years ago

The new version will be here.

howmanysmall commented 2 years ago

Wow, I forgot about this. I'll get to work on it after work tomorrow.

JohnnyMorganz commented 2 years ago

No problem, as mentioned earlier, testing for CurrentlyCleaning is sufficient for my use case.

If it's of any help, you might find a LinkedHashMap-like struct useful for this

RealistEntertainment commented 1 year ago

This would be incredibly useful. Setting a priority value would be an easy solution.

howmanysmall commented 1 year ago

There's a separate branch that adds this. It's a bit hacky and slow, but it works. It should be under AlreadyJanitor.