On Thu, 25 Apr 2024 12:11:42 GMT, Jaikiran Pai <[email protected]> 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
