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

[Volkan Yazıcı]

On Tue, 14 Jan 2025 14:48:52 GMT, Daniel Fuchs <dfuchs at openjdk.org> 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