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

[robert engels]

On Thu, 25 Apr 2024 12:28:25 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> robert engels has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fix broken test cases
>
> test/jdk/com/sun/net/httpserver/bugs/TcpNoDelayNotRequired.java line 60:
> 
>> 58:         InetAddress loopback = InetAddress.getLoopbackAddress();
>> 59:         InetSocketAddress addr = new InetSocketAddress (loopback, 0);
>> 60:         HttpServer server = HttpServer.create (addr, 0);
> 
> Given the change we are doing, I think it would be good to even include a test (within this same test class) which tests the `HttpsServer`.

done

> test/jdk/com/sun/net/httpserver/bugs/TcpNoDelayNotRequired.java line 79:
> 
>> 77:             var uri = URIBuilder.newBuilder().scheme("http").loopback().port(server.getAddress().getPort()).path("/chunked").build();
>> 78:             var response = client.send(HttpRequest.newBuilder(uri).build(), HttpResponse.BodyHandlers.ofString());
>> 79:             if(!response.body().equals("hello")) throw new IllegalStateException("incorrect body");
> 
> Same applies here, it would be better to include the failing response body in the exception message.

done

> 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.

we can fix the test cases with constants after the api change

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1579699401
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1579698757
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1579700904