Paper: Server Crashing Unexpectedly

[doitliketyler]

Description of issue:

Hello. So my server is crashing and I can't figure out why. I'll go to sleep at night and in the morning get on the network and the server is down. Thoughts?

bukkit.yml, spigot.yml, paper.yml, server.properties

Bukkit - https://pastebin.com/raw/aYX1DMvp
Spigot - https://pastebin.com/raw/UktC9d49
Paper - https://pastebin.com/raw/fhjxXny7
Server - https://pastebin.com/raw/i46EhVin

Crash, Stacktrace
https://pastebin.com/raw/FxJiLj3S

Plugin list:

https://pastebin.com/raw/vuqee5N3

Paper build number:

Paper version git-Paper-1322 (MC: 1.12.2) (Implementing API version 1.12.2-R0.1-SNAPSHOT)

[electronicboy]

Looks like denizen executing commands async managed to cause a deadlock, general fix is to don't, I'd suggest speaking to the plugin author.

[aikar]

I'm not sure how denizens even managed to do that. Something turned the async catcher off, but im not sure how as I don't see code for that in Denizens code.

But Denizens does need to properly detect that they are about to execute a command from Async Chat, and dispatch it to main instead so it doesn't rely on the server doing it, which spams consoles with stack traces, which leads confused server owners PM'ing me instead of plugin authors about it.

@mcmonkey4eva

[mcmonkey4eva]

Denizen, no "s".
Denizen code does turn the async catcher off, which Paper should respect but very clearly does not based on that stacktrace. ([Async Chat Thread - #0/ERROR]: stopTiming called async for Command: minecraft:tellraw)
The error looks like it might be the fault of Luckperms, CMI, or just Paper itself.
The main thread is frozen on CreatureSpawner, which might be directly connected to the player handling of Luckperms or the com.Zrips.CMI.commands.list.spawn.perform(spawn.java:52) command executed on a chat thread by a user-created logic pathway (as in, written by a server admin and then executed by Denizen), which if it is the fault, is either itself (CMI) corrupt, or damaged by some Paper logic trying to dance between threads in a way that it shouldn't be doing.

The server reporting this issue, like many servers, configured Denizen to be ran in a way that causes Paper to generate false-positive error messages in certain situations.
The suggested solution is nonsense.
We cannot dispatch to main while main is temporarily frozen to parse a thread-unsafe script.

We offer two routes to handling chat thread issues with non threadable scripts: use the async chat but in a fully safe way (via locking against the main thread), or use the deprecated-in-Bukkit sync chat.
We also have secondary systems that use similar setups as a safety backups and the like (EG using semi-async logic to reduce damage done if certain user logic ends up looping infinitely/locking up in an unexpected way).

The only error related to Denizen functioning here that I can see is Paper refusing to accept the AsyncCatcher setting, and instead reporting false errors, and possibly trying to "correct" the error and in the process making it worse.
Our default advice following past requests to correct this we made here is that users stay away from Paper in favor of using Spigot, as Spigot properly follows configuration settings.
We have to say this every time Paper spams false error messages which leads to confused server owners asking us for help instead of telling Paper devs about it.

[aikar]

@mcmonkey4eva the async catcher has nothing to do with that line - code is being hit that can not ever be ran async, so we print a warning. Best case we could do there is not print the warning if async catcher is off
but that warning actually is not related to the deadlock, that is just warning that an unexpected state had occurred , which gave leaning into it being denizen as the source of the issue.

Everything here boils down to commands being executed async, which this crash would of happened the same way on spigot too.

1. Denizens receives a message, acts on it and dispatches a command async
2. CMI plugin listens to PlayerCommandPreprocessEvent
   
   
   
   • THIS IS IMPORTANT TO NOTE: This command is NEVER expected to fire Async. The constructor ALWAYS sets async to false
   
   
3. SimpleCommandManager is now locked during the execution of this event due to the fact that async=false
   
   
   
   • Async must be true if an event is ever fired off of the main thread, to avoid synchronizing on the manager.
   
   
4. CMI triggers a message to be sent to the player on this same async thread
5. Denizen intercepts this message https://github.com/mcmonkey4eva/Denizen/blob/master/plugin/src/main/java/net/aufdemrand/denizen/utilities/packets/DenizenPacketHandler.java#L78
6. Denizen dispatches this to main thread for processing, and blocks
7. Deadlock - SimpleCommandManager can never unlock, causing the main thread to then be blocked as it too tries to acquire lock on SCM.

You should be able to simply schedule a sync task here: https://github.com/DenizenScript/Denizen-Core/blob/master/src/main/java/net/aufdemrand/denizencore/scripts/commands/CommandExecuter.java#L252

Then the command will simply execute the next loop around the tick loop.

You suspending the main thread here (however you are doing that) doesn't do anything to solve this deadlock risk as you are simply causing code to execute async that is synchronizing, and synchronize has no concept of AsyncCatcher either.

[aikar]

from what I can see here, is that Denizen has this risk any time an async chat event triggers a command that sends feedback to the player inside of command preprocess event.

I guess on papers side I could force denizens to run it sync by removing the async catcher check there, which should fix the crash

[aikar]

@doitliketyler try Paper build 1329. It should fix the crash.

It's still going to generate async warnings in logs though.

[doitliketyler]

Thanks @aikar

[aikar]

@doitliketyler please let me know if it resolves your crashing issues.

[mcmonkey4eva]

... Huh.
Somewhere along the way some important async handling code disappeared and also Morphan added code that thoroughly ignores our own async handling policies.
Well, that's annoying.
Either way the fix is simple for @doitliketyler , just disable the config option in Denizen that caused it to be async in the first place.
Change this:

      On player chats:
        # Whether to use the dangerous 'async' chat event (not recommended!)
Use asynchronous event: true

to "false"

[aikar]

@mcmonkey4eva if your suspending main, then you gain no benefit from even trying to keep it on the async thread.

I suggest improving denizen so that anytime the async chat event 'reacts', to dispatch its work as a sync task instead, so nothing ever processes on that thread.

That removes the need to even have this config option. Use the event to asynchronously identify if you need to do something, and when you identify you do need to do something, schedule that work to be done next tick.

[mcmonkey4eva]

That's not viable. As the simplest example of why: You can't cancel an event that's been processed and discarded already. For the chat handler to function, it must process and choose to cancel or not while the secondary thread waits.
If a server admin / user of the plugin wants to leave async on (for some reason, not sure why they'd want that) and be able to cancel or similar and also wants to avoid the risks of async caused by server-modifying instructions, it is up to them to configure their scripts that way. (IE, they can very easily configure to cancel or not and then have further processing delayed onto the next main thread tick.)

Our original code had a choice between using the old chat sync system available in Bukkit, or suspending main to process instructions (in a non-deadlocking manner) and be fine.
For whatever reason, the main suspending logic just went missing in some long ago code revision, and some packet control code added by Morphan seems to have been written without regard for threading risks.
Either the main thread suspender went off somewhere other than where it's meant to be but is still in effect, or it's gone and I'm not sure how a deadlock could arise from purely Denizen code. (If it got suspended by some other code or another plugin, it'd make sense that the awkward packet logic would induce a deadlock. That section definitely needs some revision effort.)

[aikar]

@mcmonkey4eva it sounds like you have no reason to even provide async chat event as an option.

Just remove the event, and always use the sync one.

it's not end of the world to use the sync event, its just better if you CAN get by with only async chat events.

but with how denizen is operating, it undoes all optimization of keeping chat async, and really complicates the flow.