howmanysmall/Janitor

No deterministic ordering of cleanup functions causes race conditions

JohnnyMorganz opened this issue · 7 comments

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.

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.

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

The new version will be here.

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

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

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

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