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

[Daniel Fuchs]

On Mon, 5 Oct 2020 08:52:33 GMT, Chris Hegarty <chegar at 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