On Mon, 5 Oct 2020 08:52:33 GMT, Chris Hegarty <che...@openjdk.org> wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Improved the API documentation of BodyPublishers::concat
>
> src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java
> line 502:
>
>> 500:
>> 501: public static BodyPublisher concat(BodyPublisher... publishers) {
>> 502: if (publishers == null || publishers.length == 0) {
>
> publishers cannot be null here - since NPE will be thrown from the API point.
> Maybe assert non-null or just remove?
OK - will remove.
> src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java
> line 569:
>
>> 567: this.bodies = new ConcurrentLinkedQueue<>(bodies);
>> 568: this.subscriber = subscriber;
>> 569: scheduler =
>> SequentialScheduler.synchronizedScheduler(this::run);
>
> It's a little unpleasant that this call to the scheduler is inside the
> constructor. I wonder if it could be located in
> the subscribe method above, just after the subscription is created? (since
> both classes are in the same nest )
Scheduler is a final variable in `AggregateSubscription`. Fixed to:
` this.scheduler = ....;`
(though the `this.` is not strictly needed in this third instantiation)
> src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java
> line 573:
>
>> 571:
>> 572: @Override
>> 573: public void request(long n) {
>
> I believe that a negative demand value should result in an
> IllegalArgumentException
That will be thrown by Demand::increase below.
-------------
PR: https://git.openjdk.java.net/jdk/pull/57
