SpongePowered / Sponge

The SpongeAPI implementation targeting vanilla Minecraft and 3rd party platforms.
MIT License
384 stars 211 forks source link

PhaseTracker throws error when watchdog kills server #2317

Open LordRalex opened 5 years ago

LordRalex commented 5 years ago

This is a race condition type of error, so it's not consistently going to trigger.

When Watchdog kicks in, this triggers a runtime hook which shuts down the server. This hook runs on a different thread as the main thread may have stalled. Since this isn't the main thread, Phasetracker will throw errors if it tries to handle things.

Example stacktrace https://pastebin.com/raw/XFDd0hbk

MinecraftServer has code that is mixin'ed in to prevent a shutdown that's not on the main thread, however this doesn't seem to be accurate/applying when watchdog is involved. https://github.com/SpongePowered/SpongeCommon/blob/00f5e56506b1bf9f52489a453443c215473a0534/src/main/java/org/spongepowered/common/mixin/core/server/MinecraftServerMixin.java#L457

serverRunning is not set to false when watchdog kills a server, causing the above check to fail. It just simply calls the stopServer method.

JBYoshi commented 5 years ago

From what I can tell, it appears that the stack trace is consistently logged... but the log it uses happens to be disabled at the time. Not sure how that happens.

The CauseStackManager doesn't complain about this because it exempts the server shutdown thread from these checks.

I'm wondering if we should fix this by setting up separate PhaseTracker instances for different threads? There's already a hidden CLIENT instance; we could add an additional one for the shutdown hook.

gabizou commented 5 years ago

The problem is that the server thread PhaseTracker is already populated, so that phase is trying to complete, as it should. Or just ignore the case when the server is shutting down.

sibomots commented 3 years ago

Is this issue being worked? Does it need an assignee ?

Thx