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

fix: allow game to quit without waiting for registry updater thread #3953

Merged
merged 1 commit into from
Aug 10, 2020

Conversation

keturn
Copy link
Member

@keturn keturn commented May 12, 2020

This fixes #3940. The thread that was holding things up there was this one that's updating the mod registry.

How to test

  • Start Terasology. Push the Exit Terasology button quickly, before everything has time to settle.
  • Make sure the window not only closed, but the process has quit as well. i.e. you see "process finished with exit code 0" in intellij or "build successful" and a return to the command line from the gradle task.

Outstanding before merging

  • use a more convenient and readable thread runner

…exiting

set it to daemon-mode and give it a name for easier troubleshooting.
@keturn
Copy link
Member Author

keturn commented May 12, 2020

The initial version of this I've provided here is deliberately verbose.

Not that I want creating threads to be easy, but I certainly don't want the right way to be this much more difficult than the wrong way.

The three qualities we've got here are:

  1. takes a Callable, returns a Future
  2. is a daemon thread
  3. has a name that tells us something useful about what it's doing or where it was spawned

and I threw in the abbreviator for good measure, but

org.terasology.rendering.nui.layers.mainMenu.advancedGameSetupScreen.AdvancedGameSetupScreen#initialise

is a really long string.

@keturn
Copy link
Member Author

keturn commented May 12, 2020

Side note: Is it surprising that this is running as soon as the MainMenuScreen is initialized? The user may never visit the AdvancedGameSetupScreen.

@keturn
Copy link
Member Author

keturn commented May 12, 2020

oh, that distracted me from what I was saying about my deliberately-verbose implementation. That's a straw man, posted here in the hope of generating discussion that will point toward an existing utility, or some agreement about where to add this as a new utility function.

@keturn keturn added Type: Bug Issues reporting and PRs fixing problems Status: Needs Investigation Requires to be debugged or checked for feasibility, etc. labels May 19, 2020
.submit(moduleManager.getInstallManager().updateRemoteRegistry());
remoteModuleRegistryUpdater = Executors.newSingleThreadExecutor(
new ThreadFactoryBuilder()
.setNameFormat(new TargetLengthBasedClassNameAbbreviator(36).abbreviate(getClass().getName()) + "-%d")
Copy link
Contributor

Choose a reason for hiding this comment

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

"ModuleRegistryUpdater" sounds better and more ralate then "AdvancedGameSetupScreen"

@pollend
Copy link
Member

pollend commented Jul 14, 2020

Just tested this with and without the PR and the game exits correctly when closing.

@jdrueckert jdrueckert changed the title allow game to quit without waiting for registry updater thread fix: allow game to quit without waiting for registry updater thread Aug 10, 2020
@jdrueckert jdrueckert merged commit 25b81f4 into MovingBlocks:develop Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Investigation Requires to be debugged or checked for feasibility, etc. Type: Bug Issues reporting and PRs fixing problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

process does not quit when game closes, some threads still live
4 participants