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

[robert engels]

On Thu, 25 Apr 2024 12:11:42 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 29:
> 
>> 27:  * @summary  tcp no delay not required for small payloads
>> 28:  * @library /test/lib
>> 29:  * @run main/othervm/timeout=5 -Dsun.net.httpserver.nodelay=false  TcpNoDelayNotRequired
> 
> I think we should remove the `timeout=5` here. In the past we have seen that such timeouts have contributed to intermittent failures in the CI. jtreg itself has a (sufficiently large) timeout and if the test doesn't complete by then, then jtreg errors that test as timed out. Relying on jtreg timeout handling will avoid guessing the right timeout value here in the test definition.

The timeout is the test. These changes fix the performance by an order of 100x. The timeout is the only way I know to test this.

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

Isn’t consistency in the code base more important. I like the change but it seems there should be a single PR that changes all of the test cases to this format.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1579405001
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1579409863