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

Make log handler system thread safe #3800

Closed

Conversation

TPGamesNL
Copy link
Member

@TPGamesNL TPGamesNL commented Mar 8, 2021

Description

This change gives each thread its own HandlerList, making it so that two log systems on separate threads won't interfere with each other.

I see two ways this can break:

  1. A log system started on one thread is stopped on another.

    • I don't think this happens anywhere in vanilla Skript and I don't see why an addon would do this, but I can't be sure.
  2. A log system is started on one thread, any relies on log entries from another thread.

    • This doesn't happen often in Skript, there's only one instance I can think of: async script loading.
      • What should happen with async loading: Skript should create a RedirectingLogHandler when the reload command is ran, and stop this log handler when the loading is done.
      • What actually happens with async loading: Because the log handler is started and stopped in the SkriptCommand class, the log handler is often stopped before the loading is done, causing the error count to be wrong, and the log to be redirected to console, instead of the command executor.
      • What happens with async loading after this change: Since the log handler is created on a different thread, none of the errors will be redirected to the command executor, and the message will always say it's successful.
      • See Status of async script loading #3743 for more about this

Target Minecraft Versions: any
Requirements: none
Related Issues: #3453 #1002

@TPGamesNL TPGamesNL added 2.6 bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. labels Mar 8, 2021
@TheDGOfficial
Copy link
Contributor

TheDGOfficial commented Mar 8, 2021

Any reason why not to use java.lang.ThreadLocal?

What happens with async loading after this change: Since the log handler is created on a different thread, none of the errors will be redirected to the command executor, and the message will always say it's successful.

Should it be fixed on my PR then (or another PR), or this one?

@TPGamesNL
Copy link
Member Author

TPGamesNL commented Mar 8, 2021

AFAIK ThreadLocal is for a variable for one specific thread, not for a separate variable per thread.
The issue with the RedirectingLogHandler with reload commands should be fixed in another PR (could be yours), since it's already broken without this PR, this PR just makes the behaviour more predictable.

EDIT: nvm, just found out that ThreadLocal isn't what I thought it was

@TPGamesNL
Copy link
Member Author

If you want to test the Skript from #3453, use this version of skript-reflect: skript-reflect-2.2.2-dev.jar.zip

@TPGamesNL
Copy link
Member Author

TPGamesNL commented Mar 14, 2021

I've also removed the ParserInstance class, since it wasn't used for anything (it was supposed to be used for logging but never implemented)
nvm that, reverted the commit

TPGamesNL added a commit to SkriptLang/skript-reflect that referenced this pull request Mar 17, 2021
@TPGamesNL
Copy link
Member Author

Closing as #3924 has been merged, fixing the same issues

@TPGamesNL TPGamesNL closed this May 18, 2021
@TPGamesNL TPGamesNL deleted the fix/log-handler-thread-safety branch May 18, 2021 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants