Skip to content
This repository has been archived by the owner on Nov 4, 2024. It is now read-only.

Threading and Exceptions in Commands #106

Open
A248 opened this issue Sep 14, 2020 · 2 comments
Open

Threading and Exceptions in Commands #106

A248 opened this issue Sep 14, 2020 · 2 comments

Comments

@A248
Copy link
Contributor

A248 commented Sep 14, 2020

Two problems I noticed with commands:

  1. All command execution is ran asynchronously, per, ironically, SaneCommand. On the contrary this is not sane at all, for multiple reasons:
  • SaneEconomy's command implementations are not thread safe. player.isOnline() and hasPermission(String), for example, are both not thread safe - the latter can be in certain circumstances, but there is zero guarantee. Violating thread safety can lead to very bad things happening.
  • The minimal performance gain from running a few commands contrast with the costs of thread creation and context switching, making the net performance gain of SaneEconomy's "just run things async" mentality actually negative. Running all commands asynchronously is a performance loss to the main thread as well as the machine as a whole.
  1. Using exceptions as control flow. Though tempting as it is, exceptions should be reserved for truly exceptional behaviour. I've violated this rule myself when I initially thought better, but coming back to the code has always shown me it's wrong. This is less severe than No.1, but it's something you may want to reconsider nonetheless. Far less of a priority.
@AppleDash
Copy link
Owner

1: a) Pull requests welcome, but in this case it's considered not enough of an issue to matter. b) Would it be better to access a potentially unresponsive MySQL server on the main thread and lock up the rest of the server while we wait?
2: My opinion differs and this won't be changed.

@A248
Copy link
Contributor Author

A248 commented Sep 14, 2020

Would it be better to access a potentially unresponsive MySQL server on the main thread and lock up the rest of the server while we wait?

Don't get me wrong, I never said that. Certain operations need to be run asynchronously whereas others do not.

Pull requests welcome

I am planning to work on this when I have more time : )

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants