Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8262027: Improve how HttpConnection detects a closed channel when taking/returning a connection to the pool #2649

Closed
wants to merge 8 commits into from

Conversation

@dfuch
Copy link
Member

@dfuch dfuch commented Feb 19, 2021

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8262027: Improve how HttpConnection detects a closed channel when taking/returning a connection to the pool

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2649/head:pull/2649
$ git checkout pull/2649

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 19, 2021

👋 Welcome back dfuchs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Feb 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 19, 2021

@dfuch The following label will be automatically applied to this pull request:

  • net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the net label Feb 19, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 19, 2021

@openjdk openjdk bot removed the rfr label Feb 19, 2021
}
clientSocket.shutdownInput();
close();
return;
Copy link
Member

@Michael-Mc-Mahon Michael-Mc-Mahon Feb 19, 2021

I realise it isn't related to this change, but why is the test proxy closing the connection?

Copy link
Member Author

@dfuch dfuch Feb 19, 2021

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.

Copy link
Member

@Michael-Mc-Mahon Michael-Mc-Mahon Feb 19, 2021

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?

Copy link
Member Author

@dfuch dfuch Feb 22, 2021

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 :-)

Copy link
Member

@Michael-Mc-Mahon Michael-Mc-Mahon Feb 23, 2021

Thanks. That looks fine now.

test/jdk/java/net/httpclient/ProxyServer.java Show resolved Hide resolved
@openjdk openjdk bot added the rfr label Feb 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 23, 2021

@dfuch This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8262027: Improve how HttpConnection detects a closed channel when taking/returning a connection to the pool

Reviewed-by: chegar, michaelm

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 10 new commits pushed to the master branch:

  • fac37bf: 8262269: javadoc test TestGeneratedClasses.java fails on Windows
  • 3e13b66: 8262157: LingeredApp.startAppExactJvmOpts does not print app output when launching fails
  • c769388: 8262266: JDK-8262049 fails validate-source
  • 03e781b: 8262265: ProblemList jdk/javadoc/doclet/testGeneratedClasses/TestGeneratedClasses.java on Windows
  • c6eae06: 8262049: [TESTBUG] Fix TestReferenceRefersTo.java for Shenandoah IU mode
  • e5304b3: 8253409: Double-rounding possibility in float fma
  • 3132b1c: 8261665: Clean up naming of StringContent and FixedStringContent
  • c30a90b: 8261976: Normalize id's used by the standard doclet
  • 53b1545: 8223355: Redundant output by javadoc
  • d2b9c22: 8262011: [JVMCI] allow printing to tty from unattached libgraal thread

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Feb 23, 2021
@dfuch
Copy link
Member Author

@dfuch dfuch commented Feb 23, 2021

Thanks Chris! Done.

Copy link
Member

@Michael-Mc-Mahon Michael-Mc-Mahon left a comment

LGTM

@dfuch
Copy link
Member Author

@dfuch dfuch commented Feb 24, 2021

/integrate

@openjdk openjdk bot closed this Feb 24, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Feb 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 24, 2021

@dfuch Since your change was applied there have been 11 commits pushed to the master branch:

  • 382e38d: 8256438: AArch64: Implement match rules with ROR shift register value
  • fac37bf: 8262269: javadoc test TestGeneratedClasses.java fails on Windows
  • 3e13b66: 8262157: LingeredApp.startAppExactJvmOpts does not print app output when launching fails
  • c769388: 8262266: JDK-8262049 fails validate-source
  • 03e781b: 8262265: ProblemList jdk/javadoc/doclet/testGeneratedClasses/TestGeneratedClasses.java on Windows
  • c6eae06: 8262049: [TESTBUG] Fix TestReferenceRefersTo.java for Shenandoah IU mode
  • e5304b3: 8253409: Double-rounding possibility in float fma
  • 3132b1c: 8261665: Clean up naming of StringContent and FixedStringContent
  • c30a90b: 8261976: Normalize id's used by the standard doclet
  • 53b1545: 8223355: Redundant output by javadoc
  • ... and 1 more: https://git.openjdk.java.net/jdk/compare/0257caad38b4358bd151e993b708603fce2056f2...master

Your commit was automatically rebased without conflicts.

Pushed as commit 0d2dbd2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants