Fixed BukkitCommandManager registerCommandImpl to prevent async issue…

[lunar-lunatic]

Changed BukkitCommandManager.java to execute command registration on the main thread only, preventing async command tree issues when reloading, especially on PaperMC

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 5 complexity · 0 duplication

Metric	Results
Complexity	5
Duplication	0

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[magicmq]

I appreciate the focus on user-friendliness, but I have a couple of concerns with this PR:

First, I have made it very clear (or so I think) in PySpigot’s documentation that any interaction with the Bukkit API, and with PySpigot itself, should only occur on the main server thread (synchronously). While I certainly value user-friendliness, I also think it's important to encourage good programming practices, including an understanding of thread safety and multithreaded programming. I would much rather a user make this mistake and learn both why it failed and how to fix it, rather than have PySpigot silently correct it for them.

Could you think of a situation where this might be useful outside of user error (I.E. trying to register a command on an asynchronous thread)?

And second, adding this to the command manager would also imply adding similar handling to all of the other managers, since in theory, none of those are thread-safe either (aside from the task manager). That would be a substantial amount of additional work for a relatively minor issue.

[lunar-lunatic]

First, I have made it very clear (or so I think) in PySpigot’s documentation that any interaction with the Bukkit API, and with PySpigot itself, should only occur on the main server thread (synchronously). While I certainly value user-friendliness, I also think it's important to encourage good programming practices, including an understanding of thread safety and multithreaded programming. I would much rather a user make this mistake and learn both why it failed and how to fix it, rather than have PySpigot silently correct it for them.
Could you think of a situation where this might be useful outside of user error (I.E. trying to register a command on an asynchronous thread)?

I understand your concerns, so let me explain clearly what is happening and why its out of the user's hands:
When Paper's syncCommands() is called, it creates background threads to rebuild the brigadier command tree and push it out. Then when PySpigot calls syncBukkitCommands(), whilst those background threads are still running from a previous sync (f.e. startup/reload/sequences registrations), those threads are iterating the tree while PySpigot goes and mutates it causing the brigadier command tree to become corrupted and commands to behave unexpectedly
The code causing this to happen from Paper is as follows, located in Paper/paper-server/patches/sources/net/minecraft/commands/Commands.java.patch:

COMMAND_SENDING_POOL.execute(() -> this.sendAsync(player, commandNodes));
...
public static final java.util.concurrent.ExecutorService COMMAND_SENDING_POOL = new java.util.concurrent.ThreadPoolExecutor

It even directly states "Paper start - Perf: Async command map building", so it is Paper running in async to the Bukkit.getScheduler().runTaskTimer() that causes the issue, the person developing PySpigot scripts (user) cant prevent this. The current behaviour follows these steps:

• PySpigot registers a command into Bukkit's command map, immediately calling syncBukkitCommands(), which in turns calls Paper's CraftServer.syncCommands()
• Paper's syncCommands() is rebuilding the brigadier command tree, using background threads, pushing out the updated tree
• When a script registers more than one command, the 2nd would start mutating the brigadier command tree while Paper is still running background threads for the 1st command, iterating through it, and this clash causes corruption

The PR would fix this by delaying registrations by one tick, ensuring Paper's background threads finish before the tree is mutated again, it would look like this:

• PySpigot registers a command into Bukkit's command map, immediately calling syncBukkitCommands(), which in turns calls Paper's CraftServer.syncCommands()
• Paper's syncCommands() is rebuilding the brigadier command tree, using background threads, pushing out the updated tree
• PySpigot registers a new command, but it is delayed until the previous syncCommands() threads have had time to finish, preventing simultaneous accessing

And second, adding this to the command manager would also imply adding similar handling to all of the other managers, since in theory, none of those are thread-safe either (aside from the task manager). That would be a substantial amount of additional work for a relatively minor issue.

In my opinion an easily corrupted brigadier command tree poses a noticeable issue to the users, and the fix essentially involves adding a delay to each execution, which should not be too hard to port over.

Please do let me know if you have further concerns or if something I explained above requires further clarification.

[magicmq]

Ahh, I see the issue now! This has actually been a bug I've encountered a few times over the past couple of years, but I was never able to pinpoint the cause. Thank you!

I like your fix, but it assumes that Paper's background thread takes <=1 tick to complete. Suppose Paper's asynchronous task to rebuild the command map takes more than one tick, then I think we would have the same issue: another command would be registered when the command map is still being rebuilt, since the delay wouldn't be long enough. Honestly, I don't know how long it takes to rebuild the command map, so maybe this isn't an issue.

Back when I first wrote this code I was testing with Spigot only, which, as far as I know, rebuilds the command map synchronously, so it wasn't an issue at the beginning. Maybe Paper has another thread-safe way to register commands? I will need to look into it.

[virustotalop]

I believe this pr is related to the issue: PaperMC/Paper#13626 and seems to be a long standing bug with paper if it is the same issue that I'm referring to. Also for some context brigadier command map building should be very fast. Whenever a player switches worlds the command map is rebuilt and then sent, due to this I worked on a patch awhile ago to solve a performance bottleneck we encountered the patch can be found here: Mojang/brigadier@242de3f