Re: RFR: 8262027: Improve how HttpConnection detects a closed channel when taking/returning a connection to the pool [v5]
> 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 six additional commits since the last revision: - 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/2649/files - new: https://git.openjdk.java.net/jdk/pull/2649/files/c43abe96..d37e7bdb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2649&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2649&range=03-04 Stats: 2007 lines in 70 files changed: 1442 ins; 278 del; 287 mod Patch: https://git.openjdk.java.net/jdk/pull/2649.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2649/head:pull/2649 PR: https://git.openjdk.java.net/jdk/pull/2649
Re: RFR: 8262027: Improve how HttpConnection detects a closed channel when taking/returning a connection to the pool [v2]
On Fri, 19 Feb 2021 19:34:26 GMT, Michael McMahon wrote: >> There's no guarantee that the proxy will have read all the bytes sent by the >> client - even if it attempts to drain the connection. So the only sane >> reaction if you're not going to parse the request body is to close the >> connection. > > Right, I was mistaken. It actually is related to this change. You are testing > what happens *if* the proxy closes the connection. Though that wouldn't be > normal behavior for a proxy. If you are sending 407 to the client then you > would want to keep the connection open so the client can retry the request. > Maybe we need some comments in ProxyServer to indicate that the connection is > being closed to test this specific scenario. Though if ProxyServer is used in > other tests, I wonder if it might be better to use some flag or switch to > enable this connection closing behavior? I have reworked the ProxyServer to keep reusing the connection if it can detect that there will be no request body. If there is a request body and 407 is returned however, it will close the connection. That should leave things unchanged for tests that might have tried to use the ProxyServer for GET/HEAD requests. I suspect that no test was using it for POST as that obviously could not work if authentication was required. That said, all tests are passing :-) - PR: https://git.openjdk.java.net/jdk/pull/2649
Re: RFR: 8224775: test/jdk/com/sun/jdi/JdwpListenTest.java failed to attach
On Thu, 18 Feb 2021 21:43:00 GMT, Alex Menkov wrote:
> The fix also partially fixes JdwpAttachTest failures (JDK-8253940).
> The failures are caused by network configuration changes during test
> execution ("global" IPv6 addresses disappears from interface).
> To minimize chances of intermittent failures like this java.net tests use
> only link-local addresses whenever possible.
> The fix does the same for JDI tests
> (Utils.getAddressesWithSymbolicAndNumericScopes is used by JdwpListenTest
> and JdwpAttachTest)
I don't see any specific issue with the proposed change but I don't know the
JDWP tests enough to provide more feedback than that. Do you have special test
cases for the IPv6 loopback? AFAIU this code here will filter it out?
-
PR: https://git.openjdk.java.net/jdk/pull/2633
Integrated: 8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl
On Wed, 13 Jan 2021 06:19:18 GMT, Clive Verghese wrote: > Redo for 8237578: JDK-8214339 (SSLSocketImpl wraps SocketException) appears > to not be fully fixed > > This also fixes JDK-8259516: Alerts sent by peer may not be received > correctly during TLS handshake This pull request has now been integrated. Changeset: 63f8fc87 Author:Clive Verghese Committer: Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/63f8fc87 Stats: 199 lines in 7 files changed: 177 ins; 0 del; 22 mod 8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl Reviewed-by: xuelei - PR: https://git.openjdk.java.net/jdk/pull/2057
Re: RFR: 8224775: test/jdk/com/sun/jdi/JdwpListenTest.java failed to attach
On Mon, 22 Feb 2021 17:34:03 GMT, Daniel Fuchs wrote: > > > I don't see any specific issue with the proposed change but I don't know the > JDWP tests enough to provide more feedback than that. Do you have special > test cases for the IPv6 loopback? AFAIU this code here will filter it out? Good catch. IPv6 loopback addresses shouldn't be filtered out. I'll update PR after re-testing - PR: https://git.openjdk.java.net/jdk/pull/2633
Re: RFR: 8224775: test/jdk/com/sun/jdi/JdwpListenTest.java failed to attach [v2]
> The fix also partially fixes JdwpAttachTest failures (JDK-8253940).
> The failures are caused by network configuration changes during test
> execution ("global" IPv6 addresses disappears from interface).
> To minimize chances of intermittent failures like this java.net tests use
> only link-local addresses whenever possible.
> The fix does the same for JDI tests
> (Utils.getAddressesWithSymbolicAndNumericScopes is used by JdwpListenTest
> and JdwpAttachTest)
Alex Menkov has updated the pull request incrementally with one additional
commit since the last revision:
include loopback addresses in testing
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2633/files
- new: https://git.openjdk.java.net/jdk/pull/2633/files/96176a61..4d59abf6
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2633&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2633&range=00-01
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/2633.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2633/head:pull/2633
PR: https://git.openjdk.java.net/jdk/pull/2633
