On Fri, 18 Sep 2020 16:41:43 GMT, Daniel Fuchs <dfu...@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
