Fix that the list Switch operator drops completion and throws errors - #1139
Merged
dwcullop merged 4 commits intoAug 1, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes DynamicData’s list Switch operator to correctly propagate terminal events (completion + errors) so it matches Observable.Switch semantics, addressing #1138.
Changes:
- Added a terminal signaling path in
List/Internal/Switchto stop the output sequence when the active inner and outer sequences terminate, and to forward errors/completion. - Added list-level regression tests for outer/inner error propagation and completion semantics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/DynamicData/List/Internal/Switch.cs |
Updates list Switch implementation to carry terminal events across the private SourceList<T> relay. |
src/DynamicData.Tests/List/SwitchFixture.cs |
Adds regression tests validating error and completion behavior for list Switch. |
This was referenced Jul 26, 2026
dwcullop
force-pushed
the
bugfix/list_switch_terminal_events
branch
from
July 27, 2026 15:56
6b8127f to
3663816
Compare
JakenVeina
requested changes
Jul 29, 2026
JakenVeina
left a comment
Collaborator
There was a problem hiding this comment.
Same comments as the cache version: .SubscribeSafe() and Disposable.Create().
JakenVeina
force-pushed
the
bugfix/list_switch_terminal_events
branch
4 times, most recently
from
July 31, 2026 04:33
8d636b4 to
5b9500b
Compare
… without taking a lock Switch relayed changes through a private SourceList and subscribed the observer to that, so the terminal event of the source had nowhere to go. Completion was dropped outright, and an error was rethrown out of the subscription rather than delivered as OnError. Switching is now explicit rather than delegated to Observable.Switch, which holds its gate for the whole of the downstream OnNext call. A pipeline that crosses into another collection runs that work under the gate, and a producer on another thread blocks behind it, which is the cross cache deadlock shape the delivery queue exists to avoid. Measured against a subscriber that blocks inside OnNext, writing to the source from another thread took 748ms through Observable.Switch and 1ms through the queue, which enqueues and returns. A SerialDisposable holds the current inner subscription, and each one carries an identity, so a superseded source still delivering concurrently is dropped rather than applied on top of the state its replacement has already established. All state changes and all delivery happen through the queue lock, which is released before anything is handed downstream. The result completes once the sources and the current inner have both completed, and fails as soon as either does.
The list counterpart of the same gap in the cache operator: OnNext and OnCompleted check the captured id against the active source, but OnError went straight to the queue, so a late failure from a source already switched away from could terminate the output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Same correction as the cache side. The Subject version was disposed by SerialDisposable before the failure was raised, so Rx suppressed it and the test passed with or without the guard. RawAnonymousObservable delivers the failure after the switch, which is the in-flight case the guard is for. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Matches what the cache version got in reactivemarbles#1137. SubscribeSafe on both the outer and inner subscriptions, and an explicit teardown instead of CompositeDisposable, which does not specify a disposal order. The queue has to go first so any delivery in flight finishes before the subscriptions feeding it are torn down. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9582bb33-26d3-4aa5-8dd7-57dc55304680
JakenVeina
force-pushed
the
bugfix/list_switch_terminal_events
branch
from
July 31, 2026 06:01
5b9500b to
e62ba6b
Compare
Member
Author
Fixed. |
JakenVeina
approved these changes
Aug 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1138.
The list
Switchoperator dropped completion, threw errors instead of delivering them, and routed delivery through a lock.Dropped completion, thrown errors
It relayed changes through a private
SourceListand subscribed the observer to that:A relay only ends when it is disposed, so the terminal event of the source had nowhere to go. Completion was dropped outright. An error was worse than dropped:
PopulateIntosubscribes without an error handler, so Rx rethrew it out of the subscription, on whatever thread happened to be writing to the source rather than at the subscription.That is a step worse than the cache operator (#1137), which at least carried errors across by hand.
The lock
The obvious fix is to let
Observable.Switchsit between the sources and the observer and propagate terminal events on its own. That is wrong here, and it took a measurement to see why.Observable.Switchholds its gate for the whole of the downstreamOnNextcall. A pipeline that crosses into another collection runs that work under the gate, and a producer on another thread blocks behind it. That is precisely the cross cache deadlock shape the delivery queue exists to avoid, and the operator onmainavoids it today by keeping the observer on the far side of the relay list.Measured with a subscriber that blocks inside
OnNext, writing to the source from another thread:mainObservable.SwitchSo the operator is now written by hand. A
SerialDisposableholds the current inner subscription, and every subscription carries an identity, so a superseded source still delivering concurrently is dropped rather than applied on top of the state its replacement has already established. All state changes and all delivery go through the queue, which releases its lock before handing anything downstream, so a second producer enqueues and returns instead of waiting.The
Clearthat removes the previous source's contribution is enqueued under the same lock acquisition that takes the new identity, which keeps it ordered against the changes that follow it.Terminal semantics
Completes once the sources and the current inner have both completed. Fails as soon as either does. That matches
Observable.Switch, which is what callers reasonably expect from something called Switch.Tests
List/SwitchFixture:CompletesWhenSourcesAndInnerCompleteDoesNotCompleteWhileInnerIsStillRunningDoesNotCompleteWhenOnlyASupersededInnerCompletesCompletesWhenSourcesAndInnerCompleteSynchronouslyDeliversChangesEmittedBeforeSynchronousCompletionPropagatesInnerErrorsRaisedSynchronouslyIgnoresChangesFromASupersededSourceDoesNotHoldALockWhileDeliveringDownstreamThat last one blocks a subscriber inside
OnNextand asserts that writing to the source from another thread still returns. It fails against anObservable.Switchimplementation and passes against this one, so the lock behaviour cannot quietly come back.Note the list aggregator exposes
Exceptionwhere the cache one exposesError, which is why the error assertions read differently from #1137.Notes
Rebased onto current
mainand squashed. OnlyList/Internal/Switch.csand its fixture are touched.The cache
Switchhas the same defects and is fixed the same way in #1137. The two are independent and can land in either order.Behavioural change to a public operator: sequences that previously never terminated now terminate when their sources do, and an error that previously escaped as a thrown exception now arrives as
OnError.mainis on10.0-preview.