Await h2 connection releases before closing pool #950
Conversation
| @@ -150,6 +164,13 @@ private void releaseParentChannel(Channel parentChannel) { | |||
|
|
|||
| @Override | |||
| public void close() { | |||
| connections.forEach(c -> { | |||
There was a problem hiding this comment.
Should these awaits eventually timeout? Also it may not be a good idea to dispatch the close to the connection pool as it also awaits so that would block a netty thread. I think we can safely move it to the calling thread. What do you think?
There was a problem hiding this comment.
Not sure about having a timeout, but what about switching to a normal await() so it's interruptible? +1 moving the close on to the calling thread.
dagnir
force-pushed
the
transcribe-fix-final
branch
2 times, most recently
from
fae1b0f to
12f8440
Compare
| @@ -77,6 +92,10 @@ | |||
| } | |||
|
|
|||
| private Future<Channel> acquire0(Promise<Channel> promise) { | |||
| if (closed) { | |||
There was a problem hiding this comment.
Future improvement, we can make this a decorator to reuse this functionality for HTTP/1.1
| @@ -124,6 +143,11 @@ private void releaseParentChannel0(Channel parentChannel, MultiplexedChannelReco | |||
| } | |||
|
|
|||
| private void release0(Channel channel, Promise<Void> promise) { | |||
| if (closed) { | |||
There was a problem hiding this comment.
Should we still let them release after close?
There was a problem hiding this comment.
Hmm not sure, I think it's better to error out so the caller can just close the channel if it gets rejected otherwise the other option would be to close it for them here but it seems a little non-obvious
There was a problem hiding this comment.
Chatted offline, should be fine to allow release after closing since in reality this would have already been cleaned up by close.
| } | ||
|
|
||
| private Promise<Void> setClosedFlag() { |
There was a problem hiding this comment.
Is there an advantage to doing it this way versus using an AtomicBoolean or something? Just to let the acquires submitted before close called to get a chance to acquire?
There was a problem hiding this comment.
Yeah, I wanted to make sure acquire and close serialized so it's not possible to close while in the middle of an acquire.
We must wait for the connection release to be completed (it's an asynchronous operation) before closing the pool, otherwise it could be left dangling and lead to a socket leak.
dagnir
force-pushed
the
transcribe-fix-final
branch
from
12f8440 to
44ca0f4
Compare
Tests behavior that is not being enforced. Forgot to remove from aws#950
Tests behavior that is not being enforced. Forgot to remove from #950
…22c5ac02 Pull request: release <- staging/bb1d5935-f586-4cf7-b8ee-5cd522c5ac02
Description
We must wait for the connection release to be completed (it's an asynchronous
operation) before closing the pool, otherwise it could be left dangling and lead
to a socket leak.
Motivation and Context
Fix socket leak.
Testing
mvn clean installScreenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsLicense