Race condition on 401 introduced in #420

[slandelle]

PR #420 introduced a race condition on channel usage.

The author of this PR claimed that a new socket was open on 401, but this is not the case:

• the callback calls drainChannel: https://github.com/AsyncHttpClient/async-http-client/blob/ahc-1.7.x/src/main/java/com/ning/http/client/providers/netty/NettyAsyncHttpProvider.java#L2084
• when the channel is drained, if keep-alive is used, it's offered to the pool: https://github.com/AsyncHttpClient/async-http-client/blob/ahc-1.7.x/src/main/java/com/ning/http/client/providers/netty/NettyAsyncHttpProvider.java#L1315

So the client doesn't necessarily open a new socket. It only does so if some other request got the connection from the pool before the next request is indeed performed: https://github.com/AsyncHttpClient/async-http-client/blob/ahc-1.7.x/src/main/java/com/ning/http/client/providers/netty/NettyAsyncHttpProvider.java#L1355

This change cause a race condition, where nextRequest tries to reuse the previous channel while it has been offered to the pool and is eventually used by someone else.

[slandelle]

Master not impacted

[slandelle]

Actually, @taer was right about a new Socket being created in the sense that nextRequest is performed immediately, before the original channel is drained and offered to the pull.

@jfarcand WDYT? Does it make sense to change the callback in this case so that it doesn't offer to the pool once drain is done, but perform the next request?

[slandelle]

Actually, we're not opening more sockets that we should as long as connection pooling is allowed, which should generally be the case. Let's leave it like this for now.