Prevent Netty connection pool from killing active connections #1114
Conversation
|
Hi @malaporte Thanks a lot! Reviewing atm. One first question: can the test you added actually fail, as a unit test, or is it something that's intended to create some conditions that you have to check, eg with netstat? |
|
It fails if the fix isn't present, for example if you comment out the part that calls |
|
(I should say: it eventually fails after some time running) |
| @@ -257,7 +252,7 @@ private boolean offer0(Channel channel, Object partitionKey, long now) { | |||
| } | |||
|
|
|||
| private void registerChannelCreation(Channel channel, Object partitionKey, long now) { | |||
| if (channelId2Creation.containsKey(partitionKey)) { | |||
| if (partitions.containsKey(partitionKey)) { | |||
There was a problem hiding this comment.
Actually, there's a stupid bug here: the test was supposed to be if (!channelId2Creation.containsKey(partitionKey)). The goal was to avoid creating a new ChannelCreation if the entry is already there. And you don't gain anything here with using computeIfAbsent instead of putIfAbsent because the lambda would be capturing, and that the key usually exists.
Could you please revert your change and fix the if test?
There was a problem hiding this comment.
You mean if (!channelId2Creation.containsKey(channelId(channel))) { right? Aka the key is not the partition key as in your sample? This plus the absence of the ! is what got me confused initially...
There was a problem hiding this comment.
Yeah, right (was looking at old code and got confused).
So yes. And please compute channelId only once.
|
Should the UT be flagged with |
|
Absolutely please (with TestNG, I think it's enabled = false) |
|
Once you're done, please rebase into one single commit. |
…handed out of the pool.
malaporte
force-pushed
the
fix-netty-connection-pool
branch
from
b3db03d to
e654be1
Compare
Prevent Netty connection pool from killing active connections
|
Thanks a lot! |
|
@malaporte Do you want to push the fix on 1.9 too? |
|
If you'd left me some more time I'd have gladly provided that :) Thanks! |
|
Wow and thanks for the fast release, much appreciated. |
|
No pro. This bug has been lurking around for some time, but I didn't get a chance to reproduce it, so I really appreciate your help here! Thanks again! |
This fixes the issue #1113 I reported previously by ensuring that a channel can't be both allocated to a caller and expired by the timer task. It uses a simple atomic boolean to grant "ownership" of the channel to one or another.
While testing I also found another issue: channels that are rejected by the pool are not closed properly by
ChannelManager. This is particularly impactful on the test I wrote to reproduce the initial issue, as it quickly leads to all available sockets to be exhausted.And finally, the code that does check for TTL is broken in 2.0 because the creation time for a channel is never put into the map due to the wrong map being used when checking if the partition exists. I'm pretty sure the change I made is correct although I haven't researched this part yet (my initial fix was made on top of 1.9.33 which doesn't have this issue).
I added a test that can be used to reproduce the initial problem (
TimeToLiveIssue) but now works OK once the fix is made.