On Sat, 6 Apr 2024 23:35:48 GMT, robert engels <d...@openjdk.org> wrote:
> fix bug JDK-B6968351 by avoiding flush after response headers
This is not as complex as I expected it to be. Which is a good thing!
Some tests are failing with this change. See:
test/jdk/sun/net/www/http/KeepAliveCache/B8293562.java
test/jdk/java/net/Authenticator/B4769350.java
These tests don't close the exchange or the output stream after sending the
response, and the headers are never flushed. Users might have similar code, so
I think a release note might be needed.
src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 39:
> 37: import javax.net.ssl.SSLContext;
> 38: import javax.net.ssl.SSLEngine;
> 39: import java.io.*;
Please do not use wildcard imports in this file.
test/jdk/com/sun/net/httpserver/bugs/B6968351.java line 30:
> 28: * @library /test/lib
> 29: * @run main B6968351
> 30: * @run main/othervm -Dsun.net.httpserver.nodelay=false
> -Djdk.httpclient.HttpClient.log=all -Djava.net.preferIPv6Addresses=true
> -Djavax.net.debug=all B6968351
Do you need all these system properties here?
test/jdk/com/sun/net/httpserver/bugs/B6968351.java line 45:
> 43: import java.util.logging.*;
> 44:
> 45: public class B6968351 {
You could use a more revealing name here; the B<bug numer> aren't very useful
test/jdk/com/sun/net/httpserver/bugs/B6968351.java line 75:
> 73: long time = System.currentTimeMillis()-start;
> 74: System.out.println("time "+time);
> 75: if(time>5000) throw new IllegalStateException("took too long");
Use jtreg timeout instead, like:
@run main/timeout=5 B6968351
(the timeout value is in seconds, but it's scaled x4 by default)
this will interrupt the test if/when the timeout is hit, and will also enable
scaling the timeout on slower machines
test/jdk/com/sun/net/httpserver/bugs/B6968351.java line 90:
> 88: is.close();
> 89: rmap.add("content-type","text/plain");
> 90: t.sendResponseHeaders(200,5);
if I read the code correctly, there might be a similar delay when sending the
last chunk of a chunked response. Would you like to fix it here as well? If
not, we can file another ticket for it.
In order to send a chunked response, change the second parameter to `0` here.
test/jdk/com/sun/net/httpserver/bugs/B6968351.java line 91:
> 89: rmap.add("content-type","text/plain");
> 90: t.sendResponseHeaders(200,5);
> 91: t.getResponseBody().write("hello".getBytes());
Suggestion:
t.getResponseBody().write("hello".getBytes(StandardCharsets.ISO_8859_1));
-------------
PR Review: https://git.openjdk.org/jdk/pull/18667#pullrequestreview-2010884813
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1572046765
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1572048695
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1572121432
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1572052902
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1572123627
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1572059622
