Fix plugin loggers on shutdown

[josemmo]

• Added stop signal listener to MinecraftServer
• Fixes Disable plugins when shutting down server using SIGTERM/SIGINT #5270

When system property "terminal.jline" is false and the server receives a SIGINT/SIGTERM plugin, non-root loggers did not work due to how shutdown hooks work.

[Proximyst]

warning: sun.misc.SignalHandler is Sun proprietary API and may be removed in a future release

Dunno if we want to rely on this...

[electronicboy]

that API has been around for a loooong time, I can't really see something getting rid of it on the basis that it's the only way to actually hook into these signals afaik, I can't see this getting stripped out any time soon

[electronicboy]

I'm assuming that you've tested that this doesn't bork anything when ran with/without jline?

[Shevchik]

The attempt to init signal handler should be wrapped in try-catch then, just in case.

[josemmo]

@electronicboy
I'm assuming that you've tested that this doesn't bork anything when ran with/without jline?

Tested on Java 8 with nogui (JLine enabled) and --nojline nogui (JLine disabled), and stopping server using the /stop command and sending CTRL+C keyboard combination.

With JLine enabled the stop signal handler doesn't get called as JLine intercepts the program input.

---

@electronicboy
that API has been around for a loooong time, I can't really see something getting rid of it on the basis that it's the only way to actually hook into these signals afaik, I can't see this getting stripped out any time soon

Just created a simple project with JDK 16 and "sun.misc.*" is still there. It is also available in the internal "jdk.internal.misc.*" package. In the wost case scenario this package can be exposed with --add-exports.

---

@Proximyst
Dunno if we want to rely on this...

If hooking into internal APIs is an issue the other alternative is to hijack the app log manager and override the reset() method, which is the one responsible for loggers not working on shutdown:

System.setProperty("java.util.logging.manager", CustomLogManager.class.getName());

public static class CustomLogManager extends LogManager {
    // [...]
    @Override
    public void reset() {
        // Ignore method call if still shutting down
        // [...]
    }
}

[kashike]

Ref: https://openjdk.java.net/jeps/260

[A248]

warning: sun.misc.SignalHandler is Sun proprietary API and may be removed in a future release

If there's a warning about it, that seems reason enough to avoid it. In JDK 16 there is also Strongly Encapsulate JDK Internals by Default. Now is definitely not the time to regress back to depending on internals.

[josemmo]

Ok, I guess the consensus here is to avoid using signal handlers.
Should I update the PR to fix #5270 by hijacking the log manager then?

[josemmo]

Just pushed a new patch that uses the custom log manager approach instead of signal handlers.

[Proximyst]

Clean! Nice!

[josemmo]

Had to rebase due to a mistype... 🤦‍♂️
Anyway, the custom logger class FQN now is io.papermc.paper.log.CustomLogManager.

[A248]

More of a question than a comment.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[AwesomeDude091]

Could this be looked at again?

Resolved