Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 58 additions & 26 deletions Sources/CoalesceLatestPublisher.swift
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,12 @@ private final class CoalesceLatestInner<Downstream: Subscriber, Context: Schedul
private var hasReceivedReplay = false
private var windowStart: Context.SchedulerTimeType?
private var pendingValue: Input?
private var readyValue: Input?
private var pendingCompletion: Subscribers.Completion<Never>?
private var downstreamDemand: Subscribers.Demand = .none
private var isDelivering = false
private var trailingScheduled = false
private var isCancelled = false
private var demand: Subscribers.Demand = .none
/// Latest value that reached an emission point while downstream demand
/// was zero; conflated (newer emissions overwrite it) and delivered from
/// `request(_:)` when demand arrives.
private var undeliveredValue: Input?

init(downstream: Downstream, interval: Context.SchedulerTimeType.Stride, scheduler: Context) {
self.downstream = downstream
Expand All @@ -95,14 +94,18 @@ private final class CoalesceLatestInner<Downstream: Subscriber, Context: Schedul
func receive(subscription: Subscription) {
upstreamSubscription = subscription
downstream.receive(subscription: self)
guard !isCancelled else {
subscription.cancel()
return
}
subscription.request(.unlimited)
}

func receive(_ input: Input) -> Subscribers.Demand {
guard !isCancelled else { return .none }
if !hasReceivedReplay {
hasReceivedReplay = true
deliver(input)
enqueueForDelivery(input)
return .none
}
let now = scheduler.now
Expand All @@ -116,18 +119,19 @@ private final class CoalesceLatestInner<Downstream: Subscriber, Context: Schedul
// callback cannot emit it out of order after this value.
pendingValue = nil
windowStart = now
deliver(input)
enqueueForDelivery(input)
}
return .none
}

func receive(completion: Subscribers.Completion<Never>) {
guard !isCancelled else { return }
pendingCompletion = completion
if let value = pendingValue {
pendingValue = nil
deliver(value)
enqueueForDelivery(value)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the reentrant leading value at completion

With unlimited demand, if a downstream receiving the replay reentrantly sends a leading value, then another value inside that new window, then completes (send(2); send(3); send(completion:)), this line enqueues the pending trailing value through readyValue = value while isDelivering is still true, overwriting the held leading value before the outer drain can emit it. Fresh evidence beyond the prior fixed thread is the extra in-window value before completion; the old sink behavior would emit [1, 2, 3], but this change emits [1, 3], so leading-edge semantics are still broken for completing reentrant streams.

Useful? React with 👍 / 👎.

}
downstream.receive(completion: completion)
drainReadyValue()
}

private func scheduleTrailingEmission(at deadline: Context.SchedulerTimeType) {
Expand All @@ -153,33 +157,61 @@ private final class CoalesceLatestInner<Downstream: Subscriber, Context: Schedul
}
pendingValue = nil
windowStart = scheduler.now
deliver(value)
enqueueForDelivery(value)
}

