8331195: Improve com.sun.net.httpserver.HttpExchange usability #28132

Michael-Mc-Mahon · 2025-11-04T14:10:14Z

I'd like to return to this issue with a new PR that addresses the usability issue minimally. Namely to define numeric constants
for the counter-intuitive responseLength values in HttpExchange.sendResponseHeaders. The apidoc then refers to the constant names rather than the numeric values.

I've updated the implementation and some tests (though not all yet) to use the constant names. I'm open to suggestions on the names themselves. Once that is agreed, I'll update the remaining tests and any other call sites in the implementation.

Thanks,
Michael

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
Change requires CSR request JDK-8371247 to be approved

Issues

JDK-8331195: Improve com.sun.net.httpserver.HttpExchange usability (Enhancement - P4)
JDK-8371247: Improve com.sun.net.httpserver.HttpExchange usability (CSR)

Reviewers

Daniel Fuchs (@dfuch - Reviewer) Review applies to 15f4179f
Jaikiran Pai (@jaikiran - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28132

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

Using diff file

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

Using Webrev

Link to Webrev Comment

Michael-Mc-Mahon · 2025-11-04T14:10:46Z

/csr needed

bridgekeeper · 2025-11-04T14:11:22Z

👋 Welcome back michaelm! 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 · 2025-11-04T14:12:34Z

@Michael-Mc-Mahon 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:

8331195: Improve com.sun.net.httpserver.HttpExchange usability

Reviewed-by: jpai, dfuchs

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 95 new commits pushed to the master branch:

98f40e4: 8371425: Include folder names in vscode workspace virtual folders
681dab7: 8365972: JFR: ThreadDump and ClassLoaderStatistics events may cause back to back rotations
49f51f9: 8370612: Simplify implementation of dark theme
... and 92 more: https://git.openjdk.org/jdk/compare/e6546683a8dd9a64255ce4c5606089068ec92e5d...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 · 2025-11-04T14:13:53Z

@Michael-Mc-Mahon has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@Michael-Mc-Mahon please create a CSR request for issue JDK-8331195 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk · 2025-11-04T14:14:31Z

@Michael-Mc-Mahon 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 · 2025-11-04T14:17:43Z

Webrevs

dfuch · 2025-11-04T16:02:41Z

This is a good idea, but it kind of hides the fact that passing 0 may not do what you think it does.

Typically the pitfall is to assume that if you have an array of bytes then you can can call sendResponseHeaders with the array length. The API doc of sendResponseHeaders should probably make it clear that 0 is a special value, and that RSPBODY_CHUNKED is 0.

In addition I would suggest adding code snipets showing both cases (one using a byte array and passing bytes.length == 0 ? RSPBODY_EMPTY : bytes.length and the other using RSPBODY_CHUNKED with e.g. transferring the content of an InputStream.

Michael-Mc-Mahon · 2025-11-04T16:23:43Z

This is a good idea, but it kind of hides the fact that passing 0 may not do what you think it does.

Typically the pitfall is to assume that if you have an array of bytes then you can can call sendResponseHeaders with the array length. The API doc of sendResponseHeaders should probably make it clear that 0 is a special value, and that RSPBODY_CHUNKED is 0.

Actually, that case is not so bad. What happens is that chunked encoding is selected but the response body
content is empty as intended, and it should work. But, maybe we should still highlight it anyway.

In addition I would suggest adding code snipets showing both cases (one using a byte array and passing bytes.length == 0 ? RSPBODY_EMPTY : bytes.length and the other using RSPBODY_CHUNKED with e.g. transferring the content of an InputStream.

The worse case is expecting a negative value to mean an indefinite (chunked encoded) length. You definitely don't get what you expect in that case.

Michael-Mc-Mahon · 2025-11-05T08:21:36Z

This is a good idea, but it kind of hides the fact that passing 0 may not do what you think it does.

Typically the pitfall is to assume that if you have an array of bytes then you can can call sendResponseHeaders with the array length. The API doc of sendResponseHeaders should probably make it clear that 0 is a special value, and that RSPBODY_CHUNKED is 0.

In addition I would suggest adding code snipets showing both cases (one using a byte array and passing bytes.length == 0 ? RSPBODY_EMPTY : bytes.length and the other using RSPBODY_CHUNKED with e.g. transferring the content of an InputStream.

I've added an API note. I'd prefer not to drown the whole spec out with code samples just for this issue if possible.

dfuch

Thanks for adding the @apiNote and explicitely mentionning the value of RSPBODY_CHUNKED. The new text looks good to me now.

Could be good to double check what would happen if you have something like that:

public void handle(final HttpExchange exchange) throws IOException {
     byte[] bytes = getResponseBody(exchange.getRequestURI());
     long fixedlen = bytes.length == 0 ? RSPBODY_EMPTY : bytes.length;
     exchange.sendResponseHeaders(200, fixedlen);
     try (OutputStream os = exchange.getResponseBody()) {
         os.write(bytes);
     }
}

where getResponseBody(exchange.getRequestURI()); returns new byte[0];
will os.write throw, or will it be no-op?
If it throws then should it be specified that sendResponseHeaders closes the output stream?

Michael-Mc-Mahon · 2025-11-05T13:38:41Z

Thanks for adding the @apiNote and explicitely mentionning the value of RSPBODY_CHUNKED. The new text looks good to me now.

Could be good to double check what would happen if you have something like that:
public void handle(final HttpExchange exchange) throws IOException {
     byte[] bytes = getResponseBody(exchange.getRequestURI());
     long fixedlen = bytes.length == 0 ? RSPBODY_EMPTY : bytes.length;
     exchange.sendResponseHeaders(200, fixedlen);
     try (OutputStream os = exchange.getResponseBody()) {
         os.write(bytes);
     }
}
where getResponseBody(exchange.getRequestURI()); returns new byte[0]; will os.write throw, or will it be no-op? If it throws then should it be specified that sendResponseHeaders closes the output stream?

That would work fine. What possibly needs to be mentioned is that with chunked encoding, you must always
close the output stream explicitly (or through a try with resources block as in your example). I will add
a sentence to that effect.

dfuch · 2025-11-05T14:53:49Z

src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpExchange.java

+    /**
+     * No response body is being sent with this response
+     *
+     * @since 26
+     */
+    public static final long RSPBODY_EMPTY = -1l;
+
+    /**
+     * The response body is unspecified and will be chunk encoded
+     *
+     * @since 26
+     */
+    public static final long RSPBODY_CHUNKED = 0;


Should we add:

@see #sendResponseHeaders(int, long)

to make it clearer what these constants are for?

Should we add:

@see #sendResponseHeaders(int, long)

to make it clearer what these constants are for?

Good idea

bowbahdoe · 2025-11-05T16:05:05Z

I've said all my objections before. I think if we waited for member patterns we could have an API that doesn't need the constants and I think that the constants do not solve for the bad patterns today.

#18955

bowbahdoe · 2025-11-05T16:07:42Z

I think it makes the API very slightly better, but in a way that would detract from future attempts to make the API holistically better to use.

Seriously consider that if we had member patterns would we add this API? If the answer is no (and I think it is no) lets just not do it

bowbahdoe · 2025-11-05T16:14:29Z

public void handle(final HttpExchange exchange) throws IOException {
     byte[] bytes = getResponseBody(exchange.getRequestURI());
     long fixedlen = bytes.length == 0 ? RSPBODY_EMPTY : bytes.length;
     exchange.sendResponseHeaders(200, fixedlen);
     try (OutputStream os = exchange.getResponseBody()) {
         os.write(bytes);
     }
}

versus

public void handle(final HttpExchange exchange) throws IOException {
     byte[] bytes = getResponseBody(exchange.getRequestURI());
     exchange.sendResponseHeaders(200, ResponseLength.known(bytes));
     try (OutputStream os = exchange.getResponseBody()) {
         os.write(bytes);
     }
}

This has the benefit of us being able to deprecate .sendResponseHeaders(int, long) to guide usage to it. The only reason not to do it today is that we lack the ability to make ResponseLength in the way that is ideal (with known(...) and unknown() having matching extraction patterns)

And in the presence of such an API there are no uses for the constants so...do you see my frustration? You need to know to look for RSPBODY_EMPTY and write bytes.length == 0 ? RSPBODY_EMPTY : bytes.length; This will not lead to a better result.

dfuch · 2025-11-05T16:31:52Z

I don't disagree but adding new APIs is a bigger maintenance cost than adding two simple constants.
I believe the proposed change will improve the usability. The sendResponseHeaders(int, long) cannot be removed, and even if we added a new API it would still be here. There's also the impacts on the SPI to take into account.

Michael-Mc-Mahon · 2025-11-06T13:25:17Z

I don't agree that you have to go looking for these constants. I think we have to look at it from the point of view of a developer new to the API and someone reading the apidoc for the first time will be clearly drawn to the constant names. I agree a new method with new parameter types would be nicer from an IDE perspective. But if we are going to add a new method then I think we are going to have a wider discussion such as occurred previously with this issue. So, even if we end up doing that and deprecating the present sendResponseHeaders() this effort is not going to get in the way of that.

dfuch

LGTM. Please hold off integrating right now - I've launched a couple of tests.

Michael-Mc-Mahon · 2025-11-08T21:47:41Z

LGTM. Please hold off integrating right now - I've launched a couple of tests.

My tier1 and tier2 test run passes

jaikiran · 2025-11-10T11:20:17Z

test/jdk/com/sun/net/httpserver/ServerStopTerminationTest.java

                0,
                "/", exchange -> {
-                    exchange.sendResponseHeaders(200, 0);
+                    exchange.sendResponseHeaders(200, RSPBODY_EMPTY);


I'm guessing that this is intentionally using RSPBODY_EMPTY and previously it was a bug to send 0?

Never mind, I just heard the answer to this offline. This looks OK to me.

Hi Jaikiran. I think 0 was probably not intended. I decided to "fix" it in this place, but leave it in the others

jaikiran · 2025-11-10T11:24:57Z

test/jdk/com/sun/net/httpserver/bugs/8300268/MaxIdleConnectionsTest.java

            System.out.println("Sending response for request " + exchange.getRequestURI() + " from " + exchange.getRemoteAddress());
            reqFinishedProcessing.countDown();
-            exchange.sendResponseHeaders(200, 0);
+            exchange.sendResponseHeaders(200, RSPBODY_CHUNKED);


Should this use RSPBODY_EMPTY instead?

As noted above, I decided to change it in one place, but leave it as is, in others.

Thank you Michael. That's fine with me.

jaikiran · 2025-11-10T11:33:25Z

Overall this looks good to me. I just have one question which I've added inline.

jaikiran

The copyright year updates look good to me and so does the rest of this PR. I just have one comment here #28132 (comment)

Michael-Mc-Mahon · 2025-11-10T14:33:06Z

/integrate

openjdk · 2025-11-10T14:34:32Z

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

9d2fa8f: 8371321: Remove unused last arg of BarrierSetAssembler::arraycopy_epilogue
1142d29: 8369920: HttpClient QuicSelectorThread could be a VirtualThread
98f40e4: 8371425: Include folder names in vscode workspace virtual folders
... and 94 more: https://git.openjdk.org/jdk/compare/e6546683a8dd9a64255ce4c5606089068ec92e5d...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-11-10T14:34:48Z

@Michael-Mc-Mahon Pushed as commit 1877ff9.

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

Michael-Mc-Mahon added 3 commits November 4, 2025 11:47

doc update

8d401c4

Merge branch 'master' into httpserver-8368955

42b2a32

implementation and test

8fe3a2b

openjdk bot added the csr Pull request needs approved CSR before integration label Nov 4, 2025

openjdk bot added the net net-dev@openjdk.org label Nov 4, 2025

openjdk bot added the rfr Pull request is ready for review label Nov 4, 2025

doc update

5f9f259

revert unintended spec change to sendResponseHeaders

f567e10

dfuch reviewed Nov 5, 2025

View reviewed changes

clarify empty byte[] behavior

df70fb5

dfuch approved these changes Nov 5, 2025

View reviewed changes

dfuch reviewed Nov 5, 2025

View reviewed changes

update

5633b0d

dfuch approved these changes Nov 5, 2025

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Nov 5, 2025

updated remaining callsites for new constants

15f4179

openjdk bot removed the ready Pull request is ready to be integrated label Nov 7, 2025

dfuch approved these changes Nov 7, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 7, 2025

jaikiran reviewed Nov 10, 2025

View reviewed changes

Copyright header updates

64073c6

openjdk bot removed the ready Pull request is ready to be integrated label Nov 10, 2025

jaikiran approved these changes Nov 10, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 10, 2025

openjdk bot added the integrated Pull request has been integrated label Nov 10, 2025

openjdk bot closed this Nov 10, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 10, 2025

8331195: Improve com.sun.net.httpserver.HttpExchange usability #28132

8331195: Improve com.sun.net.httpserver.HttpExchange usability #28132

Uh oh!

Conversation

Michael-Mc-Mahon commented Nov 4, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

Michael-Mc-Mahon commented Nov 4, 2025

Uh oh!

bridgekeeper bot commented Nov 4, 2025

Uh oh!

openjdk bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Nov 4, 2025

Uh oh!

openjdk bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dfuch commented Nov 4, 2025

Uh oh!

Michael-Mc-Mahon commented Nov 4, 2025

Uh oh!

Michael-Mc-Mahon commented Nov 5, 2025

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

Michael-Mc-Mahon commented Nov 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bowbahdoe commented Nov 5, 2025

Uh oh!

bowbahdoe commented Nov 5, 2025

Uh oh!

bowbahdoe commented Nov 5, 2025

Uh oh!

dfuch commented Nov 5, 2025

Uh oh!

Michael-Mc-Mahon commented Nov 6, 2025

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

Michael-Mc-Mahon commented Nov 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaikiran commented Nov 10, 2025

Uh oh!

jaikiran left a comment

Choose a reason for hiding this comment

Uh oh!

Michael-Mc-Mahon commented Nov 10, 2025

Uh oh!

openjdk bot commented Nov 10, 2025

Uh oh!

openjdk bot commented Nov 10, 2025

Uh oh!

Michael-Mc-Mahon commented Nov 4, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Nov 4, 2025 •

edited

Loading

openjdk bot commented Nov 4, 2025 •

edited

Loading

mlbridge bot commented Nov 4, 2025 •

edited

Loading