8368695: Support 101 switching protocol in jdk.httpserver

[SentryMan]

• adds a flag to ExchangeImpl to signal whether the current request is an Upgrade request
• Adds a new UpgradeInputStream to ensure that the server keeps track of when the upgraded request is closed
• on 101 response codes, sendResponseHeaders will not immediately close the output stream
• on 101 response codes, sendResponseHeaders will use an UndefLengthOutputStream

---

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-8368695: Support 101 switching protocol in jdk.httpserver (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27751

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

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

@SentryMan Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[mlbridge]

Webrevs

• 12: Full (ae2b1184)
• 11: Full - Incremental (8872bca5)
• 10: Full - Incremental (a3250508)
• 09: Full - Incremental (fb551787)
• 08: Full - Incremental (3d1e7f58)
• 07: Full - Incremental (b8f58985)
• 06: Full - Incremental (cc80cd9c)
• 05: Full - Incremental (a648a7ba)
• 04: Full - Incremental (7818864f)
• 03: Full - Incremental (49772413)
• 02: Full - Incremental (ee8b2bcd)
• 01: Full - Incremental (d3835941)
• 00: Full (8b68230c)

[Michael-Mc-Mahon]

Is the intention behind this to support websocket?

[SentryMan]

Is the intention behind this to support websocket?

To speak plainly, my plan is not for adding official websocket support into the JDK. That part I can do on my own, the only thing I lack is access to the input/output streams after sending a 101 code. (the current logic in send headers closes both after sending 101 headers)

[SentryMan]

Don't get me wrong though, if you guys decide you want to fully support websocket, I'll gladly offer a PR. (But I highly doubt such a thing will happen)

[Michael-Mc-Mahon]

Don't get me wrong though, if you guys decide you want to fully support websocket, I'll gladly offer a PR. (But I highly doubt such a thing will happen)

Right, I don't think we want to support websocket. There are plenty of third party implementations out there and it's a bit too far removed from the purpose as a lightweight server, for serving mainly static content.

However, if we were to allow this enabling functionality, it would have to be documented and specified, so that other people could use it. I'd have preferred if the general shape of such an "upgrade API" was discussed on the net-dev email list but we can discuss it here, without prejudice to whether it goes ahead or not.

As a minimum, the feature would have to be "opt in" by the handler, probably through a new method on HttpExchange and we have to consider the impact on the rest of the API, such as what happens when HttpServer.stop is called.

[SentryMan]

it would have to be documented and specified, so that other people could use it.

For me, it was already implied that 101 status could be sent, because the documentation for sendResponseHeaders did not even hint that it treats informational responses like 204/304 status codes and closes the streams. No matter what happens,
I agree that we should update the documentation.

probably through a new method on HttpExchange

For http2 I'm in agreement for a new method, as supporting that kind of upgrade is has far more complexity and requires consuming the body before upgrading.

For WebSocket though, such a thing can be accomplished cleanly through the current API. Indeed, in some other implementations of jdk.httpserver I'm able to upgrade to websocket with the same API because 101 status was not treated like a 204, and the streams were not closed immediately.

As a minimum, the feature would have to be "opt in" by the handler,

Requiring a manual call of sendResponseHeaders(101, 0) and the requirement to custom implement the entire protocol appears pretty opt in to me.

we have to consider the impact on the rest of the API, such as what happens when HttpServer.stop is called.

Testing locally, calling stop appears to work the same as an extended GET request. What kind of test assertions would you like to see?

[SentryMan]

work the same as an extended GET request.

In fact, this might be a good way of thinking about it. When you call sendResponseHeaders(200, 0) a similar thing happens to what I'm asking for 101. The streams are not closed, and the handler is free to read the inputstream and write to the outputstream as it pleases.

    server.createContext(
        "/test",
        exchange -> {
          exchange.sendResponseHeaders(200, 0);
          exchange.getResponseBody().write("My Response".getBytes());
          try (var is = exchange.getRequestBody()) {
            is.transferTo(exchange.getResponseBody());
          }
        });

[dfuch]

Requiring a manual call of sendResponseHeaders(101, 0) and the requirement to custom implement the entire protocol appears pretty opt in to me.

Right. But your PR changes the request body input stream preemptively, before sendResponseHeaders(101, 0) is called.
Which let me think that sendResponseHeaders is not the right API for doing this.

[SentryMan]

But your PR changes the request body input stream preemptively,

That is true, but it's only for GET requests. Thus it effectively remains optional as regular GET request handlers wouldn't attempt to read the body.

[dfuch]

So your proposal is that we support switching protocols only for GET requests, and only for those that have no Content-length header in the request header?

[SentryMan]

Essentially yes, because that's all that's needed to support implementing the websocket protocol.

[dfuch]

But that would be an incompatible change, isn't it?

Consider the following:

httpserver.createContext("/echo/", (exch) -> {
   byte[] bytes;
   try (var is = exch.getRequestBody()) { bytes = is.readAllBytes(); }
   exch.sendResponseHeaders(200, bytes.length == 0 ? -1  : bytes.length);
   try (var os = exch.getResponseBody()) { if (bytes.length > 0) os.write(bytes); } 
});

wouldn't that now block forever if the request contains an upgrade header?

[SentryMan]

But that would be an incompatible change, isn't it?

That we even return an inputstream for a GET request is pretty strange but yeah that would indeed block forever. I'll fix to keep compatibility.

[openjdk]

@SentryMan build, client, compiler, core-libs, graal, hotspot, i18n, javadoc, nio, security, serviceability, shenandoah have been added to this pull request based on files touched in new commit(s).

[SentryMan]

/label remove core-libs, build, client, compiler, core-libs, core-libs, graal, hotspot, i18n, javadoc, nio, security, serviceability, shenandoah

[openjdk]

@SentryMan
The core-libs label was successfully removed.

The build label was successfully removed.

The client label was successfully removed.

The compiler label was successfully removed.
The core-libs label was not set.
The core-libs label was not set.

The graal label was successfully removed.

The hotspot label was successfully removed.

The i18n label was successfully removed.

The javadoc label was successfully removed.

The nio label was successfully removed.

The security label was successfully removed.

The serviceability label was successfully removed.

The shenandoah label was successfully removed.