Cleanup old async commands patch

[SoSeDiK]

Sometime in the past, Spigot added its own async command catcher making Paper's logic fail to execute due to the raised error.

This PR comments out Spigot's async command catcher in favor of Paper's one since Paper's catcher contains more useful data like the link for additional info and the dispatched command, and also allows the command to execute nevertheless. Specifying the command is especially useful in cases where the error shows no traces of the plugin and the command is at least something to look for.

Before: (command not executed)

[14:56:12 ERROR]: Thread Craft Scheduler Thread - 3 - Testik failed main thread check: command dispatch
java.lang.Throwable: null
        at org.spigotmc.AsyncCatcher.catchOp(AsyncCatcher.java:15) ~[paper-1.19.3.jar:git-Paper-413]
        at org.bukkit.craftbukkit.v1_19_R2.CraftServer.dispatchCommand(CraftServer.java:888) ~[paper-1.19.3.jar:git-Paper-413]
        at org.bukkit.craftbukkit.v1_19_R2.entity.CraftPlayer.performCommand(CraftPlayer.java:721) ~[paper-1.19.3.jar:git-Paper-413]
        at me.sosedik.testik.Testik.lambda$onChat$1(Testik.java:33) ~[Testik-1.0-SNAPSHOT.jar:?]
        at org.bukkit.craftbukkit.v1_19_R2.scheduler.CraftTask.run(CraftTask.java:101) ~[paper-1.19.3.jar:git-Paper-413]
        at org.bukkit.craftbukkit.v1_19_R2.scheduler.CraftAsyncTask.run(CraftAsyncTask.java:57) ~[paper-1.19.3.jar:git-Paper-413]
        at com.destroystokyo.paper.ServerSchedulerReportingWrapper.run(ServerSchedulerReportingWrapper.java:22) ~[paper-1.19.3.jar:?]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?]
        at java.lang.Thread.run(Thread.java:833) ~[?:?]
[14:56:12 WARN]: [Testik] Plugin Testik v1.0-SNAPSHOT generated an exception while executing task 6
java.lang.IllegalStateException: Asynchronous command dispatch!
        at org.spigotmc.AsyncCatcher.catchOp(AsyncCatcher.java:16) ~[paper-1.19.3.jar:git-Paper-413]
        at org.bukkit.craftbukkit.v1_19_R2.CraftServer.dispatchCommand(CraftServer.java:888) ~[paper-1.19.3.jar:git-Paper-413]
        at org.bukkit.craftbukkit.v1_19_R2.entity.CraftPlayer.performCommand(CraftPlayer.java:721) ~[paper-1.19.3.jar:git-Paper-413]
        at me.sosedik.testik.Testik.lambda$onChat$1(Testik.java:33) ~[Testik-1.0-SNAPSHOT.jar:?]
        at org.bukkit.craftbukkit.v1_19_R2.scheduler.CraftTask.run(CraftTask.java:101) ~[paper-1.19.3.jar:git-Paper-413]
        at org.bukkit.craftbukkit.v1_19_R2.scheduler.CraftAsyncTask.run(CraftAsyncTask.java:57) ~[paper-1.19.3.jar:git-Paper-413]
        at com.destroystokyo.paper.ServerSchedulerReportingWrapper.run(ServerSchedulerReportingWrapper.java:22) ~[paper-1.19.3.jar:?]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?]
        at java.lang.Thread.run(Thread.java:833) ~[?:?]

After: (command executed)

[14:58:53 ERROR]: Command Dispatched Async: /testik
[14:58:53 ERROR]: Please, notify the author of the plugin causing this execution to fix this bug! See: http://bit.ly/1oSiM6C
java.lang.Throwable: null
        at org.bukkit.craftbukkit.v1_19_R2.CraftServer.dispatchCommand(CraftServer.java:898) ~[paper-1.19.3.jar:git-Paper-"bb63a61"]
        at org.bukkit.craftbukkit.v1_19_R2.entity.CraftPlayer.performCommand(CraftPlayer.java:721) ~[paper-1.19.3.jar:git-Paper-"bb63a61"]
        at me.sosedik.testik.Testik.lambda$onChat$1(Testik.java:33) ~[Testik-1.0-SNAPSHOT.jar:?]
        at org.bukkit.craftbukkit.v1_19_R2.scheduler.CraftTask.run(CraftTask.java:101) ~[paper-1.19.3.jar:git-Paper-"bb63a61"]
        at org.bukkit.craftbukkit.v1_19_R2.scheduler.CraftAsyncTask.run(CraftAsyncTask.java:57) ~[paper-1.19.3.jar:git-Paper-"bb63a61"]
        at com.destroystokyo.paper.ServerSchedulerReportingWrapper.run(ServerSchedulerReportingWrapper.java:22) ~[paper-1.19.3.jar:?]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?]
        at java.lang.Thread.run(Thread.java:833) ~[?:?]