/// Hands a value to the downstream only when it has outstanding demand;
/// otherwise conflates it into the latest-value slot for `request(_:)`.
private func deliver(_ value: Input) {
guard demand > .none else {
undeliveredValue = value
return
func request(_ demand: Subscribers.Demand) {
guard !isCancelled, demand > .none else { return }
downstreamDemand += demand
drainReadyValue()
}

private func enqueueForDelivery(_ value: Input) {
readyValue = value

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Don't overwrite demanded reentrant emissions

When downstream still has outstanding demand but enqueueForDelivery is re-entered before the current downstream.receive returns, this single slot replaces an earlier ready emission. For example, with .coalesceLatest(for: .zero, ...) (or a callback that stalls past the interval), an unlimited-demand sink that sends 2 and then 3 from its receive(1) only receives [1, 3]; neither value is inside a coalescing window and the old unlimited-demand behavior delivered both. Keep a FIFO of ready emission points, or otherwise avoid conflating values while demand remains.

Useful? React with 👍 / 👎.

drainReadyValue()
}

private func drainReadyValue() {
guard !isDelivering else { return }
isDelivering = true
defer {
isDelivering = false
finishPendingCompletionIfPossible()
}

while !isCancelled,
downstreamDemand > .none,
let value = readyValue {
readyValue = nil
downstreamDemand -= .max(1)
let additionalDemand = downstream.receive(value)
guard !isCancelled else { return }
downstreamDemand += additionalDemand
}
demand -= 1
demand += downstream.receive(value)
}

func request(_ additionalDemand: Subscribers.Demand) {
guard !isCancelled else { return }
demand += additionalDemand
guard demand > .none, let value = undeliveredValue else { return }
undeliveredValue = nil
demand -= 1
demand += downstream.receive(value)
private func finishPendingCompletionIfPossible() {
guard !isCancelled,
!isDelivering,
readyValue == nil,
let completion = pendingCompletion
else { return }

pendingCompletion = nil
// Combine completion is not demand-gated, but it cannot overtake a
// buffered value. Once demand drains that value, completion follows.
downstreamDemand = .none
isCancelled = true
downstream.receive(completion: completion)
upstreamSubscription = nil
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +193 to 207

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Silent value drop when demand is zero at completion

finishPendingCompletionIfPossible unconditionally clears readyValue before forwarding completion. In the unlimited-demand path this is always nil by the time the drain exits, so there is no loss. For a demand-limited subscriber, however, if the upstream completes while downstreamDemand == .none and a value is sitting in readyValue (e.g. the leading replay was delivered but the subscriber has not yet called request again), that buffered value is silently dropped — the subscriber receives completion without ever seeing the latest coalesced value.

For the Publisher.values / AsyncSequence bridge motivating this fix, next() always requests .max(1) before suspending, so demand is always in-flight and this case cannot arise in practice. Worth noting here for future demand-limited consumers that are slow to re-request: the contract is "latest value only when demand exists", and a completion that races a pending request wins.


func cancel() {
isCancelled = true
pendingValue = nil
undeliveredValue = nil
readyValue = nil
pendingCompletion = nil
downstreamDemand = .none
upstreamSubscription?.cancel()
upstreamSubscription = nil
}
Expand Down
84 changes: 84 additions & 0 deletions cmuxTests/WorkspaceSidebarObservationTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,57 @@ struct WorkspaceSidebarObservationTests {
)
}

@Test func coalesceLatestDrainsReentrantValueBeforeCompletionWithUnlimitedDemand() {
let scheduler = VirtualCoalesceScheduler()
let subject = PassthroughSubject<Int, Never>()
let subscriber = DemandControlledSubscriber<Int>()
subject
.coalesceLatest(for: .milliseconds(50), scheduler: scheduler)
.subscribe(subscriber)
defer { subscriber.cancel() }

subscriber.onValue = { value in
if value == 1 {
subject.send(2)
subject.send(completion: .finished)
}
}
subscriber.request(.unlimited)
subject.send(1)

#expect(
subscriber.received == [1, 2],
"A reentrant value that arrived before completion must drain while unlimited demand remains."
)
#expect(subscriber.completionCount == 1)
#expect(subscriber.receivedValuesAtCompletion == [[1, 2]])
}

@Test func coalesceLatestDeliversBufferedValueBeforeCompletionWhenDemandResumes() {
let scheduler = VirtualCoalesceScheduler()
let subject = PassthroughSubject<Int, Never>()
let subscriber = DemandControlledSubscriber<Int>()
subject
.coalesceLatest(for: .milliseconds(50), scheduler: scheduler)
.subscribe(subscriber)
defer { subscriber.cancel() }

subject.send(1)
subject.send(completion: .finished)

#expect(subscriber.received.isEmpty)
#expect(
subscriber.completionCount == 0,
"Completion must wait while the final value is buffered without demand."
)

subscriber.request(.max(1))

#expect(subscriber.received == [1])
#expect(subscriber.completionCount == 1)
#expect(subscriber.receivedValuesAtCompletion == [[1]])
}

@Test func sidebarObservationPublisherIgnoresRemoteHeartbeatOnlyChanges() {
let workspace = Workspace()

Expand Down Expand Up @@ -378,6 +429,39 @@ private final class ObservationChangeFlag: @unchecked Sendable {
}
}

private final class DemandControlledSubscriber<Input>: Subscriber {
typealias Failure = Never

private var subscription: Subscription?
private(set) var received: [Input] = []
private(set) var completionCount = 0
private(set) var receivedValuesAtCompletion: [[Input]] = []
var onValue: ((Input) -> Void)?

func receive(subscription: Subscription) {
self.subscription = subscription
}

func receive(_ input: Input) -> Subscribers.Demand {
received.append(input)
onValue?(input)
return .none
}

func receive(completion: Subscribers.Completion<Never>) {
completionCount += 1
receivedValuesAtCompletion.append(received)
}

func request(_ demand: Subscribers.Demand) {
subscription?.request(demand)
}

func cancel() {
subscription?.cancel()
subscription = nil
}
}
// Deterministic Combine scheduler for coalesceLatest tests: `now` only moves
// via advance(by:), and scheduled actions run only when runScheduledActions()
// is called, so overdue-timer interleavings are exact instead of wall-clock.
Expand Down