RFR: 6968351: httpserver clashes with delayed TCP ACKs for low Content-Length

[Daniel Jeliński]

On Sat, 6 Apr 2024 23:35:48 GMT, robert engels <duke at 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