Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No deterministic ordering of cleanup functions causes race conditions #26

Open
JohnnyMorganz opened this issue Jul 2, 2022 · 7 comments

Comments

@JohnnyMorganz
Copy link

JohnnyMorganz commented Jul 2, 2022

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
Copy link
Author

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
Copy link
Owner

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

@howmanysmall
Copy link
Owner

howmanysmall commented Jul 3, 2022

The new version will be here.

@howmanysmall
Copy link
Owner

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

@JohnnyMorganz
Copy link
Author

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
Copy link

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

@howmanysmall
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants