On Tue, 23 Apr 2024 19:10:48 GMT, robert engels <d...@openjdk.org> wrote:
>> fix bug JDK-B6968351 by avoiding flush after response headers
>
> robert engels has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix broken test cases
I think it might be better to move out the new test from
`test/jdk/com/sun/net/httpserver/bugs/TcpNoDelayNotRequired.java` to
`test/jdk/com/sun/net/httpserver/TcpNoDelayNotRequired.java`. We haven't been
using the `bugs` directory (or the bug number as the file name) for a while now.
test/jdk/com/sun/net/httpserver/bugs/TcpNoDelayNotRequired.java line 99:
> 97: t.sendResponseHeaders(200,5);
> 98:
> t.getResponseBody().write("hello".getBytes(StandardCharsets.ISO_8859_1));
> 99: t.getResponseBody().close();
I don't have a strong preference, but would you be open to updating this (and
the ChunkedHandler) to:
try (InputStream is = t.getRequestBody()) {
is.readAllBytes();
}
Headers reqHeaders = t.getRequestHeaders();
Headers respHeaders = t.getResponseHeaders();
respHeaders.add("content-type", "text/plain");
t.sendResponseHeaders(200,5);
try (OutputStream os = t.getResponseBody()) {
os.write("hello".getBytes(StandardCharsets.ISO_8859_1));
}
(I haven't compiled it or done any testing with the proposed snippet)
test/jdk/com/sun/net/httpserver/bugs/TcpNoDelayNotRequired.java line 112:
> 110: is.close();
> 111: rmap.add("content-type","text/plain");
> 112: t.sendResponseHeaders(200,0);
The value 0 and -1 have been a constant confusion in our tests when calling
`sendResponseHeaders`. Can you update this line to:
t.sendResponseHeaders(200, 0 /* chunked */);
so that it's immediately obvious what the intent here is.
test/jdk/java/net/Authenticator/B4769350.java line 358:
> 356: {
> 357: exchange.getResponseHeaders().add("Proxy-Authenticate",
> reply);
> 358: exchange.sendResponseHeaders(407, -1);
Similarly here:
exchange.sendResponseHeaders(407, -1 /* no response body */);
-------------
PR Comment: https://git.openjdk.org/jdk/pull/18667#issuecomment-2077092241
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1579399205
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1579400959
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1579402251
