On Wed, 22 Jan 2025 09:45:16 GMT, Volkan Yazici <vyaz...@openjdk.org> wrote:
>> Adds `limiting()` factory methods to
>> `HttpResponse.Body{Handlers,Subscribers}` to handle excessive server input
>> in `HttpClient`. I would appreciate your input whether `discardExcess`
>> should be kept or dropped. I plan to file a CSR once there is an agreement
>> on the PR.
>
> 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 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.
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.
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1925427812
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1925436156
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1925444519
