On Tue, 21 Jan 2025 18:33:50 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> There's a subtle race condition in AggregatePublisher.AggregateSubscription. >> The AggregatePublisher will subscribe to downstream publishers in turn, >> subscribing to the next publisher when the previous publisher onComplete() >> has been called. >> >> The request() method passed to the upstream subscriber detects that all >> subscribers have completed if the current downstream publisher is null and >> the queue of publisher yet to subscribe to is empty. >> >> The event loop is responsible for subscribing to the next publisher, and >> does so by polling the queue and assigning the returned value to >> this.publisher, and then subscribe to that publisher. However, when >> subscribing to the last publisher in the queue, there is a small time window >> where the queue can be observed to be empty, before this.publisher is >> assigned. >> >> The fix adds synchronization to make sure a consistent state is observed >> when it matters. Typically - setting `this.publisher` or taking a snapshot >> of a publisher and its subscription, should be done within synchronized. > > 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 five additional > commits since the last revision: > > - Merge branch 'master' into AggregatePublisher-8348108 > - Fields could be final in test class > - Review feedback > - Merge branch 'master' into AggregatePublisher-8348108 > - 8348108: Race condition in AggregatePublisher.AggregateSubscription The change looks reasonable to me. src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java line 575: > 573: subscription.cancel(); > 574: } > 575: // This nethod is called when cancel is true, so Typo - should have been "method". ------------- Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23204#pullrequestreview-2567524295 PR Review Comment: https://git.openjdk.org/jdk/pull/23204#discussion_r1925505924
