On Wed, 18 Feb 2026 04:11:56 GMT, Josiah Noel <[email protected]> wrote:
>> Remaking the PR since I messed up the upstream merge on the other one. See >> (https://github.com/openjdk/jdk/pull/27751) for the bulk of the previous >> discussion >> >> - adds a flag to ExchangeImpl to signal whether the current request is a GET >> Upgrade request >> - Adds a new `UpgradeInputStream`/`UpgradeOutputStream` 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 >> `UpgradeInputStream` > > Josiah Noel has updated the pull request incrementally with one additional > commit since the last revision: > > Update UpgradeOutputStream.java src/jdk.httpserver/share/classes/sun/net/httpserver/ExchangeImpl.java line 207: > 205: && headers.containsKey("Upgrade") > 206: && > values.stream().filter("Upgrade"::equalsIgnoreCase).findAny().isPresent(); > 207: } I believe we should also check that `Content-Length` and `Transfer-Encoding` are not present, as that would indicate a GET with a body. src/jdk.httpserver/share/classes/sun/net/httpserver/ExchangeImpl.java line 308: > 306: > 307: /* check for connection upgrade */ > 308: if (rCode == 101) { Shouldn't we check `upgrade` here too? src/jdk.httpserver/share/classes/sun/net/httpserver/ExchangeImpl.java line 315: > 313: () -> "sendResponseHeaders: rCode = " + rCode + ": > forcing contentLen = 0"); > 314: } > 315: contentLen = 0; Suggestion: contentLen = RSPBODY_CHUNKED; src/jdk.httpserver/share/classes/sun/net/httpserver/ExchangeImpl.java line 349: > 347: } else if (upgrade && rCode == 101) { > 348: o.setWrappedStream (new UpgradeOutputStream(this, > ros)); > 349: close = true; Suggestion: // the connection should not be returned to the pool but should // be closed when the upgraded exchange finishes close = true; src/jdk.httpserver/share/classes/sun/net/httpserver/UpgradeInputStream.java line 2: > 1: /* > 2: * Copyright (c) 2005, 2026, Oracle and/or its affiliates. All rights > reserved. Suggestion: * Copyright (c) 2026, Oracle and/or its affiliates. All rights reserved. src/jdk.httpserver/share/classes/sun/net/httpserver/UpgradeInputStream.java line 46: > 44: if (!t.upgraded) { > 45: return -1; > 46: } This will prevent draining the input stream properly if the request is not upgraded. src/jdk.httpserver/share/classes/sun/net/httpserver/UpgradeInputStream.java line 69: > 67: t.getServerImpl().requestCompleted(t.getConnection()); > 68: } > 69: } This is incorrect if `t.upgraded == false`. I believe there need to be more thought about what happens when an upgrade request is not upgraded. src/jdk.httpserver/share/classes/sun/net/httpserver/UpgradeInputStream.java line 72: > 70: > 71: @Override > 72: public byte[] readAllBytes() throws IOException { should throw if closed ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27989#discussion_r2822195367 PR Review Comment: https://git.openjdk.org/jdk/pull/27989#discussion_r2822157777 PR Review Comment: https://git.openjdk.org/jdk/pull/27989#discussion_r2822160222 PR Review Comment: https://git.openjdk.org/jdk/pull/27989#discussion_r2822183050 PR Review Comment: https://git.openjdk.org/jdk/pull/27989#discussion_r2822204842 PR Review Comment: https://git.openjdk.org/jdk/pull/27989#discussion_r2822290905 PR Review Comment: https://git.openjdk.org/jdk/pull/27989#discussion_r2822276502 PR Review Comment: https://git.openjdk.org/jdk/pull/27989#discussion_r2822277740
