NettyResponseFuture never completes because netty provider uses closed channel

[hvesalai]

Using version 1.7.21, a NettyResponseFuture never gets completed because the provider uses a channel that has already been closed and handled by closeChannel(...).

If I have read the source properly, what we have here is a classical race condition:

1. [in client thread] doConnect(...) is called and a cached channel is selected.

   At this point, the attachment connected to the channel is still DiscardEvent

   NettyAsyncHttpProvider.java#L913

   doConnect(...) checks that the connection isOpen(), which it still is.

   NettyAsyncHttpProvider.java#L923

2. [in I/O thread] The channel is closed by the remote end and channelClosed(...) is called. Since the attachment is DiscardEvent, nothing is done.

   NettyAsyncHttpProvider.java#L1341

3. [in client thread] doConnect(...) continues using the now closed channel by attaching a NettyResponseFuture to the channel.

   NettyAsyncHttpProvider.java#L937

4. [in client thread] doConnect(...) calls writeRequest(...) which checks if the channel is closed, which it is.

   NettyAsyncHttpProvider.java#L447

   However, writeRequest(...) does nothing about the fact, relying on channelClosed(...) to handle the situation. But, as channelClosed(...) has already been executed at step 2, the abort(...) or done(...) methods will never get called, resulting in that the listeners of the future will never get notified.

[javateck]

good analysis, in my test, I have thread pool and latch in my stress client side, callback will simply do latch.countDown(), and I constantly see my threads stuck, meaning some callbacks never being called. Wondering how this basic is missed.

[slandelle]

Once again, please share your test case, as you claim to have one where you can easily reproduce.

[hvesalai]

@slandelle, are you asking for a test case to reproduce a race condition? Have you read the analysis, what do you think about it?

[slandelle]

@hvesalai Read briefly, but hadn't dug in yet. But that's typically the kind of stuff that's easier to fix if you have a test case to reproduce. @javateck claims he has a very obvious one, so I'd like to get my hands on it...

[slandelle]

@hvesalai What a mess! Thanks for your analysis. I'm afraid I'll only be able to come up with an ugly fix for AHC 1. Will try to come up with something better for AHC 2.

[slandelle]

I've just pushed an imperfect fix on 1.7.X branch.

Limitations are:

• there's always a time window between the moment a Channel is fetched from the pool and the moment a future is attached so that channelClosed can handle it
• Netty HttpRequest is built again if retry fails and we finally have to go with a new Channel
• it's ugly

Will first port the logic as is on AHC2, but we'll try to come up with something better (maybe use Netty custom events).

[slandelle]

Still not perfect, but chances are much much slower now (just a few simple instructions)