On Tue, 23 Feb 2021 17:20:13 GMT, Daniel Fuchs <[email protected]> wrote:
>> Hi,
>>
>> Please find here a fix for:
>> 8262027: Improve how HttpConnection detects a closed channel when
>> taking/returning a connection to the pool
>>
>> While writing a new test to verify that it was possible to handle proxy
>> *and* server authentication manually when both proxy and server required
>> authentication, I stumbled on a race condition where the next request after
>> receiving 407 would manage to retrieve the connection from the pool just
>> before the connection close was receive from the proxy. Since the test was
>> using POST, and POST are not retried by default, this caused the test to
>> fail randomly and intermittently.
>>
>> This fix proposes to add a checkOpen() method to HttpConnection, which we
>> can call just after retrieving a connection from the pool. This method will
>> attempt to read 1 byte from the channel. Because the connection was in the
>> pool, it should not have received anything, and because the channel is
>> non-blocking, the `read` should always return 0, unless the channel has been
>> closed. This forces an early check on the channel state, rather then waiting
>> for the selector to wake up the Selector Manager Thread - which might happen
>> too late.
>>
>> This is not a 100% silver bullet, but it drastically reduced the number of
>> failures I was observing (to 0 after several 100 loops of testing on all
>> machines). The only other failures I observed was on windows, where
>> apparently closing the socket on the server side can cause a reset, even
>> when SO_LINGER and TCP_NODELAY are specified. I solved that by adding a
>> small delay between socket.shutdownOutput() and socket.close() in the test
>> proxy - when running on windows.
>
> Daniel Fuchs has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains eight additional
> commits since the last revision:
>
> - Incorporoted review comments
> - Merge branch 'master' into proxy-server-auth-8262027
> - Reworked ProxyServer to reuse the connection in case it can detect that
> there will be no request body
> - Merge branch 'master' into proxy-server-auth-8262027
> - Added blank line
> - Fixed trailing white spaces
> - Remove commented code in test
> - 8262027: Improve how HttpConnection detects a closed channel when
> taking/returning a connection to the pool
if (!secure) {
c = pool.getConnection(false, addr, proxy);
if (c != null && c.checkOpen() /* may have been eof/closed when in the pool
*/) {
...
} else {
...
}
} else { // secure
if (version != HTTP_2) { // only HTTP/1.1 connections are in the pool
c = pool.getConnection(true, addr, proxy);
}
if (c != null && c.isOpen()) { <-------------------------------- Why is
this still isOpen()?
...
I wonder why the additional check wasn't added to the HTTPS branch. Can someone
enlighten me?
-------------
PR Comment: https://git.openjdk.org/jdk/pull/2649#issuecomment-2216706000
