Awaits EventLoopGroup Shutdown when closing Netty client #1069
Conversation
| } | ||
| return CompletedCloseFuture.COMPLETED_FUTURE; |
There was a problem hiding this comment.
You can use SucceededFuture with a null executor instead of creating your own future implementation.
There was a problem hiding this comment.
Yeah, I considered using that but was too afraid to pass two nulls to the constructor. I wish they have an overloaded constructor. Will update to use that instead.
| * | ||
| * @param promise the promise to complete | ||
| */ | ||
| public static void syncInterruptibly(Future<?> promise) { |
There was a problem hiding this comment.
This isn't that much different than netty's awaitUninterruptibly(). Any reason we care about re-throwing the interrupt exception if we're just going to log it?
There was a problem hiding this comment.
Per Javadoc of awaitUniterrupttibly,
Waits for this future to be completed within the specified time limit without interruption. This method catches an {@link InterruptedException} and discards it silently.
it doesn't re-interrupt the thread and that's why I created this method to re-interrupt the thread.
There was a problem hiding this comment.
This does re-interrupt the thread, if you look at the implementation.
There was a problem hiding this comment.
Ohh yeah, it does. I will update. I was looking at the implementation of an overloaded method of awaitUninterruptibly and from first look, it looks like it doesn't re-interrupt the method.
@Override
public boolean awaitUninterruptibly(long timeout, TimeUnit unit) {
try {
return await0(unit.toNanos(timeout), false);
} catch (InterruptedException e) {
// Should not be raised at all.
throw new InternalError();
}
}
| @@ -212,7 +214,8 @@ private SdkEventLoopGroup nonManagedEventLoopGroup(SdkEventLoopGroup eventLoopGr | |||
| @Override | |||
| public void close() { | |||
| runAndLogError(log, "Unable to close channel pools", pools::close); | |||
| runAndLogError(log, "Unable to shutdown event loop", sdkEventLoopGroup.eventLoopGroup()::shutdownGracefully); | |||
| Future<?> closePromise = sdkEventLoopGroup.eventLoopGroup().shutdownGracefully(); | |||
| runAndLogError(log, "Unable to shutdown event loop", () -> syncInterruptibly(closePromise)); | |||
There was a problem hiding this comment.
Should we do the acquiring of the closePromise within the runAndLogError as well so that close() won't throw any exceptions?
zoewangg
force-pushed
the
zoewang-awaitsEventLoopGroupShutdown
branch
from
b26402a to
6c61b4a
Compare
|
|
|
Okay, this time |
zoewangg
force-pushed
the
zoewang-awaitsEventLoopGroupShutdown
branch
2 times, most recently
from
482ff8a to
598da7a
Compare
Codecov Report
@@ Coverage Diff @@
## master #1069 +/- ##
===========================================
- Coverage 56.2% 56.2% -0.01%
- Complexity 4694 4698 +4
===========================================
Files 824 824
Lines 28027 28039 +12
Branches 2257 2257
===========================================
+ Hits 15753 15759 +6
- Misses 11543 11549 +6
Partials 731 731
Continue to review full report at Codecov.
|
zoewangg
force-pushed
the
zoewang-awaitsEventLoopGroupShutdown
branch
3 times, most recently
from
6235c86 to
b49a0f5
Compare
|
Changing from
to
I compared the source code of |
zoewangg
force-pushed
the
zoewang-awaitsEventLoopGroupShutdown
branch
3 times, most recently
from
5bd5283 to
35b7e61
Compare
|
I updated to use
I think timeout is very necessary as it's extremely difficult to debug which process is the culprit when the application gets stuck. |
| private void closeEventLoopUninterruptibly(EventLoopGroup eventLoopGroup) throws | ||
| TimeoutException, ExecutionException { | ||
| try { | ||
| eventLoopGroup.shutdownGracefully().get(3000, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
shutdownGracefully has a timeout passed to it as well, that by default is longer than 3 seconds. Should we align these values in some way? Can we also catch errors we think can happen sometimes (eg. TimeoutException) to log a less scary error, if the customer doesn't need to worry about them?
There was a problem hiding this comment.
Should we align these values in some way?
+1, it took me a while to understand the javadoc of #shutdownGracefully(long, long, TimeUnit). The default timeout is 15 seconds though and I will update it to more than 15 seconds.
Can we also catch errors we think can happen sometimes (eg. TimeoutException) to log a less scary error, if the customer doesn't need to worry about them?
Hmm, I do think customers need to be aware of the issue though as unclosed eventloop can cause resource leak. What do you think of changing the log level to warn?
There was a problem hiding this comment.
Oh, do you mean just catching TimeoutException and changing the error message? Yeah, we can do that. I'll update it
|
|
||
| private void closeEventLoopUninterruptibly(EventLoopGroup eventLoopGroup) throws ExecutionException { | ||
| try { | ||
| eventLoopGroup.shutdownGracefully().get(EVENTLOOP_SHUTDOWN_TIMEOUT_SECONDS, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Should we pass the same(ish) value to shutdownGracefully, to make it clear why this value was chosen? Worried a future dev might not understand why this particular value is chosen and lessen it.
zoewangg
force-pushed
the
zoewang-awaitsEventLoopGroupShutdown
branch
from
bec2d9b to
d5b79a9
Compare
…6ec1651cd Pull request: release <- staging/8a15f3ae-efe8-4560-9dcf-db46ec1651cd
Description
Awaits
EventLoopGroup#shutdownGracefullyto complete when closing Netty client.Motivation and Context
Fun fact:
EventLoopGroup#shutdownGracefullyis also asynchronous.Testing
unit tests and manually triggered smart integ tests.
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsLicense