RFR: 7026262: HttpServer: improve handling of finished HTTP exchanges

[Daniel Fuchs]

On Thu, 23 Mar 2023 11:33:32 GMT, Daniel Jeliński <djelinski at openjdk.org> wrote:

> This patch fixes the following issues with HttpHandler exception handling:
> - The connection is no longer closed if an exception is thrown after the response is completed. As soon as the response is completed, the connection is added back to the pool, ready for reuse. Closing the connection would race with the subsequent handler.
> - The stream returned by `getResponseBody` is now usable even if the response has zero-length body. Writing any data to the stream will still fail, but zero-length writes and closing the stream will now succeed.
> - `ServerImpl.Exchange.reject` now sends a `Connection: close` header in addition to closing the connection
> 
> The test `B8293562` had to be adjusted to avoid `Connection: close`.
> Additionally, while I was looking for a good test to copy from, I found and fixed a bug in test `B5045306`.
> 
> The new tests are passing with this patch, failing without it. Tier 1-3 clean.

src/jdk.httpserver/share/classes/sun/net/httpserver/FixedLengthOutputStream.java line 76:

> 74:         if (eof) {
> 75:             throw new StreamClosedException();
> 76:         }

Maybe before checking `closed` we should just return if len == 0? (and throw if < 0 BTW)
Possibly consider doing the same for ChunkedOutputStream. 
Also ChunkedOutputStream throws StreamClosedException() when `closed` is true. Not sure why we're throwing plain IO here. Or rather - not sure why we have StreamClosedException() in the first place, it doesn't seem to be handled specially anywhere, and it can reach user code (code calling write()), and it's not a public exception - so maybe it should be removed altogether and replaced with IOException("stream closed") everywhere? But getting rid of  StreamClosedException may be a task for another PR.

src/jdk.httpserver/share/classes/sun/net/httpserver/FixedLengthOutputStream.java line 89:

> 87:             // WriteFinishedEvent was sent already; nothing to do
> 88:             return;
> 89:         }

Wouldn't it be more correct to have sendResponseHeader close the output stream, and let the output stream `close` generate the write event, rather than having sendResponseHeader generate the event and not doing anything here? Or would there be an issue with the case where the stream is chunked output stream?

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 881:

> 879:                 code, true, "<h1>"+code+Code.msg(code)+"</h1>"+message
> 880:             );
> 881:             closeConnection(connection);

Looks like this line (881) is now redundant, since `sendReply(code, true, ...)` will close the connection?

test/jdk/sun/net/www/http/KeepAliveCache/B5045306.java line 172:

> 170:             String path = trans.getRequestURI().getPath();
> 171:             if (path.equals("/firstCall")) {
> 172:                 port1 = trans.getRemoteAddress().getPort();

Arguably we could store both the local port and the remote port and check both - since a connection is identified by a tuple. If any of the two differ then it's a new connection. Would avoid having to think about which should be compared :-)

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13157#discussion_r1146664242
PR Review Comment: https://git.openjdk.org/jdk/pull/13157#discussion_r1146659226
PR Review Comment: https://git.openjdk.org/jdk/pull/13157#discussion_r1146625889
PR Review Comment: https://git.openjdk.org/jdk/pull/13157#discussion_r1146644559