RFR: 8252374: Add a new factory method to concatenate a sequence of BodyPublisher instances into a single publisher. [v6]

[Chris Hegarty]

On Fri, 18 Sep 2020 16:41:43 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> Continuing the review with a PR...
>> 
>> 8252374: Add a new factory method to concatenate a sequence
>>          of BodyPublisher instances into a single publisher.
>> https://bugs.openjdk.java.net/browse/JDK-8252374
>> 
>> 
>> Draft CSR:
>> https://bugs.openjdk.java.net/browse/JDK-8252382
>
> Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improved the API documentation of BodyPublishers::concat

Changes requested by chegar (Reviewer).

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

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?

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 )

-------------

PR: https://git.openjdk.java.net/jdk/pull/57