search

AppleDash / SaneEconomy

Finally, a sane economy plugin for Bukkit.
https://www.spigotmc.org/resources/saneeconomy-simple-but-featureful-economy.26223/
GNU General Public License v3.0
19 stars 24 forks source link

Threading and Exceptions in Commands #106

Open A248 opened 3 years ago

A248 commented 3 years ago

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.

  2. 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 commented 3 years ago

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 commented 3 years ago

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 : )