expose the shutdown method of the event loop group #744
Conversation
|
This will be accompanied by a PR to Geyser using the exposed interface. An anternative would be to insted supply the Note that the existing code shutting down the EventLoopGroup in a |
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: - TcpClientSocket created by GeyserSession - The listener threads in GeyserServerInitializer - The player threads in GeyserServerInitializer - The EXECUTOR_SERVICE in the SkinProvider This patch depends on the PR GeyserMC/MCProtocolLib#744 submitted to MCProtocolLib. 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.
src/main/java/com/github/steveice10/packetlib/tcp/TcpClientSession.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I feel like the method name could be more descriptive considering the EventLoopGroup is being shutdown. Also, should the three static fields relating to the EventLoopGroup be nullified in order to keep state consistent if someone tries to resume?
Also, it would probably be best to not catch the exception here?
|
I would suggest that someone that really understands the innards of Geyser rework this from scratch. This was very much a "poke this thing that seems broken" style of patch. The EventLoopGroup variables should be nullified if we (you) decide to go the route in this patch. Some of them were final and again - quick hack to verify that the worker pools were the culprits. |
|
I guess if we null out the event loop group variables for the event loop groups that we have shut down then they will be recreated later if used again? |
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 need to be stopped as a part of shutting down the server. This patch exposes the shutdown of the EventLoopGroup as a static function intended to be called by some central function as a part of server shutdwon. Nullify the EventLoopGroup in preparation for starting new users of TcpClientSession.
petersv5
force-pushed
the
expose_eventloopgroup_shutdown
branch
from
0f416c1 to
98a66e8
Compare
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: - TcpClientSocket created by GeyserSession - The listener threads in GeyserServerInitializer - The player threads in GeyserServerInitializer - The EXECUTOR_SERVICE in the SkinProvider This patch depends on the PR GeyserMC/MCProtocolLib#744 submitted to MCProtocolLib. 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.
|
I pushed a new version that allows the TcpClientSessino to be restarted after a shutdown. This is an enabled for the corresponding update to Geyser to allow /geyser reload to work. I also changed the name of the shutdown function to shutdownEventLoopGroup. |
|
As mentioned in issue #699 this patch is only sufficient as long as the public static variable TcpSession.USE_EVENT_LOOP_FOR_PACKETS is set to false before instantiating the first TcpClientSession. This is the case with Geyser. Otherwise the inheritance from TcpSession will create another EventLoopGroup during object construction that is unused by TcpClientSession. TL:DR the current PR is sufficient for Geyser, but the relationship with TcpSession should be considered for other used of ProtocolLib. |
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: - TcpClientSocket created by GeyserSession - The listener threads in GeyserServerInitializer - The player threads in GeyserServerInitializer - The EXECUTOR_SERVICE in the SkinProvider This patch depends on the PR GeyserMC/MCProtocolLib#744 submitted to MCProtocolLib. 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: - TcpClientSocket created by GeyserSession - The listener threads in GeyserServerInitializer - The player threads in GeyserServerInitializer - The EXECUTOR_SERVICE in the SkinProvider This patch depends on the PR GeyserMC/MCProtocolLib#744 submitted to MCProtocolLib. 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.
|
Closing in favor of #748. |
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 need to be stopped as a part of shutting down the server.
This patch exposes the shutdown of the EventLoopGroup as a static function intended to be called by some central function as a part of server shutdwon.