8347000: Bug in com/sun/net/httpserver/bugs/B6361557.java test

[jaikiran]

Can I please get a review of this change which proposes to address the test-only issue noted in https://bugs.openjdk.org/browse/JDK-8347000?

As noted in that issue, the test issues HTTP requests with Content-Length set to 0 implying no request body. However, the code unintentionally sends additional bytes (14 bytes each request) as part of the request. The JDK's HttpServer implementation currently doesn't cause errors for such requests (although the HTTP/1.1 RFC expects an error to be raised).

The change in this PR merely addresses the test code to not send these additional bytes. The test continues to pass after this change.

---

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-8347000: Bug in com/sun/net/httpserver/bugs/B6361557.java test (Bug - P4)

Reviewers

• Mark Sheppard (@msheppar - Reviewer)
• Michael McMahon (@Michael-Mc-Mahon - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22921

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jpai! 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]

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

8347000: Bug in com/sun/net/httpserver/bugs/B6361557.java test

Reviewed-by: msheppar, 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 21 new commits pushed to the master branch:

• 3f7052e: 8346868: RISC-V: compiler/sharedstubs tests fail after JDK-8332689
• f119663: 8346573: Can't use custom default file system provider with custom system class loader
• 379ac34: 8346838: RISC-V: runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java crash with debug VMs
• 0285020: 8345676: [ubsan] ProcessImpl_md.c:561:40: runtime error: applying zero offset to null pointer on macOS aarch64
• 0a81676: 8346881: [ubsan] logSelection.cpp:154:24 / logSelectionList.cpp:72:94 : runtime error: applying non-zero offset 1 to null pointer
• 08debd3: 8346993: C2 SuperWord: refactor to make more vector nodes available in VectorNode::make
• de02503: 8346832: runtime/CompressedOops/CompressedCPUSpecificClassSpaceReservation.java fails on RISC-V
• 27646e5: 8344148: Add an explicit compiler phase for warning generation
• 8d388cc: 8346457: AOT cache creation crashes with "assert(pair_at(i).match() < pair_at(i+1).match()) failed: unsorted table entries"
• d723597: 8345432: (ch, fs) Replace anonymous Thread with InnocuousThread
• ... and 11 more: https://git.openjdk.org/jdk/compare/e98f41266346aa676a3e764528806f2b82ec7e46...master

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]

@jaikiran 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]

Webrevs

• 00: Full (9d742aee)

[jaikiran]

The JDK's HttpServer implementation currently doesn't cause errors for such requests (although the HTTP/1.1 RFC expects an error to be raised).

Having read the RFCs again, I think the JDK's HttpServer implementation isn't violating any expectations set by the RFC. I have responded to the net-dev thread with my reasoning https://mail.openjdk.org/pipermail/net-dev/2025-January/025301.html.

[msheppar]

While the change is reasonable, as it will avoid any ambiguous interpretation of the HTTP GET request used in the test,
the rationale given for the change, is questionable, and based on a misinterpretation of the HTTP spec.
That is to say:
“As noted in that issue, the test issues HTTP requests with Content-Length set to 0 implying no request body. However, the code unintentionally sends additional bytes (14 bytes each request) as part of the request. The JDK's HttpServer implementation currently doesn't cause errors for such requests (although the HTTP/1.1 RFC expects an error to be raised).”

is not totally correct -- "although the HTTP/1.1 RFC expects an error to be raised", raising of an "error" relates to User Agent behaviour, not the server.

The HttpServer used in the test is parsing the GET request correctly and interpreting the Content-Length: 0 correctly, as there is no request body associated with the request, when presented to the HttpHandler. This is can be observed by instrumenting the HttpHandler’s handle method and checking for any request body data.

The delimiting of the test’s GET request is Content-Length:0 and the two blank lines. These are the key inputs when parsing a request, hence the additional data written on the HTTP content is not relevant. The stray bytes might cause a problem if a JDK HttpServer were processing a “stream of requests”. But, it would be expected that the server input processing to discard any stray data until to reads a valid request method.

Additionally, based on your assumptions, above, an HTTP user agent would not be able to submit consecutive HTTP GET request (with Content-Length: 0 ) on the same TCP connection, to a JDK HttpServer.

The change is fine, but I would suggest correcting your assumption in the context of the test.

[Michael-Mc-Mahon]

Change looks fine.

We probably should have a test of the server that checks it handles pipelined requests correctly (if not optimally). Because a thread is basically assigned to a connection for the duration of a request, it means that pipelined requests will be (or should be in this impl) handled in sequence. Like I said, this isn't optimal, but then again, HTTP/1.1 pipelining has limitations that have been ameliorated by the newer versions of the protocol h2 and h3 (QUIC).

[jaikiran]

Thank you Mark and Michael for the reviews.

We probably should have a test of the server that checks it handles pipelined requests correctly (if not optimally). Because a thread is basically assigned to a connection for the duration of a request, it means that pipelined requests will be (or should be in this impl) handled in sequence.

I will look into this and see if there's already any test where we do this. If not, I'll experiment with one as a separate task.

[jaikiran]

/integrate

[openjdk]

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

• 3f7052e: 8346868: RISC-V: compiler/sharedstubs tests fail after JDK-8332689
• f119663: 8346573: Can't use custom default file system provider with custom system class loader
• 379ac34: 8346838: RISC-V: runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java crash with debug VMs
• 0285020: 8345676: [ubsan] ProcessImpl_md.c:561:40: runtime error: applying zero offset to null pointer on macOS aarch64
• 0a81676: 8346881: [ubsan] logSelection.cpp:154:24 / logSelectionList.cpp:72:94 : runtime error: applying non-zero offset 1 to null pointer
• 08debd3: 8346993: C2 SuperWord: refactor to make more vector nodes available in VectorNode::make
• de02503: 8346832: runtime/CompressedOops/CompressedCPUSpecificClassSpaceReservation.java fails on RISC-V
• 27646e5: 8344148: Add an explicit compiler phase for warning generation
• 8d388cc: 8346457: AOT cache creation crashes with "assert(pair_at(i).match() < pair_at(i+1).match()) failed: unsorted table entries"
• d723597: 8345432: (ch, fs) Replace anonymous Thread with InnocuousThread
• ... and 11 more: https://git.openjdk.org/jdk/compare/e98f41266346aa676a3e764528806f2b82ec7e46...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jaikiran Pushed as commit 5e6cda4.

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