Skip to content
This repository was archived by the owner on Jun 3, 2024. It is now read-only.

Conversation

cittyinthecloud
Copy link
Contributor

DeferredWorkQueue is an API that mods can use to call things on the main loader thread. Since we're single threaded, this version just runs the code can creates completed futures.

@kitlith
Copy link
Contributor

kitlith commented Jun 24, 2020

Two potential issues, depending on how this is usually used.

Nothing is preventing a mod from creating a new thread itself (or use a mixin in code that gets called in another thread), and then trying to use DeferredWorkQueue to schedule things on thr main thread. That would not work as expected with this implementation.

If a mod is actually trying to defer execution (i.e. do a bit of work, then schedule itself again so that other stuff can run) it would also not work as intended.

I'm gonna admit that I don't know how this is used in practice, but I suspect they include these problem cases.

Copy link
Member

@ZNixian ZNixian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@ZNixian
Copy link
Member

ZNixian commented Jun 24, 2020

I can imagine a plausible race condition where mods assume the payload hasn't finished executing. In those cases, if they exist, it would be reasonable to file bug reports against those mods IMO.
(And as a note, please don't reply to this if you think it might lead to bikeshedding)

@cittyinthecloud
Copy link
Contributor Author

@kitlith This is only usable during loading on Forge, so spawning threads is highly unlikely.

@kitlith
Copy link
Contributor

kitlith commented Jun 24, 2020

Okay, then add a comment to the effect of that, explaining that it is only used during loading, (akin to the forge comment) and for bonus points add an explicit error when used outside of loading and LGTM.

Copy link
Contributor

@Rongmario Rongmario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, any tests being done yet?

Copy link
Member

@TheGlitch76 TheGlitch76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to check if this is on the main thread as a safety net? Concurrency errors are a pain in the ass to solve.

@TheGlitch76
Copy link
Member

@kitlith do you still have any issues with this PR?

@kitlith
Copy link
Contributor

kitlith commented Jul 4, 2020

Looks good to me!

@TheGlitch76 TheGlitch76 merged commit ab71e1b into master Jul 4, 2020
@florensie florensie deleted the feature/deferredworkqueue branch July 9, 2020 12:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants