On Wed, 15 Jan 2025 14:07:04 GMT, Jaikiran Pai <j...@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:
> 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
