Fixed a bug in the netty client, where a future may not always be completed. #1217
Conversation
millems
force-pushed
the
millem/fix-unfinished-futures
branch
2 times, most recently
from
1d9d0a0 to
738bc71
Compare
| public HealthCheckedChannelPool(EventLoopGroup eventLoopGroup, | ||
| NettyConfiguration configuration, | ||
| ChannelPool delegate) { | ||
| this.eventLoopGroup = eventLoopGroup; |
There was a problem hiding this comment.
Any reason to use the whole group instead of a single loop?
There was a problem hiding this comment.
Load balancing? It didn't seem like it would be terrible to try to better balance things across the whole group instead of an individual event loop.
| * Time out the provided acquire future, if it hasn't already been completed. | ||
| */ | ||
| private void timeoutAcquire(Promise<Channel> resultFuture) { | ||
| if (!resultFuture.isDone()) { |
There was a problem hiding this comment.
This check shouldn't be necessary
There was a problem hiding this comment.
Originally, I didn't want the overhead of allocating the IOException (filling in the stack trace is apparently pretty expensive), because I wasn't cancelling the timeout on success. Now it's not really as needed, since I'm cancelling the timeout task, so I don't mind removing it.
| @@ -224,6 +225,10 @@ private ChannelPool createChannelPool(Bootstrap bootstrap, ChannelPipelineInitia | |||
| // from the underlying pool, the channel is closed and released. | |||
| channelPool = new CancellableAcquireChannelPool(bootstrap.config().group().next(), channelPool); | |||
|
|
|||
| // Wrap the channel pool to guarantee all channels checked out are healthy, and all unhealthy channels checked in are | |||
| // closed. | |||
| channelPool = new HealthCheckedChannelPool(bootstrap.config().group(), configuration, channelPool); | |||
There was a problem hiding this comment.
We should switch with CancellableAcquireChannelPool
…pleted. If a service closes a connection between when a channel is acquired and handlers are attached, channel writes could disappear and the response future would never be completed. This change introduces health checks and retries for channel acquisition to fix the majority of cases without failing requests, as well as one last check after handlers are added to ensure the channel hasn't been closed since the channel pool health check. Fixes #1207.
millems
force-pushed
the
millem/fix-unfinished-futures
branch
from
738bc71 to
79801aa
Compare
Codecov Report
@@ Coverage Diff @@
## master #1217 +/- ##
============================================
+ Coverage 59.01% 59.02% +0.01%
- Complexity 4612 4632 +20
============================================
Files 746 747 +1
Lines 23134 23182 +48
Branches 1733 1737 +4
============================================
+ Hits 13653 13684 +31
- Misses 8786 8804 +18
+ Partials 695 694 -1
Continue to review full report at Codecov.
|
…f869903c5 Pull request: release <- staging/88647416-cbd0-4403-8adc-664f869903c5
If a service closes a connection between when a channel is acquired and handlers are attached, channel writes could disappear and the response future would never be completed. This change introduces health checks and retries for channel acquisition to fix the majority of cases without failing requests, as well as one last check after handlers are added to ensure the channel hasn't been closed since the channel pool health check. Fixes #1207.