RFR: 8328919: Add BodyHandlers / BodySubscribers methods to handle excessive server input [v13]

[Volkan Yazici]

On Wed, 22 Jan 2025 14:54:08 GMT, Jaikiran Pai <jpai at 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