search

PatchworkMC / patchwork-api

An attempt to reimplement the Minecraft Forge API on Fabric
GNU Lesser General Public License v2.1
282 stars 48 forks source link

Implement a single-threaded emulation of DeferredWorkQueue #104

Closed cittyinthecloud closed 4 years ago

cittyinthecloud commented 4 years ago

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 commented 4 years ago

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.

ZNixian commented 4 years ago

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 commented 4 years ago

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

kitlith commented 4 years ago

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.

TheGlitch76 commented 4 years ago

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

kitlith commented 4 years ago

Looks good to me!