Also tweaked some wording and added a slash before the command to better visualize that it's a command & be on pair with the second catcher (one receives "/command", but the second one receives "command").

[Owen1212055]

I'll be honest, is there any reason why we even need our async catcher? Why not just drop the patch if spigot does it already and our message has been gone for a while anyways.

[SoSeDiK]

Paper has its benefits like:

• logging the executing command (which is a major plus since there might be nothing else useful in the error log)
• passing command into main thread & waiting for it to execute
• and seems like Spigot had missed one spot where the command might be dispatched

Personally, I'd keep the Paper behavior, also considering it's not much of an issue to maintain.

[Owen1212055]

Well, it's mostly that with how long this has been enforced anyways its unlikely many plugins do this nowadays anyways.

[SoSeDiK]

Someone shared with me this monstrosity a few days ago which led me to this PR: https://mclo.gs/l55nmlL I think Paper's handler would've been way more helpful in this case 😅

[electronicboy]

The only reason the paper handler allowed the thing to execute is because that was established behavior which was stupidly common back then, if spigot is now blocking execution, I kinda believe that we should follow

On Wed, 22 Feb 2023, 21:29 SoSeDiK, @.***> wrote:

Someone shared with me this monstrosity a few days ago which led me to this PR: https://mclo.gs/l55nmlL I think Paper's handler would've been way more helpful in this case 😅

— Reply to this email directly, view it on GitHub https://github.com/PaperMC/Paper/pull/8895#issuecomment-1440826688, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJMAZGRK7S7ZJOE7OEAF2DWY2ANDANCNFSM6AAAAAAVEK2DYE . You are receiving this because your review was requested.Message ID: @.***>

[SoSeDiK]

After some consideration I guess these changes can be made:

• Present patch pretty much dropped.
• There's a special handler for #chat which handles illegal chat messages, moves handling messages starting with / from the chat handler straight to the commands handler, and adds a default decorator for chat messages. This should be moved into a separate patch in place of the current one (Improve Player chat API handling).
• Additional patch that simply changes Spigot's async command catcher message into a better one and adds additional catcher in ServerGamePacketListenerImpl#handleCommand to fail as soon as possible and avoid calling PlayerCommandPreprocessEvent event in async (what would be the patch name, suggestions?; or maybe there's already a fitting patch?).

What do you think?

[Owen1212055]

After some consideration I guess these changes can be made:

• Present patch pretty much dropped.
• There's a special handler for #chat which handles illegal chat messages, moves handling messages starting with / from the chat handler straight to the commands handler, and adds a default decorator for chat messages. This should be moved into a separate patch in place of the current one (Improve Player chat API handling).
• Additional patch that simply changes Spigot's async command catcher message into a better one and adds additional catcher in ServerGamePacketListenerImpl#handleCommand to fail as soon as possible and avoid calling PlayerCommandPreprocessEvent event in async (what would be the patch name, suggestions?; or maybe there's already a fitting patch?).

What do you think?

I think this sounds fair.

[SoSeDiK]

Removed the obsolete custom catcher, cleaned up the patch (shuttingDown boolean wasn't used anywhere else), and added a missing catcher & included the executing command in the error log.

I've left out one AT when cleaning up for now:

public net.minecraft.server.network.ServerGamePacketListenerImpl handleCommand(Ljava/lang/String;)V

Since it makes the build fail due to the Timings patch expecting private there, and I'm not sure what's the resolving strategy in this case. (Can be resolved upon request or manually on merging if needed)

[SoSeDiK]

Any chance of getting a quick review? :) Should be a subtle change