On Tue, 14 Jan 2025 20:04:40 GMT, Volkan Yazıcı <d...@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 Yazıcı has updated the pull request incrementally with four additional
> commits since the last revision:
>
> - Make `LimitingSubscriber` stateful
> - Improve Javadoc
> - Replace `IllegalStateException` with an `IOException`
> - Remove `discardExcess` argument
Thanks for taking into account the feedback! Some more comments.
src/java.net.http/share/classes/java/net/http/HttpResponse.java line 1400:
> 1398: */
> 1399: public static <T> BodySubscriber<T> limiting(BodySubscriber<T>
> downstreamSubscriber, long capacity) {
> 1400: Objects.requireNonNull(downstreamSubscriber,
> "downstreamSubscriber");
Strange to check for null but not for capacity < 0 ?
src/java.net.http/share/classes/jdk/internal/net/http/LimitingSubscriber.java
line 55:
> 53: private final AtomicReference<State> stateRef = new
> AtomicReference<>();
> 54:
> 55: private long length;
should be volatile or AtomicLong
src/java.net.http/share/classes/jdk/internal/net/http/LimitingSubscriber.java
line 107:
> 105:
> 106: // Otherwise, trigger failure
> 107: else if (stateRef.compareAndSet(subscribed,
> State.Terminated.INSTANCE)) {
I would prefer to keep the regular style:
if (condition) {
// do something
} else {
// do something else
}
-------------
Changes requested by dfuchs (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/23096#pullrequestreview-2550965206
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1915575983
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1915568330
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1915566451
