8299018: java/net/httpclient/HttpsTunnelAuthTest.java fails with java.io.IOException: HTTP/1.1 header parser received no bytes #11717

dfuch · 2022-12-19T10:24:39Z

Please find here a trivial fix for a test issue in the HttpClient test's ProxyServer.

The ProxyServer has a bad printf that throws an IllegalFormatConversionException, which causes the proxy to close the connection after sending a 407 request. Depending on timing, the client might attempt to reuse the closed (or soon to be closed) connection for sending authentication, which then gets the next POST request to fail.

This failure is rare and intermittent (the client usually gets the close notification before it reuses the connection), but sometimes it doesn't.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8299018: java/net/httpclient/HttpsTunnelAuthTest.java fails with java.io.IOException: HTTP/1.1 header parser received no bytes

Reviewers

Daniel Jeliński (@djelinski - Committer) ⚠️ Review applies to 3d4db2b6
Jaikiran Pai (@jaikiran - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11717/head:pull/11717
$ git checkout pull/11717

Update a local copy of the PR:
$ git checkout pull/11717
$ git pull https://git.openjdk.org/jdk pull/11717/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11717

View PR using the GUI difftool:
$ git pr show -t 11717

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11717.diff

bridgekeeper · 2022-12-19T10:26:15Z

👋 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 · 2022-12-19T10:28:41Z

@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.

mlbridge · 2022-12-19T10:32:43Z

Webrevs

test/jdk/java/net/httpclient/ProxyServer.java

Co-authored-by: Daniel Jelinski <djelinski1@gmail.com>

dfuch · 2022-12-19T10:36:59Z

Good catch. Thanks @djelinski

jaikiran · 2022-12-19T10:42:48Z

test/jdk/java/net/httpclient/ProxyServer.java

@@ -458,11 +458,14 @@ private boolean shouldCloseAfter407(List<String> headers) throws IOException {
            try {
                n = Integer.parseInt(cl);


Hello Daniel, is it expected that the content-length header will always be present and if it isn't then the ProxyServer is expected to close the connection?
In its current form (even without the changes in this PR), if the content-length header is missing, this block will run into a NumberFormatException leading to the connection closure. Is that OK?

The idea is that the connection can be reused only if the ProxyServer knows how many bytes it should read from the connection to get to the next request. Otherwise the connection will be closed. So if using chunked encoding or if no content-length is given the ProxyServer will simply close the connection. Maybe a smarter implementation could deal with chunked encoding and also look at the request method and accept that no content length means 0 in case of HEAD or GET - but that's not part of this fix.

Actually - maybe I should go on and make the changes wrt GET and HEAD. The ProxyServer seems to have some code for this but it looks buggy.

djelinski · 2022-12-19T10:51:28Z

Could you also modify the ProxyServer to send a Connection: close header when shouldCloseAfter407 returns true?

dfuch · 2022-12-19T12:21:42Z

Could you also modify the ProxyServer to send a Connection: close header when shouldCloseAfter407 returns true?

Good idea.

…AD and GET.

djelinski

LGTM.

djelinski · 2022-12-19T15:54:32Z

test/jdk/java/net/httpclient/ProxyServer.java

                }
-                return true;  // should close
            }
            if (n > 0 || n < -1) {


this part can be removed; we don't need any special handling for content-length: -1

djelinski · 2022-12-19T15:54:50Z

test/jdk/java/net/httpclient/ProxyServer.java

+            var cmdline = headers.get(0);
+            int m = cmdline.indexOf(' ');
+            var method = (m > 0) ? cmdline.substring(0, m) : null;
+            var nobody = List.of("GET", "HEAD");


I think we can include CONNECT in this list

Should we leave a failed CONNECT connection open? I'm not sure. CONNECT are handled separately on the client side too.

jaikiran

Looks fine to me.

openjdk · 2022-12-20T10:07:20Z

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

8299018: java/net/httpclient/HttpsTunnelAuthTest.java fails with java.io.IOException: HTTP/1.1 header parser received no bytes

Reviewed-by: djelinski, jpai

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:

dd15d30: 8299043: test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java fails with java.lang.NullPointerException
05f9e76: 8298974: Add ftcolor.c to imported freetype sources
abc1297: 8299044: test/jdk/javax/swing/JComboBox/JComboBoxBorderTest.java fails on non mac
5d330f5: 8299045: tools/doclint/BadPackageCommentTest.java fails after JDK-8298943
40cb431: 8298943: Missing escapes for single quote marks in compiler.properties
9194e91: 8298701: Cleanup SA entries in ProblemList-zgc.txt.
4c927df: 8298470: Short cut java.lang.Object super class loading
756a06d: 8299022: Linux ppc64le and s390x build issues after JDK-8160404
de0ce79: 8297801: printnm crashes with invalid address due to null pointer dereference
da38d43: 8296412: Special case infinite loops with unmerged backedges in IdealLoopTree::check_safepts

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.

dfuch · 2022-12-20T11:03:36Z

/integrate

openjdk · 2022-12-20T11:06:52Z

Going to push as commit e5edb10.
Since your change was applied there have been 12 commits pushed to the master branch:

5df00d3: 8298931: java/net/httpclient/CancelStreamedBodyTest.java fails with AssertionError due to Pending TCP connections: 1
36de61c: 8298865: Excessive memory allocation in CipherOutputStream AEAD decryption
dd15d30: 8299043: test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java fails with java.lang.NullPointerException
05f9e76: 8298974: Add ftcolor.c to imported freetype sources
abc1297: 8299044: test/jdk/javax/swing/JComboBox/JComboBoxBorderTest.java fails on non mac
5d330f5: 8299045: tools/doclint/BadPackageCommentTest.java fails after JDK-8298943
40cb431: 8298943: Missing escapes for single quote marks in compiler.properties
9194e91: 8298701: Cleanup SA entries in ProblemList-zgc.txt.
4c927df: 8298470: Short cut java.lang.Object super class loading
756a06d: 8299022: Linux ppc64le and s390x build issues after JDK-8160404
... and 2 more: https://git.openjdk.org/jdk/compare/8e49fcdde4fef5a8db36823b35d409ba2c9ec47b...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-20T11:07:16Z

@dfuch Pushed as commit e5edb10.

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

8299018

da0ca71

openjdk bot changed the title ~~8299018~~ 8299018: java/net/httpclient/HttpsTunnelAuthTest.java fails with java.io.IOException: HTTP/1.1 header parser received no bytes Dec 19, 2022

openjdk bot added the rfr Pull request is ready for review label Dec 19, 2022

openjdk bot added the net net-dev@openjdk.org label Dec 19, 2022

djelinski reviewed Dec 19, 2022

View reviewed changes

test/jdk/java/net/httpclient/ProxyServer.java Outdated Show resolved Hide resolved

Update test/jdk/java/net/httpclient/ProxyServer.java

08cc94d

Co-authored-by: Daniel Jelinski <djelinski1@gmail.com>

jaikiran reviewed Dec 19, 2022

View reviewed changes

Send "Connection: close" if closing. Accept not content-length for HE…

3d4db2b

…AD and GET.

djelinski approved these changes Dec 19, 2022

View reviewed changes

No special case for -1

4fadedf

jaikiran approved these changes Dec 20, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 20, 2022

openjdk bot added the integrated Pull request has been integrated label Dec 20, 2022

openjdk bot closed this Dec 20, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8299018: java/net/httpclient/HttpsTunnelAuthTest.java fails with java.io.IOException: HTTP/1.1 header parser received no bytes #11717

8299018: java/net/httpclient/HttpsTunnelAuthTest.java fails with java.io.IOException: HTTP/1.1 header parser received no bytes #11717

dfuch commented Dec 19, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

mlbridge bot commented Dec 19, 2022 •

edited

Loading

dfuch commented Dec 19, 2022

jaikiran Dec 19, 2022

dfuch Dec 19, 2022

dfuch Dec 19, 2022

djelinski commented Dec 19, 2022

dfuch commented Dec 19, 2022 •

edited

Loading

djelinski left a comment

djelinski Dec 19, 2022

djelinski Dec 19, 2022

dfuch Dec 19, 2022

jaikiran left a comment

openjdk bot commented Dec 20, 2022

dfuch commented Dec 20, 2022

openjdk bot commented Dec 20, 2022

openjdk bot commented Dec 20, 2022

		@@ -458,11 +458,14 @@ private boolean shouldCloseAfter407(List<String> headers) throws IOException {
		try {
		n = Integer.parseInt(cl);

8299018: java/net/httpclient/HttpsTunnelAuthTest.java fails with java.io.IOException: HTTP/1.1 header parser received no bytes #11717

8299018: java/net/httpclient/HttpsTunnelAuthTest.java fails with java.io.IOException: HTTP/1.1 header parser received no bytes #11717

Conversation

dfuch commented Dec 19, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

mlbridge bot commented Dec 19, 2022 • edited Loading

Webrevs

dfuch commented Dec 19, 2022

jaikiran Dec 19, 2022

Choose a reason for hiding this comment

dfuch Dec 19, 2022

Choose a reason for hiding this comment

dfuch Dec 19, 2022

Choose a reason for hiding this comment

djelinski commented Dec 19, 2022

dfuch commented Dec 19, 2022 • edited Loading

djelinski left a comment

Choose a reason for hiding this comment

djelinski Dec 19, 2022

Choose a reason for hiding this comment

djelinski Dec 19, 2022

Choose a reason for hiding this comment

dfuch Dec 19, 2022

Choose a reason for hiding this comment

jaikiran left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 20, 2022

dfuch commented Dec 20, 2022

openjdk bot commented Dec 20, 2022

openjdk bot commented Dec 20, 2022

dfuch commented Dec 19, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 19, 2022 •

edited

Loading

dfuch commented Dec 19, 2022 •

edited

Loading