On Tue, 14 Jan 2025 14:48:52 GMT, Daniel Fuchs <[email protected]> wrote:
>> Volkan Yazıcı has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add missing `@since` tags
>
> 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.
67d4b878047d9dc762585069fe32156cac54c698 removes `discardExcess` and
b95ff8d81751bc09e6c7ba219bbf8bd949f65ba8 improves the JavaDoc.
> 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.
@dfuch, implemented requested changes. Would you mind reviewing the newly added
`stateRef` and its usage, please?
> 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.
Added state and relevant checks in a4fd1bab9cb50eca5f2040ca5e27798f19d5abfa.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1915525560
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1915526532
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1915527051
