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

Volkan Yazici vyazici at openjdk.org
Tue Jan 21 20:04:25 UTC 2025


On Tue, 21 Jan 2025 11:01:00 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Volkan Yazici has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix `HttpResponse` copyright year
>
> test/jdk/java/net/httpclient/HttpResponseLimitingTest.java line 84:
> 
>> 82: 
>> 83:     static Arguments[] sufficientCapacities() {
>> 84:         return capacityArgs(Long.MAX_VALUE, RESPONSE_BODY.length);
> 
> If I'm reading this (and the `insufficientCapacities()`) correctly, then it appears that we are testing the "edge" capacities. I think including at least one more arbitrary capacity value which isn't an "edge" might be good too. Something like:
> 
> final long randomCapacity = new Random(RESPONSE_BODY.length + 1, Long.MAX_VALUE);
> 
> Similar idea for `insufficientCapacities()`.

Implemented in e10771317ef57f80d25be58df77eb5c5b0083b0c.

> test/jdk/java/net/httpclient/HttpResponseLimitingTest.java line 148:
> 
>> 146: 
>> 147:             // Issue the request
>> 148:             var request = HttpRequest
> 
> Are we intentionally not setting the HTTP version (being passed to this method) on the request or the client?

I thought it wasn't necessary. Nevertheless, added in e10771317ef57f80d25be58df77eb5c5b0083b0c.

> test/jdk/java/net/httpclient/HttpResponseLimitingTest.java line 150:
> 
>> 148:             var request = HttpRequest
>> 149:                     .newBuilder(requestUri)
>> 150:                     .timeout(Duration.ofSeconds(5))
> 
> Given the context of this test class, I think configuring the request with a timeout shouldn't be necessary. In general, we rarely use timeouts (hardcoded ones specifically) in our tests because these tests run on a variety of hosts (some of which are slow or are running too many tests concurrently) and thus there's no right value for the timeout and can lead to intermittent failures.

Removed the timeouts in e10771317ef57f80d25be58df77eb5c5b0083b0c.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1924296707
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1924296492
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1924296394


More information about the net-dev mailing list