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

Log handler system isn't thread safe #3453

Closed
TPGamesNL opened this issue Oct 7, 2020 · 0 comments
Closed

Log handler system isn't thread safe #3453

TPGamesNL opened this issue Oct 7, 2020 · 0 comments
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. completed The issue has been fully resolved and the change will be in the next Skript update. priority: lowest "Nice to have" updates that are not required (tiny low impact bug fixes or QoL enhancements).

Comments

@TPGamesNL
Copy link
Member

The current system for log handlers is not thread safe, due to its design. Two relatively long-lastig log handlers from seperate threads could cause problems when their timing intersects. Below is an example to demonstrate this issue:

function useLogHandler():
	set {_a} to "cowcowcowcowcowcowcowcowcowcowcowcow"
	set {_l::*} to {_a} parsed as "%entity type%%entity type%%entity type%%entity type%<.+>"

command /async:
	trigger:
		create section stored in {_section}:
			# Code below will be ran async, it lasts a couple seconds on my machine, giving the user time to reload a script
			broadcast "&eStarted using log handlers"
			loop 10 times:
				useLogHandler()
			broadcast "&eDone using log handlers"
		run section {_section} async

Reproduce by executing the command, then reloading a script file.
Examples of errors which may occur: https://pastebin.com/dfPAb3Sd

While it is unlikely for someone to require parsing a string with the word 'cow' 12 times as entity types on a seperate thread, in unlucky occasions, this problem could crash the server / spam the console (I've seen it happen at least once before, possibly also caused #3291).

I'm not sure how the current system could be improved to avoid this issue, since simply making it no longer based on the list's order will, instead of erroring, cause log entries to be logged to the wrong handler.

@TPGamesNL TPGamesNL added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. priority: lowest "Nice to have" updates that are not required (tiny low impact bug fixes or QoL enhancements). labels Mar 8, 2021
@TPGamesNL TPGamesNL added the PR available Issues which have a yet-to-be merged PR resolving it label Mar 23, 2021
@TPGamesNL TPGamesNL added completed The issue has been fully resolved and the change will be in the next Skript update. and removed PR available Issues which have a yet-to-be merged PR resolving it labels May 18, 2021
@TPGamesNL TPGamesNL mentioned this issue Oct 23, 2021
1 task
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. completed The issue has been fully resolved and the change will be in the next Skript update. priority: lowest "Nice to have" updates that are not required (tiny low impact bug fixes or QoL enhancements).
Projects
None yet
Development

No branches or pull requests

2 participants