-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Re-initialise Netty worker group on plugin restart #289
Re-initialise Netty worker group on plugin restart #289
Conversation
} | ||
|
||
public void enableSSL(SslSimpleBuilder builder) { | ||
sslBuilder = builder; | ||
} | ||
|
||
public Server listen() throws InterruptedException { | ||
workGroup = new NioEventLoopGroup(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@praseodym maybe we should make sure that if workGroup
is != null
before this line, we will shutdown the "old" worker group first so we don't leak any theads here? I think it's not impossible to get to that situation since the reason we get to a reload is effectively always an Exception
in the code that should shut this down right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I've added a non-null check with worker group shutdown and a null check at plugin shutdown. Reloads will mostly be caused by failing to listen, which leaves the worker group in a dead state (which is why reloading never worked), so I presume that the non-null shutdown will be mostly a no-op.
8cfe5bc
to
5363d0d
Compare
5363d0d
to
f082e6d
Compare
f082e6d
to
deb8225
Compare
This allows the plugin to actually recover from exceptions after a restart. It also has the side effect of providing nicer error messages and clearer stack traces to the end user.
deb8225
to
336c2f1
Compare
Rebased. Not sure why one of the Travis build jobs is failing. |
@original-brownbear could you review or merge this PR? |
@praseodym looks good, retriggered Travis and will merge if it goes green. Thanks! |
@robbavey actually now that you're the owner here, can you merge this? :) (don't wanna interfere if I shouldn't :P) |
Rob Bavey merged this into the following branches!
|
@praseodym LGTM - thanks for the contribution, and apologies for the delay. |
@robbavey thanks! |
This allows the plugin to actually recover from exceptions after a
restart. It also has the side effect of providing nicer error messages
and clearer stack traces to the end user.
Closes #268: