On Tue, 14 Jan 2025 09:24:12 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 one additional
> commit since the last revision:
>
> Add missing `@since` tags
Changes requested by dfuchs (Reviewer).
src/java.net.http/share/classes/java/net/http/HttpResponse.java line 758:
> 756: * @param downstreamHandler the downstream handler to pass
> received data to
> 757: * @param capacity the maximum number of bytes that are allowed
> 758: * @param discardExcess if {@code true}, excessive input will be
> discarded; otherwise, it will throw an exception
I would suggest to simplify the code and drop `discardExcess` for the moment.
We could add an overload later if needed. The body of the method API
specification could then read something like:
/**
* {@return a {@code BodyHandler} limiting the number of bytes
* consumed and passed to the given downstream {@code BodyHandler}}
* <p>
* If the number of bytes received exceeds the maximum number
* of bytes desired as indicated by the given {@code capacity},
* {@link BodySubscriber#onError(Throwable) onError} is called on
* the downstream {@code BodySubscriber} with an {@link
IOException}
* indicating that the capacity is exceeded; the
* upstream subscription is cancelled.
src/java.net.http/share/classes/jdk/internal/net/http/LimitingSubscriber.java
line 99:
> 97: if (lengthAllocated) {
> 98: downstreamSubscriber.onNext(buffers);
> 99: }
You would need to maintain a state of what methods have been called on the the
downstream subscriber. If onComplete/onError have been already called, onNext
should no longer be called, and onComplete/onError should not be called twice.
It's possible that onNext will still be invoked after cancelling the
subscription. This is to be expected - delivery will eventually stop, but at
this point any bytes passed to onNext should simply be discarded.
I believe it would be better in a first time to drop the `discardExcess`
feature and simply call `onError` if the capacity is exceeded. That should
simplify this code significantly.
src/java.net.http/share/classes/jdk/internal/net/http/LimitingSubscriber.java
line 162:
> 160: public void onComplete() {
> 161: downstreamSubscriber.onComplete();
> 162: }
As hinted earlier you should not call onError again if you already called
onError. The publisher should not call onError twice, but if you have already
called onError from onNext on the downstream subscriber, you should not call it
again (and neither should you call onComplete), if the upstream publisher calls
onError/onComplete on the limiting subscriber after this point.
-------------
PR Review: https://git.openjdk.org/jdk/pull/23096#pullrequestreview-2549947373
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1914956881
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1914977149
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1914989578
