Skip to content

Conversation

@glowredman
Copy link
Member

My first steps into multithreading... I hope this is the correct way to do it. The idea is to spawn a new thread which calls Config.init(). As long as that method doesn't return, the entire Config class is locked and other threads can't access its fields and methods (which are all static).

@glowredman glowredman requested review from a team and Alexdoru August 10, 2025 19:38
Copy link
Member

@Alexdoru Alexdoru left a comment

Choose a reason for hiding this comment

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

I don't think it's the best thing to do to put the entire config parsing and file reading on a separate thread.

I think it would be better to only queue the request on a separate thread and have it load your data in a local variable. And then you can queue a task on the main thread using

// this code here should be on your separate thread
final Map<> localVar= whatever you are doing
Minecraft.getMinecraft().func_152343_a(() -> {
                // the code here will run on the main thread,
                // in the runTick loop (so only when the game have fully booted)
                // you can use the final localVar 
                return null;
            });

@glowredman
Copy link
Member Author

I don't think it's the best thing to do to put the entire config parsing and file reading on a separate thread.

Why do you think so? Putting not just the request in the thread would take the (very small) load from reading and parsing off of the main thread. Using the task scheduler makes this code much more complicated because

  • I can no longer easily lock the entire class
  • the request code is inside the if (useURL) {} block and its result is needed inside and (indirectly) outside of that block

@Dream-Master Dream-Master requested review from a team and Alexdoru August 25, 2025 17:26
@Dream-Master Dream-Master added the 🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta label Oct 1, 2025
@boubou19
Copy link
Member

@Alexdoru @glowredman any news here?

@glowredman
Copy link
Member Author

@boubou19 I'm waiting for @Alexdoru 's reply

@Alexdoru
Copy link
Member

I already responded, the correct way to do multithreading is to put only the request on a separate thread and then queue a task on the main thread with the results of the request

@glowredman
Copy link
Member Author

Did you respond on Discord? I can't remember you saying anything specifically about this:

  • I can no longer easily lock the entire class
  • the request code is inside the if (useURL) {} block and its result is needed inside and (indirectly) outside of that block

@Alexdoru
Copy link
Member

  • I can no longer easily lock the entire class

you shouldn't lock the entire class, you need to make just the request is a separate thread not the whole thing

  • the request code is inside the if (useURL) {} block and its result is needed inside and (indirectly) outside of that block

just split the method in two and queue a task on the main thread once the request returns

adding two synchronize keyword and spawning a new thread is a lazy solution to multithreading your code, if you don't want to do it properly then just close the pull request. Also your code isn't even thread safe because of the public fields that are accessed from both the main thread and the other thread

@glowredman
Copy link
Member Author

@Alexdoru done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants