Clean up async script loading #3784
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Made async script loading thread not start unless async script loading is enabled. - Assigned a meaningful name to the async script loading thread. - Made async script loading not extend Thread directly but implement Runnable. - Made async script loading task terminate on InterruptedException.
FranKusmiruk
approved these changes
Mar 2, 2021
|
Closing as #3924 has been merged, fixing the same issues |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Made async script loading thread not start unless async script loading is enabled.
This will reduce thread stack and will not include async loader thread in, for example, Spigot Watchdog crash thread dumps,
unless it is enabled.
Assigned a meaningful name to the async script loading thread.
This will make clearer that Skript created and owns the thread in case it had an exception or presented in Spigot Watchdog
thread dumps, or such.
Made async script loading not extend Thread directly but implement Runnable.
Extending Thread class unless overriding methods from Thread class is needed is considered bad practice, and Runnable
should be used instead. This generally only a good practice and the only positive meaningful effect is someone can run the
task directly without the thread if desired.
Made async script loading task terminate on InterruptedException.
This made to not call Skript#exception when reloading Skript config with changing async script loading from true to false. In
that situation this PR interrupts the async loader thread since async loading is disabled and async loading thread is not
needed anymore, and this change makes async loading task respect and handle InterruptedException properly.
Re-added the
asynchronous script loadingoption to the config with a warning.However, this warning may not stop users from enabling this option. If desired, I can open another PR for adding this option
back and separate this PR to only change logic of async loading. This option should not cause any issues with vanilla Skript
though after [bug] Fix async script reloading #3324 is merged and Log handler system isn't thread safe #3453 is fixed. I tested only with example scripts, though. It can still break add-ons and
scripts.
Target Minecraft Versions: any
Requirements: none
Related Issues: #3743 (partly), should probably merge #3324 before, may also want to merge after #3453 is fixed