use daemon threads to allow shut down of application #748
Conversation
|
The TcpCLientSession change has been successfully tested with Geyser (not pushed to Geyser yet pending further discussion here). TcpSession is not tested yet. |
To exit normally all non-daemon threads of the java process are supposed to have stopped. There are several cases of EventLoopGroup-created thread pools that are not shut down as a part of the server shutdown. These threads are spawned on the first Bedrock player login. The identified cases are: - The listener threads in GeyserServerInitializer - The player threads in GeyserServerInitializer - The EXECUTOR_SERVICE in the SkinProvider This patch depends on the PR GeyserMC/MCProtocolLib#748 submitted to MCProtocolLib to allow the TcpCLientSession to shut down on process exit. This is a very quickly thrown together proof of concept to verify that the execution thread pools left running were what blocked a clean exit with Geyser active. Nullify EventLoopGroup variables after shutdown to allow them to be restarted.
|
This patch should resolve #699, needs testing. |
|
I'm not a heavy user of Java but just out of curious, how does providing a |
|
The second parameter to DefaultThreadFactory indicates daemon threads. shutdownGracefully() has always worked. What did not work before was hooking the shutdown event to do the shutdownGracefully() as that event was never fired due to the lingering threads. Daemon threads do not veto shutdown, hence the Runtime object decides to shut down the process, calling the hooks etc. |
|
Okay, thanks for the explanation! But would marking all worker threads as daemon threads hurt the performance when they do need to work, as daemon thread has lower priority to the best of my knowledge? |
|
I don't think there is any change in priority when a thread is marked as daemon. I have not checked if DefaultThreadFactory does that, but it likely don't. It is a pretty of netty, so knows it needs a reasonable priority. |
|
Oh yes I've verified their priorities by printing them out. They have the same priority as the main thread, which is 5 in my case. Sorry for the confusion. |
|
Do we want to move forward with this direction and close #744? |
|
Apparently they are too busy to review your code... |
|
I wouldn't take it personally – the team does tend to be generally busy and it was the weekend. We really do appreciate what is being done here though <3 Some internal discussion was initiated but it should be wrapped up soon |
|
I have no urgency, so no problem there for me. |
|
Not meant to blame anyone but was suggesting that it's probably going to be a while before the developers have time to handle this. Really appreciate such an open-source project where issues are actively discussed and solved.👍 |
To exit normally all non-daemon threads of the java process are supposed to have stopped. There are several cases of EventLoopGroup-created thread pools that are not shut down as a part of the server shutdown. These threads are spawned on the first Bedrock player login. The identified cases are: - The listener threads in GeyserServerInitializer - The player threads in GeyserServerInitializer - The EXECUTOR_SERVICE in the SkinProvider This patch depends on the PR GeyserMC/MCProtocolLib#748 submitted to MCProtocolLib to allow the TcpCLientSession to shut down on process exit. This is a very quickly thrown together proof of concept to verify that the execution thread pools left running were what blocked a clean exit with Geyser active. Nullify EventLoopGroup variables after shutdown to allow them to be restarted.
To exit normally all non-daemon threads of the java process are supposed to have stopped. There are several cases of EventLoopGroup-created thread pools that are not shut down as a part of the server shutdown. These threads are spawned on the first Bedrock player login. The identified cases are: - The listener threads in GeyserServerInitializer - The player threads in GeyserServerInitializer - The EXECUTOR_SERVICE in the SkinProvider This patch depends on the PR GeyserMC/MCProtocolLib#748 submitted to MCProtocolLib to allow the TcpCLientSession to shut down on process exit. This is a very quickly thrown together proof of concept to verify that the execution thread pools left running were what blocked a clean exit with Geyser active. Nullify EventLoopGroup variables after shutdown to allow them to be restarted.
|
Anything we can assist with here to help get it in shape before 1.20.2? |
To exit normally all non-daemon threads of the java process are supposed to have stopped. There are several cases of EventLoopGroup-created thread pools that are not shut down as a part of the server shutdown. These threads are spawned on the first Bedrock player login. The identified cases are: - The listener threads in GeyserServerInitializer - The player threads in GeyserServerInitializer - The EXECUTOR_SERVICE in the SkinProvider This patch depends on the PR GeyserMC/MCProtocolLib#748 submitted to MCProtocolLib to allow the TcpCLientSession to shut down on process exit. This is a very quickly thrown together proof of concept to verify that the execution thread pools left running were what blocked a clean exit with Geyser active. Nullify EventLoopGroup variables after shutdown to allow them to be restarted.
Can we add a concise comment about the 2nd parameter and about the shutdown process to one of the classes? and then in the other class its probably just fine to refer to the other class for documentation about this. |
Want me to add it, or will you guys take over updating the PR? |
To exit normally all non-daemon threads of the java process are supposed to have stopped. There are several cases of EventLoopGroup-created thread pools that are not shut down as a part of the server shutdown. These threads are spawned on the first Bedrock player login. The identified cases are: - The listener threads in GeyserServerInitializer - The player threads in GeyserServerInitializer - The EXECUTOR_SERVICE in the SkinProvider This patch depends on the PR GeyserMC/MCProtocolLib#748 submitted to MCProtocolLib to allow the TcpCLientSession to shut down on process exit. This is a very quickly thrown together proof of concept to verify that the execution thread pools left running were what blocked a clean exit with Geyser active. Nullify EventLoopGroup variables after shutdown to allow them to be restarted.
To exit normally all non-daemon threads of the java process are supposed to have stopped. TcpClientSession creates an static class member EventLoopGroup which in turn instantiates worker threads. These are changed to daemon threads to not get in the way of the shutdown. This is similar to the default in Netty 5. To ensure work is finishd the Runtime shutdown event is hooked to initiate a time-limited graceful shutdown of the event loops. Normally an application should shut down communication before the process shutdown is inititated.
petersv5
force-pushed
the
daemon_thread_executor
branch
from
35a12e3 to
46595e2
Compare
|
Pushed my proposal for comments. |
This is an alternative to #744.
To exit normally all non-daemon threads of the java process are supposed to have stopped. TcpClientSession creates an static class member EventLoopGroup which in turn instantiates worker threads. These are changed to daemon threads to not get in the way of the shutdown. This is similar to the default in Netty 5.
To ensure work is finishd the Runtime shutdown event is hooked to initiate a time-limited graceful shutdown of the event loops. Normally an application should shut down communication before the process shutdown is inititated.