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

[Daniel Fuchs]

On Fri, 18 Sep 2020 11:32:26 GMT, Chris Hegarty <chegar at openjdk.org> wrote:

>> Daniel Fuchs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev
>> excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since
>> the last revision:
>>  - Merge remote-tracking branch 'origin/master' into concat-bs-8252374
>>  - Merge remote-tracking branch 'origin/master' into concat-bs-8252374
>>  - Merge remote-tracking branch 'origin/master' into concat-bs-8252374
>>  - 8252374: Add a new factory method to concatenate a sequence of BodyPublisher instances into a single publisher.
>
> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 661:
> 
>> 659:
>> 660:         /**
>> 661:          * Creates a {@code BodyPublisher} that publishes a request
> 
> Trivially, should we replace "Creates" with "Returns" to allow for the possibility of, say, a singleton empty publisher
> (rather than having to "create" a new empty each time)

Good point!

> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 668:
> 
>> 666:          * is returned. Otherwise each publisher is lazily subscribed to in turn,
>> 667:          * until all the body bytes are published, an error occurs, or the
>> 668:          * returned publisher's subscription is cancelled.
> 
> We should probably say what the publishers `contentLength` will return.

OK.

> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 680:
> 
>> 678:          * cancelled}, or an error occurs while publishing the bytes, not all
>> 679:          * publishers in the sequence may be subscribed to.
>> 680:          * A publisher may be subscribed several times in sequence if the
> 
> I think that this sentence can be moved out of the implNote and into normative text

OK.

> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 688:
> 
>> 686:          * by each publisher in the sequence.
>> 687:          */
>> 688:         public static BodyPublisher concat(BodyPublisher... publishers) {
> 
> It's a choice whether to allow an empty publishers array, or an array of just 1, or to disallow (throw
> IllegalArgumentException), but that will depend on expected usage.

Yes - returning `noBody()` seems more natural - since varargs allows you to call `BodyPublishers.concat()` with no
parameters. I toyed with the idea of defining the method as `BodyPublisher concat(BodyPublisher first, BodyPublisher...
rest)` but that's very cumbersome for the case where you already have a `List<BodyPublishers>` or a `BodyPublisher[]`
array.

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

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