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

[Volkan Yazıcı]

On Wed, 15 Jan 2025 14:07:04 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Volkan Yazıcı has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Remove concurrency measures (methods are accessed serially due to the Reactive Streams spec)
>
> src/java.net.http/share/classes/java/net/http/HttpResponse.java line 772:
> 
>> 770:             Objects.requireNonNull(downstreamHandler, "downstreamHandler");
>> 771:             if (capacity < 0) {
>> 772:                 throw new IllegalArgumentException("was expecting \"capacity >= 0\", found: " + capacity);
> 
> Nit - in some other areas, we use messages of the form `("capacity must not be negative: " + capacity)`. The current proposed message is more commonly used in test case exceptions, so it feels a bit odd to be seeing it in this form.

Fixed in 23f79a20476f784058801454a28d37f37df92982.

> src/java.net.http/share/classes/jdk/internal/net/http/LimitingSubscriber.java line 58:
> 
>> 56:     private interface State {
>> 57: 
>> 58:         enum Terminated implements State { INSTANCE }
> 
> This was a bit confusing to read. I find the following to be a bit more easier to read:
> 
> 
> 	private interface State {
> 		State TERMINATED = new State() {};
> 
> 		record Subscribed(Object subscription) implements State {}
> 	}
> 
> The call sites would then just use `State.TERMINATED`. 
> 
> However, if you prefer it in its current form, it's fine with me.

Fixed in 23f79a20476f784058801454a28d37f37df92982.

> src/java.net.http/share/classes/jdk/internal/net/http/LimitingSubscriber.java line 107:
> 
>> 105: 
>> 106:         // Otherwise, trigger failure
>> 107:         else {
> 
> Nit - this `else` feels a bit "far" away from the corresponding `if`. Perhaps you could consider changing it to something like:
> 
> if {
>     ...
> } else { // Otherwise, trigger failure
>     ...
> }
> 
> or even,
> 
> 
> if {
>     ...
> } else { 
>     // Otherwise, trigger failure
>     ...
> }

Replaced as suggested in 23f79a20476f784058801454a28d37f37df92982.

IMHO, the suggested style causes misalignment on the comments associated with `if` and `else`. That is, the comments of the `else` block gets indented deeper, even though it is semantically at the same level with the `if` comment:

![image](https://github.com/user-attachments/assets/da465a30-284a-40f2-948a-fa5e1f79c53d)

> src/java.net.http/share/classes/jdk/internal/net/http/LimitingSubscriber.java line 110:
> 
>> 108:             state = State.Terminated.INSTANCE;
>> 109:             downstreamSubscriber.onError(new IOException(
>> 110:                     "the maximum number of bytes that are allowed to be consumed is exceeded"));
> 
> Perhaps we should consider including the capacity in the message? Something like: `new IOException("body exceeds capacity: " + capacity)`?

Fixed in 144ce26a2c0889b9b8b31d2a9ae67c3778f7715b.

> src/java.net.http/share/classes/jdk/internal/net/http/LimitingSubscriber.java line 117:
> 
>> 115: 
>> 116:     private boolean allocateLength(List<ByteBuffer> buffers) {
>> 117:         long bufferLength = buffers.stream().mapToLong(Buffer::remaining).sum();
> 
> More of a FYI than asking you to replace this - we have a `jdk.internal.net.http.common.Utils.remaining(List<ByteBuffer>)` which returns the same.

Fixed in 23f79a20476f784058801454a28d37f37df92982.

> test/jdk/java/net/httpclient/HttpResponseLimitingTest.java line 76:
> 
>> 74:     private static final Charset CHARSET = StandardCharsets.UTF_8;
>> 75: 
>> 76:     private static final HttpServer SERVER = new HttpServer();
> 
> Hello Volkan, is the use of a custom server implementation backed by reading/writing raw bytes on a `ServerSocket` intentional in this test? The test library in `/test/jdk/java/net/httpclient/lib` already has necessary test server implementations which we use in `HttpClient` tests to be able to run those tests for different protocol versions of HTTP (`test/jdk/java/net/httpclient/HttpClientLocalAddrTest.java` is one such arbitrary test).

In 64190c5765dc486eafd1d37b9312b1256943ed8d, used `jdk.httpclient.test.lib.common.HttpServerAdapters` (as in `HttpClientLocalAddrTest`) with combinations of HTTP version `1.1` and `2.0`, and SSL enabled and disabled.

> test/jdk/java/net/httpclient/HttpResponseLimitingTest.java line 183:
> 
>> 181: 
>> 182:         private static final byte[] RESPONSE = (
>> 183:                 "HTTP/1.2 200 OK\r\n" +
> 
> Did you mean `HTTP/1.1`?

Fixed in 23f79a20476f784058801454a28d37f37df92982, replaced with `HttpServerAdapters` in 64190c5765dc486eafd1d37b9312b1256943ed8d.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1918023189
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1918025080
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1918046260
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1918060035
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1918048203
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1918056951
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1918059061