On Wed, 22 Jan 2025 14:54:08 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Volkan Yazici has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Verify `limiting()` doesn't affect header parsing
>
> test/jdk/java/net/httpclient/HttpResponseLimitingTest.java line 90:
>
>> 88: * A header value larger than {@link #RESPONSE_BODY} to verify that
>> {@code limiting()} doesn't affect header parsing.
>> 89: */
>> 90: private static final String RESPONSE_HEADER_VALUE =
>> "!".repeat(RESPONSE_BODY.length);
>
> Did you mean this to be:
>
> "!".repeat(RESPONSE_BODY.length + 1);
>
> `"x".repeat(1)` will generate a String of length 1.
>
> In the context of what we are doing in this test, it won't matter though.
No, it wasn't an oversight, since `data = header name + header value` which is
larger than `RESPONSE_BODY`. Nevertheless, apparently it is not clear to the
reader. b75938265560ae7c62e88e60a3895e37889316ad replaces it with
`RESPONSE_BODY.length + 1` as you suggested.
> test/jdk/java/net/httpclient/HttpResponseLimitingTest.java line 179:
>
>> 177: for (var pair : new ServerClientPair[]{HTTP1, HTTPS1, HTTP2,
>> HTTPS2}) {
>> 178: pair.client.close();
>> 179: pair.server.stop();
>
> I think we should consider catching any exception that might happen here when
> closing each client/server and log a message and continue closing the next
> pair, especially since this is not a `othervm` test - we wouldn't want the
> sockets to continue to be open after this test completes.
Implemented in 085e48e8b54475e630674686a2c884b241565b09.
> test/jdk/java/net/httpclient/HttpResponseLimitingTest.java line 198:
>
>> 196: try (InputStream responseBodyStream = response.body()) {
>> 197: byte[] responseBodyBuffer = new byte[RESPONSE_BODY.length];
>> 198: int responseBodyBufferLength =
>> responseBodyStream.read(responseBodyBuffer);
>
> I think a better way would be to read the `responseBodyStream` till EOF and
> keep accumulating the read data into the byte array. After reaching EOF, we
> then compare the accumulated byte array for the expected size and content. In
> the current form, if there's a short read (i.e. this `read()` call reads
> lesser bytes than the `responseBodyBuffer.length`, even if there's additional
> data available), then this test will fail.
Implemented in 21ae1079aaa3730317ef1bb11277e929dbd6174d.
> test/jdk/java/net/httpclient/HttpResponseLimitingTest.java line 230:
>
>> 228: var exception = assertThrows(
>> 229: IOException.class,
>> 230: () -> responseBodyStream.read(new
>> byte[RESPONSE_BODY.length]));
>
> Here too, I would suggest reading till EOF and expecting IOException at some
> point while doing so, for the same reason I noted in a review comment above.
Implemented in 21ae1079aaa3730317ef1bb11277e929dbd6174d.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1925565498
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1925565211
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1925565033
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1925564829
