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

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

[dfuch]

Before going any further in the review I would like to see how this new feature would be documented. If we eventually decide to take it on board, then more extensive tests will be needed too.

[SentryMan]

I would like to see how this new feature would be documented

My thought was to add an implnote to getRequestBody saying something to the effect of:

@implNote For GET requests with an Upgrade header, the returned stream will not return any data unless the protocol switch is first performed by calling {@link #sendResponseHeaders(int, long) sendResponseHeaders(101, 0)}

For sendResponseHeaders, it's currently already implied that you can send 101. So I would add an implnote clarifying the behavior. Something like:

@implNote This implementation offers limited support for connection upgrade on GET requests. If the request included an {@code Upgrade} header, and the response code is 101 (Switching Protocols), then the handler can use the streams returned by {@link #getRequestBody()} and {@link #getResponseBody()} to implement the desired protocol.

more extensive tests will be needed too.

Naturally, tell me what other cases you'd like and it shall be done

[Michael-Mc-Mahon]

Here is a proposal for discussion. This would be added to the end of the class level docs for HttpExchange

 <b>Upgrading HTTP/1.1 connections</b> <br>Sending a 101 response to a request from the client to upgrade to some other protocol has the effect of detaching the underlying connection from the HTTP stack. The {@link InputStream} and {@link OutputStream} returned from {@link #getRequestBody()} and {@link #getResponseBody()} respectively, after sending the 101 response, returns raw streams to the underlying socket. {@code sendResponseHeaders(101, -1)} must be called prior to obtaining the detached input and output streams. It is also an error to send a 101 response if no {@code Upgrade} header was present in the request, or the request was anything other than a GET (or CONNECT?) or to pass a content length other than {@code -1}. Other than that, the library does not restrict or interpret the Upgrade header value. It is up to the calling code to implement whatever higher level protocol is required over the underlying socket. It is up to the calling code to eventually obtain and eventually close both the detached {@link #getRequestBody() InputStream} and {@link#getResponseBody() OutputStream} to prevent resource leaks. Closing the exchange after {@code sendResponseHeaders(101, -1)} has been called has no effect on these streams. Terminating the server with {@link HttpServer.stop(int)} closes all sockets associated with the server including any sockets detached through this mechanism. 

The apidoc for HttpServer.stop would need to mention that sockets detached from the upgrade mechanism are also closed.

[dfuch]

Thanks @Michael-Mc-Mahon for driving this discussion. Would this text be normative - that it - would any implementation plugged through the SPI need to support this? If not we should find some way to tie that to the JDK built-in implementation. I know that the HttpServer API itself is not part of the Java SE spec - but maybe we should set some expectations of what an implementation plugged through the SPI must, should, or might do.

[SentryMan]

Sending a 101 response to a request from the client to upgrade to some other protocol has the effect of detaching
the underlying connection from the HTTP stack.

Isn't the behavior the same as with regular chunked encoding? Do we need a note for that as well? In both cases we aren't exactly passing a reference to the raw streams.

Closing the exchange after {@code
sendResponseHeaders(101, -1)} has been called has no effect on these streams.

Doesn't closing the exchange call close on both the input/output stream?

[dfuch]

Sending a 101 response to a request from the client to upgrade to some other protocol has the effect of detaching
the underlying connection from the HTTP stack.

Isn't the behavior the same as with regular chunked encoding? Do we need a note for that as well? In both cases we aren't exactly passing a reference to the raw streams.

Well - no. The connection will be reused for the next request when the chunked streams are closed.

Closing the exchange after {@code
sendResponseHeaders(101, -1)} has been called has no effect on these streams.

Doesn't closing the exchange call close on both the input/output stream?

We may need to link back to the upgrade paragraph in HttpExchange::close. Good remark.
The notion of "HTTP exchange" doesn't really make sense after the protocol is upgraded. That is - the HTTP/1.1 exchange should be considered closed as soon as the upgrade is effective.

[SentryMan]

Well - no. The connection will be reused for the next request when the chunked streams are closed.

I see, we should probably close the actual streams when calling close on the upgrade stream.

The notion of "HTTP exchange" doesn't really make sense after the protocol is upgraded. That is - the HTTP/1.1 exchange should be considered closed as soon as the upgrade is effective.

I suppose you're right, but it would be nice if calling close on HttpExchange would also close the upgraded streams.

[Michael-Mc-Mahon]

Sending a 101 response to a request from the client to upgrade to some other protocol has the effect of detaching
the underlying connection from the HTTP stack.

Isn't the behavior the same as with regular chunked encoding? Do we need a note for that as well? In both cases we aren't exactly passing a reference to the raw streams.

Well - no. The connection will be reused for the next request when the chunked streams are closed.

Chunked encoding does not expose the "raw" socket either. There is a framing structure (though primitive)
associated with each chunk.

Closing the exchange after {@code
sendResponseHeaders(101, -1)} has been called has no effect on these streams.

Doesn't closing the exchange call close on both the input/output stream?

We may need to link back to the upgrade paragraph in HttpExchange::close. Good remark. The notion of "HTTP exchange" doesn't really make sense after the protocol is upgraded. That is - the HTTP/1.1 exchange should be considered closed as soon as the upgrade is effective.

[Michael-Mc-Mahon]

Thanks @Michael-Mc-Mahon for driving this discussion. Would this text be normative - that it - would any implementation plugged through the SPI need to support this? If not we should find some way to tie that to the JDK built-in implementation. I know that the HttpServer API itself is not part of the Java SE spec - but maybe we should set some expectations of what an implementation plugged through the SPI must, should, or might do.

Maybe we could allow sendResponseHeaders to throw UnsupportedOperationException if the function is not supported. That would be a fully compatible change I think.

@throws UnsupportedOperationException if the handler tries to upgrade by sending a 101 response and the function is not supported by the implementation

Actually that won't work. Let me think about it ... It really needs to be a new method to allow for existing implementations that don't support it ... Something like sendUpgradeResponse() with a default implementation that throws UOE.

[SentryMan]

Would this text be normative - that it - would any implementation plugged through the SPI need to support this?

While some implementations do it that way I'm not completely sure it's right to make all of them try to implement Upgrade.

Maybe we could allow sendResponseHeaders to throw UnsupportedOperationException if the function is not supported.

Not totally sold on this

Actually that won't work. Let me think about it ... It really needs to be a new method to allow for existing implementations that don't support it ... Something like sendUpgradeResponse() with a default implementation that throws UOE.

A separate method to do essentially the same thing as sendResponseHeaders? The main change is the implicit swap of the streams. I would be agreeable with this if sendUpgradeResponse() simply called sendResponseHeaders(101, 0) for convenience.

For context on why I would prefer to stick with sendResponseHeaders, there are other impls that currently support sending 101 in this way and I would like my code to be compatible so I can cleanly switch to the JDK implementation (Assuming the feature is accepted).