From 50ad68bbd4aa74245d8a32d48d706171cfdcdf7a Mon Sep 17 00:00:00 2001 From: Darrin Cullop Date: Tue, 26 May 2026 17:20:49 -0700 Subject: [PATCH 01/24] Eliminate Rx Merge gate in queue-serialized operators #1079 moved cross-cache operators from Synchronize(lock) to SynchronizeSafe, which routes deliveries through a SharedDeliveryQueue that releases the lock before invoking downstream observers. The intent was to make the lock no longer held across cross-cache calls, so two operators on a bidirectional pipeline could not form an ABBA cycle. Six operators (Page, Virtualise, AutoRefresh, Sort, GroupOnImmutable, and QueryWhenChanged) routed every input through the queue but then combined the inputs with Observable.Merge before delivery. Rx's Merge installs its own private gate and holds it for the full duration of every downstream OnNext. When downstream delivery walks into another cache's writer lock, the two Merge gates on the two operators reconstruct the ABBA cycle that the queue- drain design was supposed to eliminate. DeadlockTortureTest.Page_DoesNotDeadlock caught this for Page; the other five had the same latent bug. This adds IObservable.UnsynchronizedMerge, a drop-in alternative to Observable.Merge that performs no synchronization of its own. It is safe to use only when every input is already serialized (in this library, by routing through the same SharedDeliveryQueue). All six operators now use it. Sort's three-source case becomes a single UnsynchronizedMerge call instead of nested .Merge().Merge(), removing one of the two gates that the chained form created. FullJoin uses the same Merge syntax but its two inputs come from independently materialized AsObservableCache().Connect() streams that share no queue. The Merge gate is the only thing serializing them; this PR leaves FullJoin alone. DeadlockTortureTest grows three new cases (GroupWithImmutableState, QueryWhenChanged, and Virtualise added to the stacked + multi-pair scenarios) so a future regression in any of the six operators is caught by the existing torture fixture. Verified: 14/14 DeadlockTortureTest pass at MaxParallelThreads=16 across 10 iterations; 422/422 Sort/Virtualise/Page/AutoRefresh/Group/QueryWhenChanged unit tests pass; full Cache + List suite passes (2321 passed, 1 skipped). --- .../Cache/DeadlockTortureTest.cs | 38 ++++++++++- src/DynamicData/Cache/Internal/AutoRefresh.cs | 2 +- .../Cache/Internal/GroupOnImmutable.cs | 2 +- src/DynamicData/Cache/Internal/Page.cs | 2 +- .../Cache/Internal/QueryWhenChanged.cs | 2 +- src/DynamicData/Cache/Internal/Sort.cs | 2 +- src/DynamicData/Cache/Internal/Virtualise.cs | 2 +- .../Internal/SynchronizeSafeExtensions.cs | 66 +++++++++++++++++++ 8 files changed, 109 insertions(+), 7 deletions(-) diff --git a/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs b/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs index 4f4ddf9a..8576d34c 100644 --- a/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs +++ b/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs @@ -64,6 +64,9 @@ [Fact] public async Task AutoRefresh_DoesNotDeadlock() => [Fact] public async Task GroupOn_DoesNotDeadlock() => (await RunBidirectionalDeadlockTest(s => s.Group(p => p.Age % 3).MergeMany(g => g.Cache.Connect()))).Should().BeTrue(); + [Fact] public async Task GroupWithImmutableState_DoesNotDeadlock() => + (await RunBidirectionalDeadlockTest(s => s.GroupWithImmutableState(p => p.Age % 3).TransformMany(g => g.Items, p => p.UniqueKey))).Should().BeTrue(); + [Fact] public async Task Page_DoesNotDeadlock() { using var req = new BehaviorSubject(new PageRequest(1, 50)); @@ -76,6 +79,34 @@ [Fact] public async Task Virtualise_DoesNotDeadlock() (await RunBidirectionalDeadlockTest(s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)).Virtualise(req))).Should().BeTrue(); } + [Fact] public async Task QueryWhenChanged_DoesNotDeadlock() + { + for (var iter = 0; iter < Iterations; iter++) + { + using var sourceA = new SourceCache(p => p.UniqueKey); + using var sourceB = new SourceCache(p => p.UniqueKey); + + // QueryWhenChanged with an itemChangedTrigger exercises the Merge branch. + // A side-channel write into the other cache closes the same ABBA cycle that + // PopulateInto would close for changeset-shaped operators. + using var aToB = sourceA.Connect() + .Filter(p => p.Name.StartsWith("A")) + .QueryWhenChanged(p => p.WhenPropertyChanged(x => x.Age)) + .Subscribe(_ => sourceB.AddOrUpdate(new Person("A-marker", 0))); + using var bToA = sourceB.Connect() + .Filter(p => p.Name.StartsWith("B")) + .QueryWhenChanged(p => p.WhenPropertyChanged(x => x.Age)) + .Subscribe(_ => sourceA.AddOrUpdate(new Person("B-marker", 0))); + + using var barrier = new Barrier(2); + var taskA = Task.Run(() => { barrier.SignalAndWait(); for (var i = 0; i < ItemCount; i++) sourceA.AddOrUpdate(new Person("A-" + iter + "-" + i, i)); }); + var taskB = Task.Run(() => { barrier.SignalAndWait(); for (var i = 0; i < ItemCount; i++) sourceB.AddOrUpdate(new Person("B-" + iter + "-" + i, i)); }); + + var completed = Task.WhenAll(taskA, taskB); + (await Task.WhenAny(completed, Task.Delay(TimeSpan.FromSeconds(TimeoutSeconds)))).Should().BeSameAs(completed, "iteration " + iter); + } + } + [Fact] public async Task TransformWithForce_DoesNotDeadlock() { using var force = new Subject>(); @@ -94,14 +125,18 @@ [Fact] public async Task OnItemRemoved_DoesNotDeadlock() => [Fact] public async Task AllDangerous_Stacked_DoNotDeadlock() { using var pageReq = new BehaviorSubject(new PageRequest(1, 100)); + using var virtReq = new BehaviorSubject(new VirtualRequest(0, 100)); using var force = new Subject>(); (await RunBidirectionalDeadlockTest( - s => s.AutoRefresh(p => p.Age) + s => s.GroupWithImmutableState(p => p.Age % 3) + .TransformMany(g => g.Items, p => p.UniqueKey) + .AutoRefresh(p => p.Age) .Filter(p => p.Age >= 0) .Transform((p, k) => new Person("X-" + p.Name, p.Age), force) .OnItemRemoved(_ => { }) .DisposeMany() .Sort(SortExpressionComparer.Ascending(p => p.Age)) + .Virtualise(virtReq) .Page(pageReq), iterations: Iterations * 2)).Should().BeTrue(); } @@ -114,6 +149,7 @@ [Fact] public async Task MultiplePairs_Simultaneous_NoDeadlock() RunBidirectionalDeadlockTest(s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)), 30), RunBidirectionalDeadlockTest(s => s.AutoRefresh(p => p.Age), 30), RunBidirectionalDeadlockTest(s => s.Group(p => p.Age % 3).MergeMany(g => g.Cache.Connect()), 30), + RunBidirectionalDeadlockTest(s => s.GroupWithImmutableState(p => p.Age % 3).TransformMany(g => g.Items, p => p.UniqueKey), 30), RunBidirectionalDeadlockTest(s => s.OnItemRemoved(_ => { }), 30), RunBidirectionalDeadlockTest(s => s.DisposeMany(), 30), RunBidirectionalDeadlockTest(s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)).Page(pageReq), 30), diff --git a/src/DynamicData/Cache/Internal/AutoRefresh.cs b/src/DynamicData/Cache/Internal/AutoRefresh.cs index ee81fe58..7edd3f01 100644 --- a/src/DynamicData/Cache/Internal/AutoRefresh.cs +++ b/src/DynamicData/Cache/Internal/AutoRefresh.cs @@ -33,7 +33,7 @@ public IObservable> Run() => Observable.Create> Run() => var regroup = _regrouper.SynchronizeSafe(queue).Select(_ => grouper.Regroup()).Where(changes => changes.Count != 0); - return new CompositeDisposable(groups.Merge(regroup).SubscribeSafe(observer), queue); + return new CompositeDisposable(groups.UnsynchronizedMerge(regroup).SubscribeSafe(observer), queue); }); private sealed class Grouper(Func groupSelectorKey) diff --git a/src/DynamicData/Cache/Internal/Page.cs b/src/DynamicData/Cache/Internal/Page.cs index e4159587..2d91ac29 100644 --- a/src/DynamicData/Cache/Internal/Page.cs +++ b/src/DynamicData/Cache/Internal/Page.cs @@ -19,7 +19,7 @@ public IObservable> Run() => Observable.Create updates is not null) .Select(x => x!) .SubscribeSafe(observer), queue); diff --git a/src/DynamicData/Cache/Internal/QueryWhenChanged.cs b/src/DynamicData/Cache/Internal/QueryWhenChanged.cs index 01828e85..7e976424 100644 --- a/src/DynamicData/Cache/Internal/QueryWhenChanged.cs +++ b/src/DynamicData/Cache/Internal/QueryWhenChanged.cs @@ -49,7 +49,7 @@ public IObservable> Run() return cache; }).Select(list => new AnonymousQuery(list)); - return new CompositeDisposable(sourceChanged.Merge(inlineChange).SubscribeSafe(observer), shared.Connect(), queue); + return new CompositeDisposable(sourceChanged.UnsynchronizedMerge(inlineChange).SubscribeSafe(observer), shared.Connect(), queue); }); } } diff --git a/src/DynamicData/Cache/Internal/Sort.cs b/src/DynamicData/Cache/Internal/Sort.cs index 11b39035..6afb9632 100644 --- a/src/DynamicData/Cache/Internal/Sort.cs +++ b/src/DynamicData/Cache/Internal/Sort.cs @@ -57,7 +57,7 @@ public IObservable> Run() => Observable.Create result is not null).Select(x => x!).SubscribeSafe(observer), queue); + return new CompositeDisposable(comparerChanged.UnsynchronizedMerge(dataChanged, sortAgain).Where(result => result is not null).Select(x => x!).SubscribeSafe(observer), queue); }); private sealed class Sorter(SortOptimisations optimisations, IComparer? comparer = null, int resetThreshold = -1) diff --git a/src/DynamicData/Cache/Internal/Virtualise.cs b/src/DynamicData/Cache/Internal/Virtualise.cs index 9a5fefbc..9e858f5d 100644 --- a/src/DynamicData/Cache/Internal/Virtualise.cs +++ b/src/DynamicData/Cache/Internal/Virtualise.cs @@ -23,7 +23,7 @@ public IObservable> Run() => Observable.Create< var request = _virtualRequests.SynchronizeSafe(queue).Select(virtualiser.Virtualise).Where(x => x is not null).Select(x => x!); var dataChange = _source.SynchronizeSafe(queue).Select(virtualiser.Update).Where(x => x is not null).Select(x => x!); - return new CompositeDisposable(request.Merge(dataChange).Where(updates => updates is not null).SubscribeSafe(observer), queue); + return new CompositeDisposable(request.UnsynchronizedMerge(dataChange).Where(updates => updates is not null).SubscribeSafe(observer), queue); }); private sealed class Virtualiser(VirtualRequest? request = null) diff --git a/src/DynamicData/Internal/SynchronizeSafeExtensions.cs b/src/DynamicData/Internal/SynchronizeSafeExtensions.cs index ace8ed4a..577bc64a 100644 --- a/src/DynamicData/Internal/SynchronizeSafeExtensions.cs +++ b/src/DynamicData/Internal/SynchronizeSafeExtensions.cs @@ -2,6 +2,7 @@ // Roland Pheasant licenses this file to you under the MIT license. // See the LICENSE file in the project root for full license information. +using System.Reactive; using System.Reactive.Disposables; using System.Reactive.Linq; @@ -78,4 +79,69 @@ public static IObservable SynchronizeSafe(this IObservable source) => // Queue first: ensures in-flight deliveries complete before teardown side effects run return new CompositeDisposable(queue, source.SubscribeSafe(queue)); }); + + /// + /// Merges with into a single observable + /// without taking any synchronization gate. Functionally equivalent to + /// : completes only after + /// every source completes; the first error terminates; subscription occurs in argument order. + /// + /// + /// The caller MUST ensure that delivery from every source is already serialized. + /// In this library the precondition is satisfied by routing every source through the + /// same via + /// . The shared + /// queue's drain loop guarantees that at most one notification is in flight to the + /// downstream observer at a time, so the additional gate that Observable.Merge + /// would install is redundant. + /// Removing that gate matters in cross-cache pipelines: Observable.Merge + /// holds its private _gate for the entire duration of downstream delivery, and + /// when downstream delivery walks into another cache's writer lock, two such gates on + /// two operators form an ABBA cycle that the queue-drain design is meant to prevent. + /// Without the external serialization precondition, concurrent OnNext + /// calls into the shared observer will race. Do not use as a general-purpose + /// Observable.Merge replacement. + /// + public static IObservable UnsynchronizedMerge(this IObservable first, params IObservable[] others) => + Observable.Create(observer => + { + var totalSources = others.Length + 1; + var subscriptions = new CompositeDisposable(totalSources); + var pending = totalSources; + var terminated = 0; + + void OnNextSafe(T value) + { + if (Volatile.Read(ref terminated) == 0) + { + observer.OnNext(value); + } + } + + void OnErrorSafe(Exception error) + { + if (Interlocked.Exchange(ref terminated, 1) == 0) + { + observer.OnError(error); + } + } + + void OnCompletedSafe() + { + if (Interlocked.Decrement(ref pending) == 0 && + Interlocked.Exchange(ref terminated, 1) == 0) + { + observer.OnCompleted(); + } + } + + var fanOut = Observer.Create(OnNextSafe, OnErrorSafe, OnCompletedSafe); + subscriptions.Add(first.SubscribeSafe(fanOut)); + foreach (var source in others) + { + subscriptions.Add(source.SubscribeSafe(fanOut)); + } + + return subscriptions; + }); } From 7bfe343acfc59477fb8170ccd14003ca4509c2b5 Mon Sep 17 00:00:00 2001 From: Darrin Cullop Date: Wed, 27 May 2026 00:22:15 -0700 Subject: [PATCH 02/24] Fix UnsynchronizedMerge dropping terminal notifications Initial implementation subscribed every source to a single shared Observer.Create instance. The instance is an AnonymousObserver, which derives from ObserverBase and tracks a one-shot _isStopped flag inside its OnCompleted/OnError. Once any source's terminal notification flips that flag, every subsequent OnCompleted from the remaining sources is silently dropped before reaching the pending counter, so the merged observable never emits OnCompleted downstream. CrossCacheDeadlockStressTest.AllOperators_CrossCache_NoDeadlock_CorrectResults caught this consistently in CI: the sourceB.Sort.Virtualise pipeline received OnCompleted from virtBRequests (its first source), but the matching OnCompleted from sourceB.Dispose arrived at a stopped observer and was discarded, leaving LastOrDefaultAsync waiting forever. Each source now subscribes through its own Observer.Create instance. The OnNextSafe/OnErrorSafe/OnCompletedSafe actions close over the same shared pending and terminated counters, so the all-must-complete and first-error-wins semantics are unchanged; only the per-observer one-shot state is now isolated per source. This matches the per-InnerObserver pattern that Rx's own Observable.Merge uses internally. Also apply UnsynchronizedMerge to TransformWithForcedTransform, which was missed in the original survey. Its shared.Merge(refresher) routed both inputs through the same SharedDeliveryQueue but kept Rx's gate, giving the same latent ABBA exposure that DeadlockTortureTest.TransformWithForce_DoesNotDeadlock flagged in CI. Verified: CrossCacheDeadlockStressTest plus the full DeadlockTortureTest fixture pass 10/10 at xUnit.MaxParallelThreads=16; full test suite passes 2323/2323 at xUnit.MaxParallelThreads=4. --- .../Cache/Internal/TransformWithForcedTransform.cs | 2 +- src/DynamicData/Internal/SharedDeliveryQueue.cs | 2 +- .../Internal/SynchronizeSafeExtensions.cs | 12 +++++++++--- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/DynamicData/Cache/Internal/TransformWithForcedTransform.cs b/src/DynamicData/Cache/Internal/TransformWithForcedTransform.cs index c7c95aa6..4bce97ac 100644 --- a/src/DynamicData/Cache/Internal/TransformWithForcedTransform.cs +++ b/src/DynamicData/Cache/Internal/TransformWithForcedTransform.cs @@ -25,7 +25,7 @@ public IObservable> Run() => Observable.Create CaptureChanges(cache, selector)).Select(changes => new ChangeSet(changes)).NotEmpty(); - var sourceAndRefreshes = shared.Merge(refresher); + var sourceAndRefreshes = shared.UnsynchronizedMerge(refresher); // do raw transform var transform = new Transform(sourceAndRefreshes, transformFactory, exceptionCallback, true).Run(); diff --git a/src/DynamicData/Internal/SharedDeliveryQueue.cs b/src/DynamicData/Internal/SharedDeliveryQueue.cs index 6eab2889..9ec57a8e 100644 --- a/src/DynamicData/Internal/SharedDeliveryQueue.cs +++ b/src/DynamicData/Internal/SharedDeliveryQueue.cs @@ -1,4 +1,4 @@ -// Copyright (c) 2011-2025 Roland Pheasant. All rights reserved. +// Copyright (c) 2011-2025 Roland Pheasant. All rights reserved. // Roland Pheasant licenses this file to you under the MIT license. // See the LICENSE file in the project root for full license information. diff --git a/src/DynamicData/Internal/SynchronizeSafeExtensions.cs b/src/DynamicData/Internal/SynchronizeSafeExtensions.cs index 577bc64a..e20131e1 100644 --- a/src/DynamicData/Internal/SynchronizeSafeExtensions.cs +++ b/src/DynamicData/Internal/SynchronizeSafeExtensions.cs @@ -98,6 +98,11 @@ public static IObservable SynchronizeSafe(this IObservable source) => /// holds its private _gate for the entire duration of downstream delivery, and /// when downstream delivery walks into another cache's writer lock, two such gates on /// two operators form an ABBA cycle that the queue-drain design is meant to prevent. + /// Each source is subscribed through its own + /// instance. The actions close over shared pending and terminated counters, but + /// the observer instances must be distinct because Rx's ObserverBase sets a one-shot + /// stopped flag on the first OnCompleted/OnError; a single shared observer + /// would silently drop terminal notifications from every source after the first. /// Without the external serialization precondition, concurrent OnNext /// calls into the shared observer will race. Do not use as a general-purpose /// Observable.Merge replacement. @@ -135,11 +140,12 @@ void OnCompletedSafe() } } - var fanOut = Observer.Create(OnNextSafe, OnErrorSafe, OnCompletedSafe); - subscriptions.Add(first.SubscribeSafe(fanOut)); + IObserver CreateInner() => Observer.Create(OnNextSafe, OnErrorSafe, OnCompletedSafe); + + subscriptions.Add(first.SubscribeSafe(CreateInner())); foreach (var source in others) { - subscriptions.Add(source.SubscribeSafe(fanOut)); + subscriptions.Add(source.SubscribeSafe(CreateInner())); } return subscriptions; From 860adb5d90b6a4070d1a868be18ee11cdc16a60b Mon Sep 17 00:00:00 2001 From: Darrin Cullop Date: Wed, 27 May 2026 00:49:09 -0700 Subject: [PATCH 03/24] Introduce DeliveryQueueMerge to combine SDQ routing and gate-free merge Six of the operators changed in this branch followed the same shape: var queue = new SharedDeliveryQueue(); var s1 = source1.SynchronizeSafe(queue).Select(projection1); var s2 = source2.SynchronizeSafe(queue).Select(projection2); return new CompositeDisposable(s1.UnsynchronizedMerge(s2)... , queue); Every site allocates its own queue, threads it through each input, and unwinds it in the disposable. The pattern is mechanical and easy to get wrong: the queue must outlive the subscription, every input must be serialized through the same queue, and the merge must skip Rx's gate. DeliveryQueueMerge wraps that pattern as one operator. Each overload owns its own SharedDeliveryQueue, routes every input through it via SynchronizeSafe(queue), and combines the serialized streams with UnsynchronizedMerge. The returned disposable tears down the merge before the queue so terminal notifications still flow through the still-active queue. Two flavours: DeliveryQueueMerge(IObservable, params IObservable[]) same-type merge, no projection (AutoRefresh) DeliveryQueueMerge(IObservable, Func, IObservable, Func) heterogeneous two-source merge with projections invoked inside the drain (Page, Virtualise, GroupOnImmutable, QueryWhenChanged) DeliveryQueueMerge(IObservable, ..., IObservable, ..., IObservable, ...) three-source heterogeneous merge (Sort, non-early-return branch) TransformWithForcedTransform keeps its current shape: its queue is shared with a Publish()/cacheLoader subscription that lives outside the merge, so the queue cannot be encapsulated by a merge operator. UnsynchronizedMerge remains the helper there. Verified locally: 437/437 unit tests across the six affected operators pass; DeadlockTortureTest plus CrossCacheDeadlockStressTest pass 10/10 at xUnit.MaxParallelThreads=16; full test suite passes at MaxParallelThreads=4. --- src/DynamicData/Cache/Internal/AutoRefresh.cs | 5 +- .../Cache/Internal/GroupOnImmutable.cs | 12 +- src/DynamicData/Cache/Internal/Page.cs | 10 +- .../Cache/Internal/QueryWhenChanged.cs | 19 ++- src/DynamicData/Cache/Internal/Sort.cs | 17 +-- src/DynamicData/Cache/Internal/Virtualise.cs | 11 +- .../Internal/DeliveryQueueMergeExtensions.cs | 122 ++++++++++++++++++ 7 files changed, 157 insertions(+), 39 deletions(-) create mode 100644 src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs diff --git a/src/DynamicData/Cache/Internal/AutoRefresh.cs b/src/DynamicData/Cache/Internal/AutoRefresh.cs index 7edd3f01..f1f17d67 100644 --- a/src/DynamicData/Cache/Internal/AutoRefresh.cs +++ b/src/DynamicData/Cache/Internal/AutoRefresh.cs @@ -32,9 +32,8 @@ public IObservable> Run() => Observable.Create list.Count > 0).Select(items => new ChangeSet(items)); // publish refreshes and underlying changes - var queue = new SharedDeliveryQueue(); - var publisher = shared.SynchronizeSafe(queue).UnsynchronizedMerge(refreshChanges.SynchronizeSafe(queue)).SubscribeSafe(observer); + var publisher = shared.DeliveryQueueMerge(refreshChanges).SubscribeSafe(observer); - return new CompositeDisposable(publisher, shared.Connect(), queue); + return new CompositeDisposable(publisher, shared.Connect()); }); } diff --git a/src/DynamicData/Cache/Internal/GroupOnImmutable.cs b/src/DynamicData/Cache/Internal/GroupOnImmutable.cs index fc6f3b16..c9b776b1 100644 --- a/src/DynamicData/Cache/Internal/GroupOnImmutable.cs +++ b/src/DynamicData/Cache/Internal/GroupOnImmutable.cs @@ -22,14 +22,12 @@ internal sealed class GroupOnImmutable(IObservable> Run() => Observable.Create>( observer => { - var queue = new SharedDeliveryQueue(); var grouper = new Grouper(_groupSelectorKey); - - var groups = _source.SynchronizeSafe(queue).Select(grouper.Update).Where(changes => changes.Count != 0); - - var regroup = _regrouper.SynchronizeSafe(queue).Select(_ => grouper.Regroup()).Where(changes => changes.Count != 0); - - return new CompositeDisposable(groups.UnsynchronizedMerge(regroup).SubscribeSafe(observer), queue); + return DeliveryQueueMergeExtensions.DeliveryQueueMerge, Unit, IImmutableGroupChangeSet>( + _source, grouper.Update, + _regrouper, _ => grouper.Regroup()) + .Where(changes => changes.Count != 0) + .SubscribeSafe(observer); }); private sealed class Grouper(Func groupSelectorKey) diff --git a/src/DynamicData/Cache/Internal/Page.cs b/src/DynamicData/Cache/Internal/Page.cs index 2d91ac29..f707b657 100644 --- a/src/DynamicData/Cache/Internal/Page.cs +++ b/src/DynamicData/Cache/Internal/Page.cs @@ -14,15 +14,13 @@ internal sealed class Page(IObservable> Run() => Observable.Create>( observer => { - var queue = new SharedDeliveryQueue(); var paginator = new Paginator(); - var request = pageRequests.SynchronizeSafe(queue).Select(paginator.Paginate); - var dataChange = source.SynchronizeSafe(queue).Select(paginator.Update); - - return new CompositeDisposable(request.UnsynchronizedMerge(dataChange) + return DeliveryQueueMergeExtensions.DeliveryQueueMerge, IPagedChangeSet?>( + pageRequests, paginator.Paginate, + source, paginator.Update) .Where(updates => updates is not null) .Select(x => x!) - .SubscribeSafe(observer), queue); + .SubscribeSafe(observer); }); private sealed class Paginator diff --git a/src/DynamicData/Cache/Internal/QueryWhenChanged.cs b/src/DynamicData/Cache/Internal/QueryWhenChanged.cs index 7e976424..11dc5bdf 100644 --- a/src/DynamicData/Cache/Internal/QueryWhenChanged.cs +++ b/src/DynamicData/Cache/Internal/QueryWhenChanged.cs @@ -34,22 +34,21 @@ public IObservable> Run() return Observable.Create>(observer => { - var queue = new SharedDeliveryQueue(); var state = new Cache(); var shared = _source.Publish(); - var inlineChange = shared.MergeMany(itemChangedTrigger).SynchronizeSafe(queue).Select(_ => new AnonymousQuery(state)); - - var sourceChanged = shared.SynchronizeSafe(queue).Scan( - state, - (cache, changes) => + var merged = DeliveryQueueMergeExtensions.DeliveryQueueMerge, TValue, IQuery>( + shared, + changes => { - cache.Clone(changes); - return cache; - }).Select(list => new AnonymousQuery(list)); + state.Clone(changes); + return new AnonymousQuery(state); + }, + shared.MergeMany(itemChangedTrigger), + _ => new AnonymousQuery(state)); - return new CompositeDisposable(sourceChanged.UnsynchronizedMerge(inlineChange).SubscribeSafe(observer), shared.Connect(), queue); + return new CompositeDisposable(merged.SubscribeSafe(observer), shared.Connect()); }); } } diff --git a/src/DynamicData/Cache/Internal/Sort.cs b/src/DynamicData/Cache/Internal/Sort.cs index 6afb9632..4d3c2495 100644 --- a/src/DynamicData/Cache/Internal/Sort.cs +++ b/src/DynamicData/Cache/Internal/Sort.cs @@ -43,7 +43,6 @@ public IObservable> Run() => Observable.Create { var sorter = new Sorter(_sortOptimisations, _comparer, _resetThreshold); - var queue = new SharedDeliveryQueue(); // check for nulls so we can prevent a lock when not required if (_comparerChangedObservable is null && _resorter is null) @@ -51,13 +50,15 @@ public IObservable> Run() => Observable.Create result is not null).Select(x => x!).SubscribeSafe(observer); } - var comparerChanged = (_comparerChangedObservable ?? Observable.Never>()).SynchronizeSafe(queue).Select(sorter.Sort); - - var sortAgain = (_resorter ?? Observable.Never()).SynchronizeSafe(queue).Select(_ => sorter.Sort()); - - var dataChanged = _source.SynchronizeSafe(queue).Select(sorter.Sort); - - return new CompositeDisposable(comparerChanged.UnsynchronizedMerge(dataChanged, sortAgain).Where(result => result is not null).Select(x => x!).SubscribeSafe(observer), queue); + var comparerSource = _comparerChangedObservable ?? Observable.Never>(); + var resorterSource = _resorter ?? Observable.Never(); + return DeliveryQueueMergeExtensions.DeliveryQueueMerge, IChangeSet, Unit, ISortedChangeSet?>( + comparerSource, sorter.Sort, + _source, sorter.Sort, + resorterSource, _ => sorter.Sort()) + .Where(result => result is not null) + .Select(x => x!) + .SubscribeSafe(observer); }); private sealed class Sorter(SortOptimisations optimisations, IComparer? comparer = null, int resetThreshold = -1) diff --git a/src/DynamicData/Cache/Internal/Virtualise.cs b/src/DynamicData/Cache/Internal/Virtualise.cs index 9e858f5d..3ed1e79c 100644 --- a/src/DynamicData/Cache/Internal/Virtualise.cs +++ b/src/DynamicData/Cache/Internal/Virtualise.cs @@ -19,11 +19,12 @@ public IObservable> Run() => Observable.Create< observer => { var virtualiser = new Virtualiser(); - var queue = new SharedDeliveryQueue(); - - var request = _virtualRequests.SynchronizeSafe(queue).Select(virtualiser.Virtualise).Where(x => x is not null).Select(x => x!); - var dataChange = _source.SynchronizeSafe(queue).Select(virtualiser.Update).Where(x => x is not null).Select(x => x!); - return new CompositeDisposable(request.UnsynchronizedMerge(dataChange).Where(updates => updates is not null).SubscribeSafe(observer), queue); + return DeliveryQueueMergeExtensions.DeliveryQueueMerge, IVirtualChangeSet?>( + _virtualRequests, virtualiser.Virtualise, + _source, virtualiser.Update) + .Where(updates => updates is not null) + .Select(x => x!) + .SubscribeSafe(observer); }); private sealed class Virtualiser(VirtualRequest? request = null) diff --git a/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs b/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs new file mode 100644 index 00000000..6783d3bc --- /dev/null +++ b/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs @@ -0,0 +1,122 @@ +// Copyright (c) 2011-2025 Roland Pheasant. All rights reserved. +// Roland Pheasant licenses this file to you under the MIT license. +// See the LICENSE file in the project root for full license information. + +using System.Reactive.Disposables; +using System.Reactive.Linq; + +namespace DynamicData.Internal; + +/// +/// Provides DeliveryQueueMerge extension methods, which combine the +/// serialization step and the gate-free merge step +/// from into a single +/// operator. +/// +/// +/// The motivation is the cross-cache deadlock that operators like Page, +/// Virtualise, AutoRefresh, Sort, GroupWithImmutableState, and +/// QueryWhenChanged all needed to solve: route every input through a single +/// so downstream delivery is serialized without holding +/// any operator-level lock, then merge the serialized inputs without re-introducing an +/// Observable.Merge gate that would reconstruct the ABBA cycle. +/// Each overload creates and owns its own , wraps +/// every source through , +/// and finally combines the wrapped streams with +/// . +/// The returned tears down the merge subscription before +/// the queue, so any final terminal notification still flows through the still-active queue. +/// The heterogeneous overloads accept a projection per input. Each projection is +/// applied inside the queue's drain (via SynchronizeSafe(queue).Select(projection)), +/// so stateful projections (paginators, sorters, virtualisers, group trackers) see a single, +/// serialized stream of invocations and may safely mutate operator-private state without +/// additional locking. +/// +internal static class DeliveryQueueMergeExtensions +{ + /// + /// Merges with after routing every + /// source through a single . + /// + /// The element type, common to every input. + /// The first input observable. + /// Additional input observables. + /// An observable that emits items from every input, serialized through a shared queue. + public static IObservable DeliveryQueueMerge(this IObservable first, params IObservable[] others) => + Observable.Create(observer => + { + var queue = new SharedDeliveryQueue(); + var firstSync = first.SynchronizeSafe(queue); + var othersSync = new IObservable[others.Length]; + for (var i = 0; i < others.Length; i++) + { + othersSync[i] = others[i].SynchronizeSafe(queue); + } + + return new CompositeDisposable( + firstSync.UnsynchronizedMerge(othersSync).SubscribeSafe(observer), + queue); + }); + + /// + /// Merges two inputs of different element types into a single observable of + /// by routing each input through a single shared and applying its + /// projection inside the drain. + /// + /// Element type of the first input. + /// Element type of the second input. + /// Common output element type produced by both projections. + /// The first input observable. + /// Projection applied to each element from . + /// The second input observable. + /// Projection applied to each element from . + /// An observable emitting the projected items from both inputs, serialized through a shared queue. + public static IObservable DeliveryQueueMerge( + IObservable first, + Func firstProjection, + IObservable second, + Func secondProjection) => + Observable.Create(observer => + { + var queue = new SharedDeliveryQueue(); + var firstProjected = first.SynchronizeSafe(queue).Select(firstProjection); + var secondProjected = second.SynchronizeSafe(queue).Select(secondProjection); + + return new CompositeDisposable( + firstProjected.UnsynchronizedMerge(secondProjected).SubscribeSafe(observer), + queue); + }); + + /// + /// Three-input variant of the heterogeneous overload. + /// + /// Element type of the first input. + /// Element type of the second input. + /// Element type of the third input. + /// Common output element type produced by all projections. + /// The first input observable. + /// Projection applied to each element from . + /// The second input observable. + /// Projection applied to each element from . + /// The third input observable. + /// Projection applied to each element from . + /// An observable emitting the projected items from all three inputs, serialized through a shared queue. + public static IObservable DeliveryQueueMerge( + IObservable first, + Func firstProjection, + IObservable second, + Func secondProjection, + IObservable third, + Func thirdProjection) => + Observable.Create(observer => + { + var queue = new SharedDeliveryQueue(); + var firstProjected = first.SynchronizeSafe(queue).Select(firstProjection); + var secondProjected = second.SynchronizeSafe(queue).Select(secondProjection); + var thirdProjected = third.SynchronizeSafe(queue).Select(thirdProjection); + + return new CompositeDisposable( + firstProjected.UnsynchronizedMerge(secondProjected, thirdProjected).SubscribeSafe(observer), + queue); + }); +} \ No newline at end of file From f4e983ad5d853a2e2bf2dc94cbdad893d88d9eda Mon Sep 17 00:00:00 2001 From: Darrin Cullop Date: Wed, 27 May 2026 10:49:57 -0700 Subject: [PATCH 04/24] Scope DeliveryQueueMerge to AutoRefresh's same-type case The heterogeneous DeliveryQueueMerge overloads pushed too much into each call site to read like idiomatic Rx, and at five of the six operators the projections had to run inside the shared delivery queue to preserve Rx semantics, which the operator-level signature could not express without exposing the queue type to the caller. Keep the same-type extension overload only: public static IObservable DeliveryQueueMerge( this IObservable first, params IObservable[] others) This reads as a drop-in for Observable.Merge at AutoRefresh's call site, which is the only place all inputs are already the same type and need no per-input projection inside the drain. Page, Virtualise, Sort, GroupOnImmutable, and QueryWhenChanged keep the explicit SharedDeliveryQueue + SynchronizeSafe(queue) + UnsynchronizedMerge shape introduced earlier in this branch. Each call site shows the queue plumbing because the projections must execute inside the drain; making that visible matches the rest of the code in the file. --- .../Cache/Internal/GroupOnImmutable.cs | 12 ++- src/DynamicData/Cache/Internal/Page.cs | 10 +- .../Cache/Internal/QueryWhenChanged.cs | 19 ++-- src/DynamicData/Cache/Internal/Sort.cs | 17 ++-- src/DynamicData/Cache/Internal/Virtualise.cs | 11 +-- .../Internal/DeliveryQueueMergeExtensions.cs | 98 +++---------------- 6 files changed, 50 insertions(+), 117 deletions(-) diff --git a/src/DynamicData/Cache/Internal/GroupOnImmutable.cs b/src/DynamicData/Cache/Internal/GroupOnImmutable.cs index c9b776b1..fc6f3b16 100644 --- a/src/DynamicData/Cache/Internal/GroupOnImmutable.cs +++ b/src/DynamicData/Cache/Internal/GroupOnImmutable.cs @@ -22,12 +22,14 @@ internal sealed class GroupOnImmutable(IObservable> Run() => Observable.Create>( observer => { + var queue = new SharedDeliveryQueue(); var grouper = new Grouper(_groupSelectorKey); - return DeliveryQueueMergeExtensions.DeliveryQueueMerge, Unit, IImmutableGroupChangeSet>( - _source, grouper.Update, - _regrouper, _ => grouper.Regroup()) - .Where(changes => changes.Count != 0) - .SubscribeSafe(observer); + + var groups = _source.SynchronizeSafe(queue).Select(grouper.Update).Where(changes => changes.Count != 0); + + var regroup = _regrouper.SynchronizeSafe(queue).Select(_ => grouper.Regroup()).Where(changes => changes.Count != 0); + + return new CompositeDisposable(groups.UnsynchronizedMerge(regroup).SubscribeSafe(observer), queue); }); private sealed class Grouper(Func groupSelectorKey) diff --git a/src/DynamicData/Cache/Internal/Page.cs b/src/DynamicData/Cache/Internal/Page.cs index f707b657..2d91ac29 100644 --- a/src/DynamicData/Cache/Internal/Page.cs +++ b/src/DynamicData/Cache/Internal/Page.cs @@ -14,13 +14,15 @@ internal sealed class Page(IObservable> Run() => Observable.Create>( observer => { + var queue = new SharedDeliveryQueue(); var paginator = new Paginator(); - return DeliveryQueueMergeExtensions.DeliveryQueueMerge, IPagedChangeSet?>( - pageRequests, paginator.Paginate, - source, paginator.Update) + var request = pageRequests.SynchronizeSafe(queue).Select(paginator.Paginate); + var dataChange = source.SynchronizeSafe(queue).Select(paginator.Update); + + return new CompositeDisposable(request.UnsynchronizedMerge(dataChange) .Where(updates => updates is not null) .Select(x => x!) - .SubscribeSafe(observer); + .SubscribeSafe(observer), queue); }); private sealed class Paginator diff --git a/src/DynamicData/Cache/Internal/QueryWhenChanged.cs b/src/DynamicData/Cache/Internal/QueryWhenChanged.cs index 11dc5bdf..7e976424 100644 --- a/src/DynamicData/Cache/Internal/QueryWhenChanged.cs +++ b/src/DynamicData/Cache/Internal/QueryWhenChanged.cs @@ -34,21 +34,22 @@ public IObservable> Run() return Observable.Create>(observer => { + var queue = new SharedDeliveryQueue(); var state = new Cache(); var shared = _source.Publish(); - var merged = DeliveryQueueMergeExtensions.DeliveryQueueMerge, TValue, IQuery>( - shared, - changes => + var inlineChange = shared.MergeMany(itemChangedTrigger).SynchronizeSafe(queue).Select(_ => new AnonymousQuery(state)); + + var sourceChanged = shared.SynchronizeSafe(queue).Scan( + state, + (cache, changes) => { - state.Clone(changes); - return new AnonymousQuery(state); - }, - shared.MergeMany(itemChangedTrigger), - _ => new AnonymousQuery(state)); + cache.Clone(changes); + return cache; + }).Select(list => new AnonymousQuery(list)); - return new CompositeDisposable(merged.SubscribeSafe(observer), shared.Connect()); + return new CompositeDisposable(sourceChanged.UnsynchronizedMerge(inlineChange).SubscribeSafe(observer), shared.Connect(), queue); }); } } diff --git a/src/DynamicData/Cache/Internal/Sort.cs b/src/DynamicData/Cache/Internal/Sort.cs index 4d3c2495..6afb9632 100644 --- a/src/DynamicData/Cache/Internal/Sort.cs +++ b/src/DynamicData/Cache/Internal/Sort.cs @@ -43,6 +43,7 @@ public IObservable> Run() => Observable.Create { var sorter = new Sorter(_sortOptimisations, _comparer, _resetThreshold); + var queue = new SharedDeliveryQueue(); // check for nulls so we can prevent a lock when not required if (_comparerChangedObservable is null && _resorter is null) @@ -50,15 +51,13 @@ public IObservable> Run() => Observable.Create result is not null).Select(x => x!).SubscribeSafe(observer); } - var comparerSource = _comparerChangedObservable ?? Observable.Never>(); - var resorterSource = _resorter ?? Observable.Never(); - return DeliveryQueueMergeExtensions.DeliveryQueueMerge, IChangeSet, Unit, ISortedChangeSet?>( - comparerSource, sorter.Sort, - _source, sorter.Sort, - resorterSource, _ => sorter.Sort()) - .Where(result => result is not null) - .Select(x => x!) - .SubscribeSafe(observer); + var comparerChanged = (_comparerChangedObservable ?? Observable.Never>()).SynchronizeSafe(queue).Select(sorter.Sort); + + var sortAgain = (_resorter ?? Observable.Never()).SynchronizeSafe(queue).Select(_ => sorter.Sort()); + + var dataChanged = _source.SynchronizeSafe(queue).Select(sorter.Sort); + + return new CompositeDisposable(comparerChanged.UnsynchronizedMerge(dataChanged, sortAgain).Where(result => result is not null).Select(x => x!).SubscribeSafe(observer), queue); }); private sealed class Sorter(SortOptimisations optimisations, IComparer? comparer = null, int resetThreshold = -1) diff --git a/src/DynamicData/Cache/Internal/Virtualise.cs b/src/DynamicData/Cache/Internal/Virtualise.cs index 3ed1e79c..9e858f5d 100644 --- a/src/DynamicData/Cache/Internal/Virtualise.cs +++ b/src/DynamicData/Cache/Internal/Virtualise.cs @@ -19,12 +19,11 @@ public IObservable> Run() => Observable.Create< observer => { var virtualiser = new Virtualiser(); - return DeliveryQueueMergeExtensions.DeliveryQueueMerge, IVirtualChangeSet?>( - _virtualRequests, virtualiser.Virtualise, - _source, virtualiser.Update) - .Where(updates => updates is not null) - .Select(x => x!) - .SubscribeSafe(observer); + var queue = new SharedDeliveryQueue(); + + var request = _virtualRequests.SynchronizeSafe(queue).Select(virtualiser.Virtualise).Where(x => x is not null).Select(x => x!); + var dataChange = _source.SynchronizeSafe(queue).Select(virtualiser.Update).Where(x => x is not null).Select(x => x!); + return new CompositeDisposable(request.UnsynchronizedMerge(dataChange).Where(updates => updates is not null).SubscribeSafe(observer), queue); }); private sealed class Virtualiser(VirtualRequest? request = null) diff --git a/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs b/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs index 6783d3bc..720189f8 100644 --- a/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs +++ b/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs @@ -8,35 +8,27 @@ namespace DynamicData.Internal; /// -/// Provides DeliveryQueueMerge extension methods, which combine the -/// serialization step and the gate-free merge step -/// from into a single -/// operator. +/// Provides DeliveryQueueMerge, an Rx extension method that combines +/// the serialization step and the gate-free +/// merge step from +/// into a single operator. /// /// -/// The motivation is the cross-cache deadlock that operators like Page, -/// Virtualise, AutoRefresh, Sort, GroupWithImmutableState, and -/// QueryWhenChanged all needed to solve: route every input through a single -/// so downstream delivery is serialized without holding -/// any operator-level lock, then merge the serialized inputs without re-introducing an -/// Observable.Merge gate that would reconstruct the ABBA cycle. -/// Each overload creates and owns its own , wraps -/// every source through , -/// and finally combines the wrapped streams with -/// . -/// The returned tears down the merge subscription before -/// the queue, so any final terminal notification still flows through the still-active queue. -/// The heterogeneous overloads accept a projection per input. Each projection is -/// applied inside the queue's drain (via SynchronizeSafe(queue).Select(projection)), -/// so stateful projections (paginators, sorters, virtualisers, group trackers) see a single, -/// serialized stream of invocations and may safely mutate operator-private state without -/// additional locking. +/// Use this when every input is already of the same element type and no per-input +/// projection is needed before the merge; the operator owns the queue lifecycle so the +/// call site reads like an ordinary . +/// When the inputs have different element types or require operator-private projections +/// invoked inside the queue's drain, use +/// and directly so the +/// projections sit inside the serialized section. /// internal static class DeliveryQueueMergeExtensions { /// /// Merges with after routing every - /// source through a single . + /// source through a single . Drop-in alternative to + /// for cross-cache + /// pipelines where the Rx Merge gate would risk an ABBA cycle. /// /// The element type, common to every input. /// The first input observable. @@ -57,66 +49,4 @@ public static IObservable DeliveryQueueMerge(this IObservable first, pa firstSync.UnsynchronizedMerge(othersSync).SubscribeSafe(observer), queue); }); - - /// - /// Merges two inputs of different element types into a single observable of - /// by routing each input through a single shared and applying its - /// projection inside the drain. - /// - /// Element type of the first input. - /// Element type of the second input. - /// Common output element type produced by both projections. - /// The first input observable. - /// Projection applied to each element from . - /// The second input observable. - /// Projection applied to each element from . - /// An observable emitting the projected items from both inputs, serialized through a shared queue. - public static IObservable DeliveryQueueMerge( - IObservable first, - Func firstProjection, - IObservable second, - Func secondProjection) => - Observable.Create(observer => - { - var queue = new SharedDeliveryQueue(); - var firstProjected = first.SynchronizeSafe(queue).Select(firstProjection); - var secondProjected = second.SynchronizeSafe(queue).Select(secondProjection); - - return new CompositeDisposable( - firstProjected.UnsynchronizedMerge(secondProjected).SubscribeSafe(observer), - queue); - }); - - /// - /// Three-input variant of the heterogeneous overload. - /// - /// Element type of the first input. - /// Element type of the second input. - /// Element type of the third input. - /// Common output element type produced by all projections. - /// The first input observable. - /// Projection applied to each element from . - /// The second input observable. - /// Projection applied to each element from . - /// The third input observable. - /// Projection applied to each element from . - /// An observable emitting the projected items from all three inputs, serialized through a shared queue. - public static IObservable DeliveryQueueMerge( - IObservable first, - Func firstProjection, - IObservable second, - Func secondProjection, - IObservable third, - Func thirdProjection) => - Observable.Create(observer => - { - var queue = new SharedDeliveryQueue(); - var firstProjected = first.SynchronizeSafe(queue).Select(firstProjection); - var secondProjected = second.SynchronizeSafe(queue).Select(secondProjection); - var thirdProjected = third.SynchronizeSafe(queue).Select(thirdProjection); - - return new CompositeDisposable( - firstProjected.UnsynchronizedMerge(secondProjected, thirdProjected).SubscribeSafe(observer), - queue); - }); } \ No newline at end of file From 0cab88e42e0d03f69b8e48ab81d8db0bd3e3afbd Mon Sep 17 00:00:00 2001 From: Darrin Cullop Date: Wed, 27 May 2026 11:25:43 -0700 Subject: [PATCH 05/24] Exercise subject-driven branches in DeadlockTortureTest Tests with subject inputs (Page, Virtualise, BatchIf, TransformWithForce, AllDangerous_Stacked, MultiplePairs) created the subject but nothing ever called OnNext on it. The bidirectional source writes still flowed through the operator's Merge gate, so the original deadlock was triggered, but the operator's subject-driven branch (refresher, request changes, pause toggle) was never invoked during the race. A regression that broke only that branch would not be caught. Add an optional subjectPusher callback to RunBidirectionalDeadlockTest that runs on a third worker thread, gated by the same Barrier as the two writer threads, and have each subject-bearing test push its own pattern on the subject while sources are writing. For the Page/Virtualise/BatchIf inline subjects in MultiplePairs, lift them to named locals so they can be referenced from the pusher closure. Also collapse the vertical layout introduced in the previous commits for DeliveryQueueMerge's CompositeDisposable construction and the UnsynchronizedMerge OnCompleted predicate. --- .../Cache/DeadlockTortureTest.cs | 65 ++++++++++++++----- .../Internal/DeliveryQueueMergeExtensions.cs | 6 +- .../Internal/SynchronizeSafeExtensions.cs | 3 +- 3 files changed, 52 insertions(+), 22 deletions(-) diff --git a/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs b/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs index 8576d34c..f71971b4 100644 --- a/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs +++ b/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs @@ -34,6 +34,7 @@ public sealed class DeadlockTortureTest private static async Task RunBidirectionalDeadlockTest( Func>, IObservable>> pipeline, + Action? subjectPusher = null, int iterations = Iterations) { for (var iter = 0; iter < iterations; iter++) @@ -44,11 +45,13 @@ private static async Task RunBidirectionalDeadlockTest( using var aToB = pipeline(sourceA.Connect().Filter(x => x.Name.StartsWith("A"))).PopulateInto(sourceB); using var bToA = pipeline(sourceB.Connect().Filter(x => x.Name.StartsWith("B"))).PopulateInto(sourceA); - using var barrier = new Barrier(2); + var participants = subjectPusher is null ? 2 : 3; + using var barrier = new Barrier(participants); var taskA = Task.Run(() => { barrier.SignalAndWait(); for (var i = 0; i < ItemCount; i++) sourceA.AddOrUpdate(new Person("A-" + iter + "-" + i, i)); }); var taskB = Task.Run(() => { barrier.SignalAndWait(); for (var i = 0; i < ItemCount; i++) sourceB.AddOrUpdate(new Person("B-" + iter + "-" + i, i)); }); + var taskC = subjectPusher is null ? null : Task.Run(() => { barrier.SignalAndWait(); subjectPusher(); }); - var completed = Task.WhenAll(taskA, taskB); + var completed = taskC is null ? Task.WhenAll(taskA, taskB) : Task.WhenAll(taskA, taskB, taskC); if (await Task.WhenAny(completed, Task.Delay(TimeSpan.FromSeconds(TimeoutSeconds))) != completed) return false; } @@ -70,13 +73,17 @@ [Fact] public async Task GroupWithImmutableState_DoesNotDeadlock() => [Fact] public async Task Page_DoesNotDeadlock() { using var req = new BehaviorSubject(new PageRequest(1, 50)); - (await RunBidirectionalDeadlockTest(s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)).Page(req))).Should().BeTrue(); + (await RunBidirectionalDeadlockTest( + s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)).Page(req), + subjectPusher: () => { for (var j = 0; j < ItemCount; j++) req.OnNext(new PageRequest(1 + (j % 4), 25 + (j % 4) * 25)); })).Should().BeTrue(); } [Fact] public async Task Virtualise_DoesNotDeadlock() { using var req = new BehaviorSubject(new VirtualRequest(0, 50)); - (await RunBidirectionalDeadlockTest(s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)).Virtualise(req))).Should().BeTrue(); + (await RunBidirectionalDeadlockTest( + s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)).Virtualise(req), + subjectPusher: () => { for (var j = 0; j < ItemCount; j++) req.OnNext(new VirtualRequest(j * 5, 25 + (j % 4) * 25)); })).Should().BeTrue(); } [Fact] public async Task QueryWhenChanged_DoesNotDeadlock() @@ -110,11 +117,18 @@ [Fact] public async Task QueryWhenChanged_DoesNotDeadlock() [Fact] public async Task TransformWithForce_DoesNotDeadlock() { using var force = new Subject>(); - (await RunBidirectionalDeadlockTest(s => s.Transform((p, k) => new Person("T-" + p.Name, p.Age), force))).Should().BeTrue(); + (await RunBidirectionalDeadlockTest( + s => s.Transform((p, k) => new Person("T-" + p.Name, p.Age), force), + subjectPusher: () => { for (var j = 0; j < ItemCount; j++) force.OnNext(static (p, _) => true); })).Should().BeTrue(); } - [Fact] public async Task BatchIf_DoesNotDeadlock() => - (await RunBidirectionalDeadlockTest(s => s.BatchIf(new BehaviorSubject(false), false, (TimeSpan?)null))).Should().BeTrue(); + [Fact] public async Task BatchIf_DoesNotDeadlock() + { + using var pause = new BehaviorSubject(false); + (await RunBidirectionalDeadlockTest( + s => s.BatchIf(pause, false, (TimeSpan?)null), + subjectPusher: () => { for (var j = 0; j < ItemCount; j++) pause.OnNext(j % 2 == 0); })).Should().BeTrue(); + } [Fact] public async Task DisposeMany_DoesNotDeadlock() => (await RunBidirectionalDeadlockTest(s => s.DisposeMany())).Should().BeTrue(); @@ -138,6 +152,15 @@ [Fact] public async Task AllDangerous_Stacked_DoNotDeadlock() .Sort(SortExpressionComparer.Ascending(p => p.Age)) .Virtualise(virtReq) .Page(pageReq), + subjectPusher: () => + { + for (var j = 0; j < ItemCount; j++) + { + force.OnNext(static (p, _) => true); + pageReq.OnNext(new PageRequest(1 + (j % 4), 50 + (j % 4) * 50)); + virtReq.OnNext(new VirtualRequest(j * 5, 50 + (j % 4) * 50)); + } + }, iterations: Iterations * 2)).Should().BeTrue(); } @@ -145,16 +168,26 @@ [Fact] public async Task MultiplePairs_Simultaneous_NoDeadlock() { using var pageReq = new BehaviorSubject(new PageRequest(1, 50)); using var virtReq = new BehaviorSubject(new VirtualRequest(0, 50)); + using var pause = new BehaviorSubject(false); var results = await Task.WhenAll( - RunBidirectionalDeadlockTest(s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)), 30), - RunBidirectionalDeadlockTest(s => s.AutoRefresh(p => p.Age), 30), - RunBidirectionalDeadlockTest(s => s.Group(p => p.Age % 3).MergeMany(g => g.Cache.Connect()), 30), - RunBidirectionalDeadlockTest(s => s.GroupWithImmutableState(p => p.Age % 3).TransformMany(g => g.Items, p => p.UniqueKey), 30), - RunBidirectionalDeadlockTest(s => s.OnItemRemoved(_ => { }), 30), - RunBidirectionalDeadlockTest(s => s.DisposeMany(), 30), - RunBidirectionalDeadlockTest(s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)).Page(pageReq), 30), - RunBidirectionalDeadlockTest(s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)).Virtualise(virtReq), 30), - RunBidirectionalDeadlockTest(s => s.BatchIf(new BehaviorSubject(false), false, (TimeSpan?)null), 30)); + RunBidirectionalDeadlockTest(s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)), iterations: 30), + RunBidirectionalDeadlockTest(s => s.AutoRefresh(p => p.Age), iterations: 30), + RunBidirectionalDeadlockTest(s => s.Group(p => p.Age % 3).MergeMany(g => g.Cache.Connect()), iterations: 30), + RunBidirectionalDeadlockTest(s => s.GroupWithImmutableState(p => p.Age % 3).TransformMany(g => g.Items, p => p.UniqueKey), iterations: 30), + RunBidirectionalDeadlockTest(s => s.OnItemRemoved(_ => { }), iterations: 30), + RunBidirectionalDeadlockTest(s => s.DisposeMany(), iterations: 30), + RunBidirectionalDeadlockTest( + s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)).Page(pageReq), + subjectPusher: () => { for (var j = 0; j < ItemCount; j++) pageReq.OnNext(new PageRequest(1 + (j % 4), 25 + (j % 4) * 25)); }, + iterations: 30), + RunBidirectionalDeadlockTest( + s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)).Virtualise(virtReq), + subjectPusher: () => { for (var j = 0; j < ItemCount; j++) virtReq.OnNext(new VirtualRequest(j * 5, 25 + (j % 4) * 25)); }, + iterations: 30), + RunBidirectionalDeadlockTest( + s => s.BatchIf(pause, false, (TimeSpan?)null), + subjectPusher: () => { for (var j = 0; j < ItemCount; j++) pause.OnNext(j % 2 == 0); }, + iterations: 30)); results.Should().AllSatisfy(r => r.Should().BeTrue()); } diff --git a/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs b/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs index 720189f8..34e649ea 100644 --- a/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs +++ b/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs @@ -45,8 +45,6 @@ public static IObservable DeliveryQueueMerge(this IObservable first, pa othersSync[i] = others[i].SynchronizeSafe(queue); } - return new CompositeDisposable( - firstSync.UnsynchronizedMerge(othersSync).SubscribeSafe(observer), - queue); + return new CompositeDisposable(firstSync.UnsynchronizedMerge(othersSync).SubscribeSafe(observer), queue); }); -} \ No newline at end of file +} diff --git a/src/DynamicData/Internal/SynchronizeSafeExtensions.cs b/src/DynamicData/Internal/SynchronizeSafeExtensions.cs index e20131e1..283dfff5 100644 --- a/src/DynamicData/Internal/SynchronizeSafeExtensions.cs +++ b/src/DynamicData/Internal/SynchronizeSafeExtensions.cs @@ -133,8 +133,7 @@ void OnErrorSafe(Exception error) void OnCompletedSafe() { - if (Interlocked.Decrement(ref pending) == 0 && - Interlocked.Exchange(ref terminated, 1) == 0) + if (Interlocked.Decrement(ref pending) == 0 && Interlocked.Exchange(ref terminated, 1) == 0) { observer.OnCompleted(); } From 9d526a36da808f1b6e0dd2ebb933189ea0b92dbd Mon Sep 17 00:00:00 2001 From: Darrin Cullop Date: Wed, 27 May 2026 13:20:31 -0700 Subject: [PATCH 06/24] Use typed DeliveryQueue for DeliveryQueueMerge Every input has the same element type T, so the type-erased SharedDeliveryQueue with its per-source DeliverySubQueue wrappers was carrying machinery (bitset, sub-queue list, type-erased StageNext/ DeliverStaged dispatch) that the same-type merge never used. Replace the implementation with one DeliveryQueue and per-source Observer.Create instances: - OnNext: forwarded directly to queue.OnNext. The queue's gate serializes concurrent calls from multiple producers; the drain delivers items in arrival order outside the lock, so a downstream observer that walks into another cache's writer lock cannot deadlock with this serialization point. - OnError: forwarded directly to queue.OnError. The queue marks itself terminated at the first error reaching the drain, so a second concurrent error from another source is dropped at enqueue and the downstream observer sees OnError exactly once. - OnCompleted: counter-gated; only the last surviving source's completion calls queue.OnCompleted, matching Observable.Merge's all-must-complete semantic. If a source has already errored, the queue is terminated and the eventual OnCompleted at the counter's floor is dropped at enqueue. The per-source Observer.Create instance is required for the same reason it is in UnsynchronizedMerge: Rx's ObserverBase sets a one-shot stopped flag on the first OnCompleted/OnError, and a single shared observer would silently drop terminal notifications from every source after the first. AutoRefresh is the only consumer of DeliveryQueueMerge. All tests across AutoRefresh, DeadlockTortureTest, and CrossCacheDeadlockStressTest pass; deadlock fixture passes 5/5 at xUnit.MaxParallelThreads=16. --- .../Internal/DeliveryQueueMergeExtensions.cs | 73 +++++++++++++------ 1 file changed, 51 insertions(+), 22 deletions(-) diff --git a/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs b/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs index 34e649ea..9adefaeb 100644 --- a/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs +++ b/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs @@ -2,49 +2,78 @@ // Roland Pheasant licenses this file to you under the MIT license. // See the LICENSE file in the project root for full license information. +using System.Reactive; using System.Reactive.Disposables; using System.Reactive.Linq; namespace DynamicData.Internal; /// -/// Provides DeliveryQueueMerge, an Rx extension method that combines -/// the serialization step and the gate-free -/// merge step from -/// into a single operator. +/// Provides DeliveryQueueMerge, an Rx extension method that serializes the +/// notifications of every input through a single +/// and emits them on the downstream observer outside the queue's lock. /// /// -/// Use this when every input is already of the same element type and no per-input -/// projection is needed before the merge; the operator owns the queue lifecycle so the -/// call site reads like an ordinary . -/// When the inputs have different element types or require operator-private projections -/// invoked inside the queue's drain, use -/// and directly so the -/// projections sit inside the serialized section. +/// Drop-in alternative to +/// for cross-cache pipelines where the Rx Merge gate, held during downstream delivery, +/// would risk an ABBA cycle. serializes enqueues across +/// concurrent producers but releases its gate before delivering, so a downstream +/// observer that walks into another cache's writer lock cannot deadlock with this +/// operator's serialization point. +/// Every input must share the same element type. When the inputs have different +/// element types or require operator-private projections invoked inside the queue's +/// drain, use +/// with a and finish with +/// . /// internal static class DeliveryQueueMergeExtensions { /// - /// Merges with after routing every - /// source through a single . Drop-in alternative to - /// for cross-cache - /// pipelines where the Rx Merge gate would risk an ABBA cycle. + /// Merges with by routing every + /// source through a single . Functionally equivalent + /// to : completes + /// only after every source completes; the first error terminates; subscription + /// occurs in argument order. /// /// The element type, common to every input. /// The first input observable. /// Additional input observables. - /// An observable that emits items from every input, serialized through a shared queue. + /// An observable that emits items from every input, serialized through the queue. public static IObservable DeliveryQueueMerge(this IObservable first, params IObservable[] others) => Observable.Create(observer => { - var queue = new SharedDeliveryQueue(); - var firstSync = first.SynchronizeSafe(queue); - var othersSync = new IObservable[others.Length]; - for (var i = 0; i < others.Length; i++) + var queue = new DeliveryQueue(observer); + var totalSources = others.Length + 1; + var subscriptions = new CompositeDisposable(totalSources + 1); + var pending = totalSources; + + // Each source needs its own inner observer instance because Rx's ObserverBase + // sets a one-shot stopped flag on the first OnCompleted/OnError; a single shared + // observer would silently drop terminal notifications from every source after + // the first. OnNext and OnError forward straight to the queue (the queue's gate + // serializes concurrent calls); OnCompleted is counter-gated so only the last + // surviving source's completion terminates the merged stream. + IObserver CreateInner() => + Observer.Create( + queue.OnNext, + queue.OnError, + () => + { + if (Interlocked.Decrement(ref pending) == 0) + { + queue.OnCompleted(); + } + }); + + subscriptions.Add(first.SubscribeSafe(CreateInner())); + foreach (var source in others) { - othersSync[i] = others[i].SynchronizeSafe(queue); + subscriptions.Add(source.SubscribeSafe(CreateInner())); } - return new CompositeDisposable(firstSync.UnsynchronizedMerge(othersSync).SubscribeSafe(observer), queue); + // Subscription first so any terminal notification produced during Rx's disposal + // cascade still flows through the still-active queue. Queue last as cleanup. + subscriptions.Add(queue); + return subscriptions; }); } From b362db91cb136fca762b039e1fc51161754463cc Mon Sep 17 00:00:00 2001 From: Darrin Cullop Date: Wed, 27 May 2026 13:43:43 -0700 Subject: [PATCH 07/24] Trim AllDangerous_Stacked pusher load to fit per-iteration timeout PR build failed AllDangerous_Stacked_DoNotDeadlock after 27s on a single iteration (the per-iteration TimeoutSeconds=15 budget was exceeded, then RunBidirectionalDeadlockTest returned false). It was not a deadlock; the pipeline was just doing too much work. Each force.OnNext in this test triggers TransformWithForcedTransform's refresher, which scans cache.KeyValues and emits a refresh changeset that flows through the full 9-operator stack (GroupWithImmutableState, TransformMany, AutoRefresh, Filter, Transform, OnItemRemoved, DisposeMany, Sort, Virtualise, Page). At ItemCount=200 pusher iterations with three subjects pushed per iteration (force, pageReq, virtReq), the pusher thread did ~600 push operations per iteration on top of the two writer threads' 200 source AddOrUpdates each. The other torture tests have a single-operator pipeline and one pusher and fit well within the budget; only the stacked case combines a heavy pipeline with three concurrent pushers. Reduce StackedPushCount to ItemCount/4 = 50, three subjects each. That keeps the subject branches under contention (still 150 pushes per iteration, still well above source-write rate) while bringing each iteration's worst case comfortably under TimeoutSeconds. The other subject-bearing tests are unchanged. --- src/DynamicData.Tests/Cache/DeadlockTortureTest.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs b/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs index f71971b4..854ce050 100644 --- a/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs +++ b/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs @@ -141,6 +141,11 @@ [Fact] public async Task AllDangerous_Stacked_DoNotDeadlock() using var pageReq = new BehaviorSubject(new PageRequest(1, 100)); using var virtReq = new BehaviorSubject(new VirtualRequest(0, 100)); using var force = new Subject>(); + + // Stacked pipeline is heavy per notification; keep the per-subject pusher loop + // short so each iteration stays under TimeoutSeconds. + const int StackedPushCount = ItemCount / 4; + (await RunBidirectionalDeadlockTest( s => s.GroupWithImmutableState(p => p.Age % 3) .TransformMany(g => g.Items, p => p.UniqueKey) @@ -154,7 +159,7 @@ [Fact] public async Task AllDangerous_Stacked_DoNotDeadlock() .Page(pageReq), subjectPusher: () => { - for (var j = 0; j < ItemCount; j++) + for (var j = 0; j < StackedPushCount; j++) { force.OnNext(static (p, _) => true); pageReq.OnNext(new PageRequest(1 + (j % 4), 50 + (j % 4) * 50)); From 07e202f8aac2e88e7a59427d68f34ec56fd4856b Mon Sep 17 00:00:00 2001 From: Darrin Cullop Date: Wed, 27 May 2026 13:48:12 -0700 Subject: [PATCH 08/24] Raise DeadlockTortureTest per-iteration timeout to 60s Previous commit reduced the AllDangerous_Stacked pusher load to fit the 15s per-iteration budget on the CI runner. That was the wrong trade: the test is a torture test, and shaving load to match the slowest hardware costs coverage. The CI runners are deliberately stripped down; the test budget should account for them. Raise TimeoutSeconds from 15 to 60 across the fixture and restore the full ItemCount pusher loop in AllDangerous_Stacked. The timeout still catches an actual deadlock (which hangs forever, not 60s), and the extra budget covers worst-case scheduling on a small VM. --- src/DynamicData.Tests/Cache/DeadlockTortureTest.cs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs b/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs index 854ce050..7d90302f 100644 --- a/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs +++ b/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs @@ -30,7 +30,7 @@ public sealed class DeadlockTortureTest { private const int ItemCount = 200; private const int Iterations = 50; - private const int TimeoutSeconds = 15; + private const int TimeoutSeconds = 60; private static async Task RunBidirectionalDeadlockTest( Func>, IObservable>> pipeline, @@ -141,11 +141,6 @@ [Fact] public async Task AllDangerous_Stacked_DoNotDeadlock() using var pageReq = new BehaviorSubject(new PageRequest(1, 100)); using var virtReq = new BehaviorSubject(new VirtualRequest(0, 100)); using var force = new Subject>(); - - // Stacked pipeline is heavy per notification; keep the per-subject pusher loop - // short so each iteration stays under TimeoutSeconds. - const int StackedPushCount = ItemCount / 4; - (await RunBidirectionalDeadlockTest( s => s.GroupWithImmutableState(p => p.Age % 3) .TransformMany(g => g.Items, p => p.UniqueKey) @@ -159,7 +154,7 @@ [Fact] public async Task AllDangerous_Stacked_DoNotDeadlock() .Page(pageReq), subjectPusher: () => { - for (var j = 0; j < StackedPushCount; j++) + for (var j = 0; j < ItemCount; j++) { force.OnNext(static (p, _) => true); pageReq.OnNext(new PageRequest(1 + (j % 4), 50 + (j % 4) * 50)); From 1b3a03db40282eb9f3ef184cda1388397e2d5f6a Mon Sep 17 00:00:00 2001 From: Darrin Cullop Date: Thu, 28 May 2026 09:32:38 -0700 Subject: [PATCH 09/24] Apply UnsynchronizedMerge to SortAndPage/SortAndVirtualize and add focused helper coverage Address reviewer feedback on #1097. SortAndPage and SortAndVirtualize had the same shape that motivated the rest of this PR: three queue-serialized inputs combined with Observable.Merge, which reinstates the gate we removed elsewhere. Replace the Merge with UnsynchronizedMerge at both sites. SortAndPage drops the static Observable.Merge form for the extension-method form; SortAndVirtualize collapses chained .Merge().Merge() into a single UnsynchronizedMerge call, removing the second redundant gate too. DeadlockTortureTest now covers both new operators alongside the older Sort().Page() and Sort().Virtualise() forms. Each test pushes on its own subject during the race so the request branch of the merge fires under contention. MultiplePairs_Simultaneous_NoDeadlock gains two more parallel lanes (SortAndPage, SortAndVirtualize) wired through separate BehaviorSubjects so all four request streams are pushed concurrently. Add focused unit tests for the two helpers: UnsynchronizedMergeFixture covers the Rx Merge-compatible contract: arrival-order forwarding, all-must-complete OnCompleted, first-error-wins, late-terminal-after-error suppression, argument-order subscription, synchronous Empty/Throw sources at subscribe, and the no-others fallback. DeliveryQueueMergeFixture covers the same behavioural contract for the queue-backed variant plus a serialization check: two producers race 1000 items each through the merged stream while the observer asserts a max of one in-flight OnNext, with the full bag delivered exactly once. Verification: - 36/36 helper + DeadlockTortureTest pass in a single run. - DeadlockTortureTest 16/16 pass 5/5 consecutive runs at xUnit.MaxParallelThreads=16. - 422/422 affected operator tests pass. --- .../Cache/DeadlockTortureTest.cs | 26 ++ .../Internal/DeliveryQueueMergeFixture.cs | 232 ++++++++++++++++++ .../Internal/UnsynchronizedMergeFixture.cs | 175 +++++++++++++ src/DynamicData/Cache/Internal/SortAndPage.cs | 8 +- .../Cache/Internal/SortAndVirtualize.cs | 3 +- 5 files changed, 438 insertions(+), 6 deletions(-) create mode 100644 src/DynamicData.Tests/Internal/DeliveryQueueMergeFixture.cs create mode 100644 src/DynamicData.Tests/Internal/UnsynchronizedMergeFixture.cs diff --git a/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs b/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs index 7d90302f..30d2dc70 100644 --- a/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs +++ b/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs @@ -78,6 +78,14 @@ [Fact] public async Task Page_DoesNotDeadlock() subjectPusher: () => { for (var j = 0; j < ItemCount; j++) req.OnNext(new PageRequest(1 + (j % 4), 25 + (j % 4) * 25)); })).Should().BeTrue(); } + [Fact] public async Task SortAndPage_DoesNotDeadlock() + { + using var req = new BehaviorSubject(new PageRequest(1, 50)); + (await RunBidirectionalDeadlockTest( + s => s.SortAndPage(SortExpressionComparer.Ascending(p => p.Age), req), + subjectPusher: () => { for (var j = 0; j < ItemCount; j++) req.OnNext(new PageRequest(1 + (j % 4), 25 + (j % 4) * 25)); })).Should().BeTrue(); + } + [Fact] public async Task Virtualise_DoesNotDeadlock() { using var req = new BehaviorSubject(new VirtualRequest(0, 50)); @@ -86,6 +94,14 @@ [Fact] public async Task Virtualise_DoesNotDeadlock() subjectPusher: () => { for (var j = 0; j < ItemCount; j++) req.OnNext(new VirtualRequest(j * 5, 25 + (j % 4) * 25)); })).Should().BeTrue(); } + [Fact] public async Task SortAndVirtualize_DoesNotDeadlock() + { + using var req = new BehaviorSubject(new VirtualRequest(0, 50)); + (await RunBidirectionalDeadlockTest( + s => s.SortAndVirtualize(SortExpressionComparer.Ascending(p => p.Age), req), + subjectPusher: () => { for (var j = 0; j < ItemCount; j++) req.OnNext(new VirtualRequest(j * 5, 25 + (j % 4) * 25)); })).Should().BeTrue(); + } + [Fact] public async Task QueryWhenChanged_DoesNotDeadlock() { for (var iter = 0; iter < Iterations; iter++) @@ -167,7 +183,9 @@ [Fact] public async Task AllDangerous_Stacked_DoNotDeadlock() [Fact] public async Task MultiplePairs_Simultaneous_NoDeadlock() { using var pageReq = new BehaviorSubject(new PageRequest(1, 50)); + using var pageReq2 = new BehaviorSubject(new PageRequest(1, 50)); using var virtReq = new BehaviorSubject(new VirtualRequest(0, 50)); + using var virtReq2 = new BehaviorSubject(new VirtualRequest(0, 50)); using var pause = new BehaviorSubject(false); var results = await Task.WhenAll( RunBidirectionalDeadlockTest(s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)), iterations: 30), @@ -180,10 +198,18 @@ [Fact] public async Task MultiplePairs_Simultaneous_NoDeadlock() s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)).Page(pageReq), subjectPusher: () => { for (var j = 0; j < ItemCount; j++) pageReq.OnNext(new PageRequest(1 + (j % 4), 25 + (j % 4) * 25)); }, iterations: 30), + RunBidirectionalDeadlockTest( + s => s.SortAndPage(SortExpressionComparer.Ascending(p => p.Age), pageReq2), + subjectPusher: () => { for (var j = 0; j < ItemCount; j++) pageReq2.OnNext(new PageRequest(1 + (j % 4), 25 + (j % 4) * 25)); }, + iterations: 30), RunBidirectionalDeadlockTest( s => s.Sort(SortExpressionComparer.Ascending(p => p.Age)).Virtualise(virtReq), subjectPusher: () => { for (var j = 0; j < ItemCount; j++) virtReq.OnNext(new VirtualRequest(j * 5, 25 + (j % 4) * 25)); }, iterations: 30), + RunBidirectionalDeadlockTest( + s => s.SortAndVirtualize(SortExpressionComparer.Ascending(p => p.Age), virtReq2), + subjectPusher: () => { for (var j = 0; j < ItemCount; j++) virtReq2.OnNext(new VirtualRequest(j * 5, 25 + (j % 4) * 25)); }, + iterations: 30), RunBidirectionalDeadlockTest( s => s.BatchIf(pause, false, (TimeSpan?)null), subjectPusher: () => { for (var j = 0; j < ItemCount; j++) pause.OnNext(j % 2 == 0); }, diff --git a/src/DynamicData.Tests/Internal/DeliveryQueueMergeFixture.cs b/src/DynamicData.Tests/Internal/DeliveryQueueMergeFixture.cs new file mode 100644 index 00000000..fe67508a --- /dev/null +++ b/src/DynamicData.Tests/Internal/DeliveryQueueMergeFixture.cs @@ -0,0 +1,232 @@ +// Copyright (c) 2011-2025 Roland Pheasant. All rights reserved. +// Roland Pheasant licenses this file to you under the MIT license. +// See the LICENSE file in the project root for full license information. + +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using System.Reactive.Linq; +using System.Reactive.Subjects; +using System.Threading; +using System.Threading.Tasks; + +using DynamicData.Internal; + +using FluentAssertions; + +using Xunit; + +namespace DynamicData.Tests.Internal; + +/// +/// Focused behavioural tests for . +/// Verifies the Rx Merge-compatible terminal semantics and the queue's serialization guarantee +/// for concurrent producers. +/// +public sealed class DeliveryQueueMergeFixture +{ + [Fact] + public void OnNext_FromAllSources_IsForwardedInArrivalOrder() + { + using var a = new Subject(); + using var b = new Subject(); + using var c = new Subject(); + + var received = new List(); + using var sub = a.DeliveryQueueMerge(b, c).Subscribe(received.Add); + + a.OnNext(1); + b.OnNext(2); + c.OnNext(3); + a.OnNext(4); + + received.Should().Equal(1, 2, 3, 4); + } + + [Fact] + public void OnCompleted_FiresOnlyAfterEverySourceCompletes() + { + using var a = new Subject(); + using var b = new Subject(); + using var c = new Subject(); + + var completed = false; + using var sub = a.DeliveryQueueMerge(b, c).Subscribe(_ => { }, () => completed = true); + + a.OnCompleted(); + completed.Should().BeFalse(); + + b.OnCompleted(); + completed.Should().BeFalse(); + + c.OnCompleted(); + completed.Should().BeTrue(); + } + + [Fact] + public void OnError_FromAnySource_TerminatesImmediately() + { + using var a = new Subject(); + using var b = new Subject(); + + Exception? captured = null; + var completed = false; + using var sub = a.DeliveryQueueMerge(b).Subscribe(_ => { }, e => captured = e, () => completed = true); + + var error = new InvalidOperationException(); + a.OnError(error); + + captured.Should().BeSameAs(error); + completed.Should().BeFalse(); + } + + [Fact] + public void OnError_AfterFirstError_IsDroppedByQueue() + { + using var a = new Subject(); + using var b = new Subject(); + + Exception? captured = null; + using var sub = a.DeliveryQueueMerge(b).Subscribe(_ => { }, e => captured = e, () => { }); + + var first = new InvalidOperationException("first"); + var second = new InvalidOperationException("second"); + a.OnError(first); + b.OnError(second); + + captured.Should().BeSameAs(first); + } + + [Fact] + public void OnCompleted_AfterError_IsDroppedByQueue() + { + using var a = new Subject(); + using var b = new Subject(); + + Exception? captured = null; + var completed = false; + using var sub = a.DeliveryQueueMerge(b).Subscribe(_ => { }, e => captured = e, () => completed = true); + + var error = new InvalidOperationException(); + a.OnError(error); + b.OnCompleted(); + + captured.Should().BeSameAs(error); + completed.Should().BeFalse(); + } + + [Fact] + public void SynchronousTerminal_AtSubscribe_IsCountedTowardCompletion() + { + var immediate = Observable.Empty(); + using var live = new Subject(); + + var completed = false; + using var sub = immediate.DeliveryQueueMerge(live).Subscribe(_ => { }, () => completed = true); + + completed.Should().BeFalse(); + live.OnCompleted(); + completed.Should().BeTrue(); + } + + [Fact] + public void SynchronousError_AtSubscribe_PropagatesImmediately() + { + var error = new InvalidOperationException(); + var immediate = Observable.Throw(error); + using var live = new Subject(); + + Exception? captured = null; + using var sub = immediate.DeliveryQueueMerge(live).Subscribe(_ => { }, e => captured = e); + + captured.Should().BeSameAs(error); + } + + [Fact] + public async Task ConcurrentOnNext_FromManyProducers_IsSerializedToObserver() + { + // The queue's contract is that the downstream observer never sees concurrent OnNext calls, + // regardless of how many producers are racing on the inputs. Subscribe to two sources via + // two concurrent tasks, push interleaved items, and verify that no two OnNext calls overlap + // and every item is delivered exactly once. + const int itemsPerProducer = 1_000; + + using var a = new Subject(); + using var b = new Subject(); + + var inFlight = 0; + var maxInFlight = 0; + var received = new ConcurrentQueue(); + + using var sub = a.DeliveryQueueMerge(b).Subscribe(v => + { + var now = Interlocked.Increment(ref inFlight); + var prev = Volatile.Read(ref maxInFlight); + while (now > prev && Interlocked.CompareExchange(ref maxInFlight, now, prev) != prev) + { + prev = Volatile.Read(ref maxInFlight); + } + received.Enqueue(v); + Interlocked.Decrement(ref inFlight); + }); + + using var barrier = new Barrier(2); + var taskA = Task.Run(() => { barrier.SignalAndWait(); for (var i = 0; i < itemsPerProducer; i++) a.OnNext(i); }); + var taskB = Task.Run(() => { barrier.SignalAndWait(); for (var i = 0; i < itemsPerProducer; i++) b.OnNext(itemsPerProducer + i); }); + + await Task.WhenAll(taskA, taskB); + + received.Count.Should().Be(itemsPerProducer * 2); + maxInFlight.Should().Be(1, "concurrent OnNext to the observer must be serialized by the queue"); + + var expected = Enumerable.Range(0, itemsPerProducer * 2).ToHashSet(); + received.Should().BeEquivalentTo(expected); + } + + [Fact] + public void Subscription_OccursInArgumentOrder() + { + var subscribed = new List(); + var first = Observable.Create(o => { subscribed.Add(0); return () => { }; }); + var second = Observable.Create(o => { subscribed.Add(1); return () => { }; }); + var third = Observable.Create(o => { subscribed.Add(2); return () => { }; }); + + using var sub = first.DeliveryQueueMerge(second, third).Subscribe(_ => { }); + + subscribed.Should().Equal(0, 1, 2); + } + + [Fact] + public void Dispose_StopsForwardingFromAnySource() + { + using var a = new Subject(); + using var b = new Subject(); + + var received = new List(); + var sub = a.DeliveryQueueMerge(b).Subscribe(received.Add); + + a.OnNext(1); + sub.Dispose(); + a.OnNext(2); + b.OnNext(3); + + received.Should().Equal(1); + } + + [Fact] + public void NoOthers_FallsBackToFirstAlone() + { + using var a = new Subject(); + var received = new List(); + var completed = false; + using var sub = a.DeliveryQueueMerge().Subscribe(received.Add, () => completed = true); + + a.OnNext(7); + a.OnNext(11); + a.OnCompleted(); + + received.Should().Equal(7, 11); + completed.Should().BeTrue(); + } +} \ No newline at end of file diff --git a/src/DynamicData.Tests/Internal/UnsynchronizedMergeFixture.cs b/src/DynamicData.Tests/Internal/UnsynchronizedMergeFixture.cs new file mode 100644 index 00000000..85b95b8e --- /dev/null +++ b/src/DynamicData.Tests/Internal/UnsynchronizedMergeFixture.cs @@ -0,0 +1,175 @@ +// Copyright (c) 2011-2025 Roland Pheasant. All rights reserved. +// Roland Pheasant licenses this file to you under the MIT license. +// See the LICENSE file in the project root for full license information. + +using System; +using System.Collections.Generic; +using System.Reactive.Subjects; + +using DynamicData.Internal; + +using FluentAssertions; + +using Xunit; + +namespace DynamicData.Tests.Internal; + +/// +/// Focused behavioural tests for . +/// Covers the contract the helper has to honour as a drop-in +/// replacement: subscription order, all-must-complete OnCompleted, first-error-wins OnError, and synchronous terminal +/// notifications. +/// +public sealed class UnsynchronizedMergeFixture +{ + [Fact] + public void OnNext_FromBothSources_IsForwardedInArrivalOrder() + { + using var a = new Subject(); + using var b = new Subject(); + + var received = new List(); + using var sub = a.UnsynchronizedMerge(b).Subscribe(received.Add); + + a.OnNext(1); + b.OnNext(2); + a.OnNext(3); + b.OnNext(4); + + received.Should().Equal(1, 2, 3, 4); + } + + [Fact] + public void OnCompleted_FiresOnlyAfterAllSourcesComplete() + { + using var a = new Subject(); + using var b = new Subject(); + using var c = new Subject(); + + var completed = false; + using var sub = a.UnsynchronizedMerge(b, c).Subscribe(_ => { }, () => completed = true); + + a.OnCompleted(); + completed.Should().BeFalse("a single source completion must not terminate the merged stream"); + + b.OnCompleted(); + completed.Should().BeFalse("two of three completions still leave one source live"); + + c.OnCompleted(); + completed.Should().BeTrue("after every source has completed the merged stream must emit OnCompleted"); + } + + [Fact] + public void OnError_FromAnySource_TerminatesImmediately() + { + using var a = new Subject(); + using var b = new Subject(); + using var c = new Subject(); + + Exception? captured = null; + var completed = false; + using var sub = a.UnsynchronizedMerge(b, c).Subscribe(_ => { }, e => captured = e, () => completed = true); + + var error = new InvalidOperationException("first"); + b.OnError(error); + + captured.Should().BeSameAs(error); + completed.Should().BeFalse("OnCompleted must not fire after OnError"); + } + + [Fact] + public void OnError_AfterFirstError_IsIgnored() + { + using var a = new Subject(); + using var b = new Subject(); + + Exception? captured = null; + using var sub = a.UnsynchronizedMerge(b).Subscribe(_ => { }, e => captured = e, () => { }); + + var first = new InvalidOperationException("first"); + var second = new InvalidOperationException("second"); + a.OnError(first); + b.OnError(second); + + captured.Should().BeSameAs(first, "first error wins; subsequent errors from other sources must be dropped"); + } + + [Fact] + public void OnCompleted_AfterError_IsIgnored() + { + using var a = new Subject(); + using var b = new Subject(); + + Exception? captured = null; + var completed = false; + using var sub = a.UnsynchronizedMerge(b).Subscribe(_ => { }, e => captured = e, () => completed = true); + + var error = new InvalidOperationException(); + a.OnError(error); + b.OnCompleted(); + + captured.Should().BeSameAs(error); + completed.Should().BeFalse("a late OnCompleted from a surviving source must not arrive after OnError has fired"); + } + + [Fact] + public void Subscription_OccursInArgumentOrder() + { + var subscribed = new List(); + var first = System.Reactive.Linq.Observable.Create(o => { subscribed.Add(0); return () => { }; }); + var second = System.Reactive.Linq.Observable.Create(o => { subscribed.Add(1); return () => { }; }); + var third = System.Reactive.Linq.Observable.Create(o => { subscribed.Add(2); return () => { }; }); + + using var sub = first.UnsynchronizedMerge(second, third).Subscribe(_ => { }); + + subscribed.Should().Equal(0, 1, 2); + } + + [Fact] + public void SynchronousTerminal_BeforeOtherSourcesSubscribe_IsHandled() + { + // A source that completes synchronously at subscribe time decrements the pending counter immediately. + // If the helper miscounted, the merged stream would either complete prematurely or never complete. + var immediate = System.Reactive.Linq.Observable.Empty(); + using var live = new Subject(); + + var completed = false; + using var sub = immediate.UnsynchronizedMerge(live).Subscribe(_ => { }, () => completed = true); + + completed.Should().BeFalse("the live source has not completed yet"); + + live.OnCompleted(); + + completed.Should().BeTrue(); + } + + [Fact] + public void SynchronousError_BeforeOtherSourcesSubscribe_TerminatesImmediately() + { + var error = new InvalidOperationException(); + var immediate = System.Reactive.Linq.Observable.Throw(error); + using var live = new Subject(); + + Exception? captured = null; + using var sub = immediate.UnsynchronizedMerge(live).Subscribe(_ => { }, e => captured = e); + + captured.Should().BeSameAs(error); + } + + [Fact] + public void NoOthers_FallsBackToFirstAlone() + { + // Boundary: zero entries in the params array. Behaviour must mirror Observable.Merge over a single source. + using var a = new Subject(); + var received = new List(); + var completed = false; + using var sub = a.UnsynchronizedMerge().Subscribe(received.Add, () => completed = true); + + a.OnNext(7); + a.OnNext(11); + a.OnCompleted(); + + received.Should().Equal(7, 11); + completed.Should().BeTrue(); + } +} \ No newline at end of file diff --git a/src/DynamicData/Cache/Internal/SortAndPage.cs b/src/DynamicData/Cache/Internal/SortAndPage.cs index 2d612a9b..02ef996d 100644 --- a/src/DynamicData/Cache/Internal/SortAndPage.cs +++ b/src/DynamicData/Cache/Internal/SortAndPage.cs @@ -111,10 +111,10 @@ public IObservable>> Run() => return ApplyPagedChanges(changes); }); - return new CompositeDisposable(Observable.Merge( - comparerChanged.Skip(1), - paramsChanged.Where(changes => changes.Count is not 0), - dataChange.Where(changes => changes.Count is not 0)) + return new CompositeDisposable(comparerChanged.Skip(1) + .UnsynchronizedMerge( + paramsChanged.Where(changes => changes.Count is not 0), + dataChange.Where(changes => changes.Count is not 0)) .SubscribeSafe(observer), queue); ChangeSet> ApplyPagedChanges(IChangeSet? changeSet = null) diff --git a/src/DynamicData/Cache/Internal/SortAndVirtualize.cs b/src/DynamicData/Cache/Internal/SortAndVirtualize.cs index 44c6d3dc..0e6ae550 100644 --- a/src/DynamicData/Cache/Internal/SortAndVirtualize.cs +++ b/src/DynamicData/Cache/Internal/SortAndVirtualize.cs @@ -113,8 +113,7 @@ public IObservable>> Run() => return new CompositeDisposable( comparerChanged - .Merge(paramsChanged) - .Merge(dataChange) + .UnsynchronizedMerge(paramsChanged, dataChange) .Where(changes => changes.Count is not 0) .SubscribeSafe(observer), queue); From 07a38237fcae1bdb50aecc000b0fd77f3793c271 Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Sat, 30 May 2026 10:28:58 -0700 Subject: [PATCH 10/24] Refactor comments and improve observer handling Updated comments in `DeliveryQueueMergeExtensions` and `SynchronizeSafeExtensions` for clarity. Renamed `pending` to `remainingSources` for better readability. Adjusted `CompositeDisposable` order to ensure correct processing of terminal notifications. Ensured each source in `UnsynchronizedMerge` has its own observer instance. --- .../Internal/DeliveryQueueMergeExtensions.cs | 77 ++++----- .../Internal/SynchronizeSafeExtensions.cs | 147 ++++++++---------- 2 files changed, 92 insertions(+), 132 deletions(-) diff --git a/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs b/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs index 9adefaeb..43b3d94b 100644 --- a/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs +++ b/src/DynamicData/Internal/DeliveryQueueMergeExtensions.cs @@ -8,72 +8,51 @@ namespace DynamicData.Internal; -/// -/// Provides DeliveryQueueMerge, an Rx extension method that serializes the -/// notifications of every input through a single -/// and emits them on the downstream observer outside the queue's lock. -/// -/// -/// Drop-in alternative to -/// for cross-cache pipelines where the Rx Merge gate, held during downstream delivery, -/// would risk an ABBA cycle. serializes enqueues across -/// concurrent producers but releases its gate before delivering, so a downstream -/// observer that walks into another cache's writer lock cannot deadlock with this -/// operator's serialization point. -/// Every input must share the same element type. When the inputs have different -/// element types or require operator-private projections invoked inside the queue's -/// drain, use -/// with a and finish with -/// . -/// +// Same-type Rx merge that owns a DeliveryQueue. Serializes notifications from +// every input through the queue, which releases its gate before delivering, so +// downstream observers that walk into another cache's writer lock cannot deadlock +// with this operator's serialization point. Used where every input has the same +// element type and no per-input projection is needed inside the drain. When element +// types differ or per-input projections are required, route each input through +// SharedDeliveryQueue with SynchronizeSafe and combine them with UnsynchronizedMerge. internal static class DeliveryQueueMergeExtensions { - /// - /// Merges with by routing every - /// source through a single . Functionally equivalent - /// to : completes - /// only after every source completes; the first error terminates; subscription - /// occurs in argument order. - /// - /// The element type, common to every input. - /// The first input observable. - /// Additional input observables. - /// An observable that emits items from every input, serialized through the queue. + // Functionally equivalent to Observable.Merge: completes only after every source + // completes, the first error terminates, subscription occurs in argument order. public static IObservable DeliveryQueueMerge(this IObservable first, params IObservable[] others) => Observable.Create(observer => { var queue = new DeliveryQueue(observer); - var totalSources = others.Length + 1; - var subscriptions = new CompositeDisposable(totalSources + 1); - var pending = totalSources; + var remainingSources = others.Length + 1; + var subscriptions = new CompositeDisposable(remainingSources + 1); + + subscriptions.Add(first.SubscribeSafe(CreateInner())); + foreach (var source in others) + { + subscriptions.Add(source.SubscribeSafe(CreateInner())); + } + + // Subscription first so any terminal notification produced during Rx's disposal + // cascade still flows through the still-active queue. Queue last as cleanup. + subscriptions.Add(queue); + return subscriptions; // Each source needs its own inner observer instance because Rx's ObserverBase - // sets a one-shot stopped flag on the first OnCompleted/OnError; a single shared - // observer would silently drop terminal notifications from every source after - // the first. OnNext and OnError forward straight to the queue (the queue's gate - // serializes concurrent calls); OnCompleted is counter-gated so only the last - // surviving source's completion terminates the merged stream. + // sets a one-shot stopped flag on the first OnCompleted or OnError. A single + // shared observer would silently drop terminal notifications from every source + // after the first. OnNext and OnError forward straight to the queue (the queue's + // gate serializes concurrent calls). OnCompleted is counter-gated so only the + // last surviving source's completion terminates the merged stream. IObserver CreateInner() => Observer.Create( queue.OnNext, queue.OnError, () => { - if (Interlocked.Decrement(ref pending) == 0) + if (Interlocked.Decrement(ref remainingSources) == 0) { queue.OnCompleted(); } }); - - subscriptions.Add(first.SubscribeSafe(CreateInner())); - foreach (var source in others) - { - subscriptions.Add(source.SubscribeSafe(CreateInner())); - } - - // Subscription first so any terminal notification produced during Rx's disposal - // cascade still flows through the still-active queue. Queue last as cleanup. - subscriptions.Add(queue); - return subscriptions; }); } diff --git a/src/DynamicData/Internal/SynchronizeSafeExtensions.cs b/src/DynamicData/Internal/SynchronizeSafeExtensions.cs index 283dfff5..7f1e06f9 100644 --- a/src/DynamicData/Internal/SynchronizeSafeExtensions.cs +++ b/src/DynamicData/Internal/SynchronizeSafeExtensions.cs @@ -8,50 +8,39 @@ namespace DynamicData.Internal; -/// -/// Provides SynchronizeSafe extension methods, drop-in replacements -/// for Synchronize(lock) that release the lock before downstream delivery. -/// -/// -/// Disposal ordering matters. CompositeDisposable disposes in -/// declaration order. The queue and the source subscription have different roles: -/// -/// -/// Subscription-first (gate and SDQ overloads) -/// The queue is the IObserver that the source sends notifications to. -/// Disposing the subscription first allows any final terminal notification (OnCompleted/OnError -/// triggered by Rx's disposal cascade or a Finally operator) to flow through the -/// still-active queue. The queue is disposed last as cleanup. -/// -/// -/// Queue-first (parameterless overload) -/// Used by operators with teardown side effects (DisposeMany, OnBeingRemoved). -/// The queue is terminated first via , which ensures -/// all in-flight deliveries complete before the subscription is disposed and teardown logic -/// (e.g., disposing removed items) runs. Terminal notifications are not needed because -/// the subscriber is explicitly tearing down. -/// -/// -/// +// Drop-in replacements for Observable.Synchronize(lock) that release the lock before +// downstream delivery, plus UnsynchronizedMerge for combining streams whose inputs are +// already serialized through the same queue. +// +// Disposal ordering matters. CompositeDisposable disposes in declaration order, and the +// queue and the source subscription have different roles: +// +// Subscription-first (gate and SDQ overloads): the queue is the IObserver that the +// source sends notifications to. Disposing the subscription first allows any final +// terminal notification (OnCompleted or OnError triggered by Rx's disposal cascade +// or a Finally operator) to flow through the still-active queue. The queue is +// disposed last as cleanup. +// +// Queue-first (parameterless overload): used by operators with teardown side effects +// (DisposeMany, OnBeingRemoved). The queue is terminated first via DeliveryQueue.Dispose, +// which ensures all in-flight deliveries complete before the subscription is disposed +// and teardown logic (e.g. disposing removed items) runs. Terminal notifications are +// not needed because the subscriber is explicitly tearing down. internal static class SynchronizeSafeExtensions { - /// - /// Synchronizes the source observable through a . - /// Use when multiple sources of different types share a gate. - /// + // Routes the source through a SharedDeliveryQueue. Use when multiple sources of + // different types share a gate. public static IObservable SynchronizeSafe(this IObservable source, SharedDeliveryQueue queue) => Observable.Create(observer => { var subQueue = queue.CreateQueue(observer); - // Subscription first: terminal notifications flow through the still-active sub-queue + // Subscription first: terminal notifications flow through the still-active sub-queue. return new CompositeDisposable(source.SubscribeSafe(subQueue), subQueue); }); - /// - /// Synchronizes the source observable through an implicitly created . - /// Drop-in replacement for Synchronize(locker). - /// + // Routes the source through an implicitly created DeliveryQueue. Drop-in replacement + // for Observable.Synchronize(locker). #if NET9_0_OR_GREATER public static IObservable SynchronizeSafe(this IObservable source, Lock gate) => #else @@ -61,60 +50,62 @@ public static IObservable SynchronizeSafe(this IObservable source, obje { var queue = new DeliveryQueue(gate, observer); - // Subscription first: terminal notifications flow through the still-active queue + // Subscription first: terminal notifications flow through the still-active queue. return new CompositeDisposable(source.SubscribeSafe(queue), queue); }); - /// - /// Synchronizes the source observable through an implicitly created - /// with automatic delivery completion on dispose. The queue is terminated and drained - /// before the source subscription is disposed, ensuring all in-flight notifications - /// are delivered before teardown. - /// + // Routes the source through an implicitly created DeliveryQueue with automatic + // delivery completion on dispose. The queue is terminated and drained before the + // source subscription is disposed, ensuring all in-flight notifications are delivered + // before teardown. public static IObservable SynchronizeSafe(this IObservable source) => Observable.Create(observer => { var queue = new DeliveryQueue(observer); - // Queue first: ensures in-flight deliveries complete before teardown side effects run + // Queue first: ensures in-flight deliveries complete before teardown side effects run. return new CompositeDisposable(queue, source.SubscribeSafe(queue)); }); - /// - /// Merges with into a single observable - /// without taking any synchronization gate. Functionally equivalent to - /// : completes only after - /// every source completes; the first error terminates; subscription occurs in argument order. - /// - /// - /// The caller MUST ensure that delivery from every source is already serialized. - /// In this library the precondition is satisfied by routing every source through the - /// same via - /// . The shared - /// queue's drain loop guarantees that at most one notification is in flight to the - /// downstream observer at a time, so the additional gate that Observable.Merge - /// would install is redundant. - /// Removing that gate matters in cross-cache pipelines: Observable.Merge - /// holds its private _gate for the entire duration of downstream delivery, and - /// when downstream delivery walks into another cache's writer lock, two such gates on - /// two operators form an ABBA cycle that the queue-drain design is meant to prevent. - /// Each source is subscribed through its own - /// instance. The actions close over shared pending and terminated counters, but - /// the observer instances must be distinct because Rx's ObserverBase sets a one-shot - /// stopped flag on the first OnCompleted/OnError; a single shared observer - /// would silently drop terminal notifications from every source after the first. - /// Without the external serialization precondition, concurrent OnNext - /// calls into the shared observer will race. Do not use as a general-purpose - /// Observable.Merge replacement. - /// + // Merges every input into a single observable without taking any synchronization gate. + // Functionally equivalent to Observable.Merge: completes only after every source completes, + // the first error terminates, subscription occurs in argument order. + // + // The caller MUST ensure that delivery from every source is already serialized. In this + // library the precondition is satisfied by routing every source through the same + // SharedDeliveryQueue via SynchronizeSafe(queue). The shared queue's drain loop guarantees + // that at most one notification is in flight to the downstream observer at a time, so the + // additional gate that Observable.Merge would install is redundant. + // + // Removing that gate matters in cross-cache pipelines: Observable.Merge holds its private + // _gate for the entire duration of downstream delivery, and when downstream delivery walks + // into another cache's writer lock, two such gates on two operators form an ABBA cycle that + // the queue-drain design is meant to prevent. + // + // Without the external serialization precondition, concurrent OnNext calls into the shared + // observer will race. Do not use as a general-purpose Observable.Merge replacement. public static IObservable UnsynchronizedMerge(this IObservable first, params IObservable[] others) => Observable.Create(observer => { - var totalSources = others.Length + 1; - var subscriptions = new CompositeDisposable(totalSources); - var pending = totalSources; + var remainingSources = others.Length + 1; + var subscriptions = new CompositeDisposable(remainingSources); var terminated = 0; + subscriptions.Add(first.SubscribeSafe(CreateInner())); + foreach (var source in others) + { + subscriptions.Add(source.SubscribeSafe(CreateInner())); + } + + return subscriptions; + + // Each source needs its own inner observer instance because Rx's ObserverBase sets + // a one-shot stopped flag on the first OnCompleted or OnError. A single shared + // observer would silently drop terminal notifications from every source after the + // first. The OnNext/OnError/OnCompleted actions close over the shared remainingSources + // and terminated counters so cross-source coordination still works. + IObserver CreateInner() => Observer.Create(OnNextSafe, OnErrorSafe, OnCompletedSafe); + void OnNextSafe(T value) { if (Volatile.Read(ref terminated) == 0) @@ -133,20 +124,10 @@ void OnErrorSafe(Exception error) void OnCompletedSafe() { - if (Interlocked.Decrement(ref pending) == 0 && Interlocked.Exchange(ref terminated, 1) == 0) + if (Interlocked.Decrement(ref remainingSources) == 0 && Interlocked.Exchange(ref terminated, 1) == 0) { observer.OnCompleted(); } } - - IObserver CreateInner() => Observer.Create(OnNextSafe, OnErrorSafe, OnCompletedSafe); - - subscriptions.Add(first.SubscribeSafe(CreateInner())); - foreach (var source in others) - { - subscriptions.Add(source.SubscribeSafe(CreateInner())); - } - - return subscriptions; }); } From 30d54001c40467175f500f0c57a9116b511146b0 Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Sat, 30 May 2026 13:17:44 -0700 Subject: [PATCH 11/24] Extend Rx gate elimination to 6 additional cache operators Adds UnsynchronizedCombineLatest as a two-input drop-in for Observable.CombineLatest that does not install a gate. Same precondition as UnsynchronizedMerge: both inputs must be pre-serialized through the same SharedDeliveryQueue. Applies UnsynchronizedMerge / UnsynchronizedCombineLatest at six previously-unaddressed sites whose inputs were already queue-serialized but still combined through Rx's gate-holding combinators: GroupOn groups.UnsynchronizedMerge(regroup) GroupOnDynamic three-source UnsynchronizedMerge for completion signal TransformAsync transformer.UnsynchronizedMerge(forced) (forceTransform branch) TransformMany initial.UnsynchronizedMerge(subsequent) (childChanges path) TreeBuilder predicateChanged.UnsynchronizedCombineLatest(reFilterObservable) (reFilterObservable now also routed through the queue) Switch inlined switch logic with SerialDisposable plus UnsynchronizedMerge of destination.Connect and the errors subject Removes redundant Synchronize() from EditDiffChangeSetOptional. Per the Rx contract (B7 and C5) a single-source operator must not redundantly serialize; the source already serializes per A2. The sister class EditDiffChangeSet already follows this rule. Indentation cleaned up while there. Adds UnsynchronizedCombineLatestFixture matching the depth of UnsynchronizedMergeFixture. Adds five DeadlockTortureTest cases covering the newly-fixed sites: GroupOnWithRegrouper, GroupOnDynamicSelector, TransformAsyncWithForce, TransformToTree, Switch. --- .../Cache/DeadlockTortureTest.cs | 66 ++++++ .../UnsynchronizedCombineLatestFixture.cs | 217 ++++++++++++++++++ .../Internal/EditDiffChangeSetOptional.cs | 50 ++-- src/DynamicData/Cache/Internal/GroupOn.cs | 4 +- .../Cache/Internal/GroupOnDynamic.cs | 9 +- src/DynamicData/Cache/Internal/Switch.cs | 40 ++-- .../Cache/Internal/TransformAsync.cs | 4 +- .../Cache/Internal/TransformMany.cs | 3 +- src/DynamicData/Cache/Internal/TreeBuilder.cs | 7 +- .../Internal/SynchronizeSafeExtensions.cs | 82 +++++++ 10 files changed, 435 insertions(+), 47 deletions(-) create mode 100644 src/DynamicData.Tests/Internal/UnsynchronizedCombineLatestFixture.cs diff --git a/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs b/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs index 30d2dc70..4969eaed 100644 --- a/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs +++ b/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs @@ -70,6 +70,38 @@ [Fact] public async Task GroupOn_DoesNotDeadlock() => [Fact] public async Task GroupWithImmutableState_DoesNotDeadlock() => (await RunBidirectionalDeadlockTest(s => s.GroupWithImmutableState(p => p.Age % 3).TransformMany(g => g.Items, p => p.UniqueKey))).Should().BeTrue(); + [Fact] public async Task GroupOnWithRegrouper_DoesNotDeadlock() + { + using var regrouper = new System.Reactive.Subjects.Subject(); + (await RunBidirectionalDeadlockTest( + s => s.Group(p => p.Age % 3, regrouper).MergeMany(g => g.Cache.Connect()), + subjectPusher: () => { for (var j = 0; j < ItemCount; j++) regrouper.OnNext(System.Reactive.Unit.Default); })).Should().BeTrue(); + } + + [Fact] public async Task GroupOnDynamicSelector_DoesNotDeadlock() + { + using var selector = new BehaviorSubject>((p, _) => p.Age % 3); + using var regrouper = new System.Reactive.Subjects.Subject(); + (await RunBidirectionalDeadlockTest( + s => s.Group(selector, regrouper).MergeMany(g => g.Cache.Connect()), + subjectPusher: () => + { + for (var j = 0; j < ItemCount; j++) + { + selector.OnNext((p, _) => p.Age % (2 + (j % 4))); + regrouper.OnNext(System.Reactive.Unit.Default); + } + })).Should().BeTrue(); + } + + [Fact] public async Task TransformAsyncWithForce_DoesNotDeadlock() + { + using var force = new System.Reactive.Subjects.Subject>(); + (await RunBidirectionalDeadlockTest( + s => s.TransformAsync(p => Task.FromResult(new Person("T-" + p.Name, p.Age)), force), + subjectPusher: () => { for (var j = 0; j < ItemCount; j++) force.OnNext(static (_, _) => true); })).Should().BeTrue(); + } + [Fact] public async Task Page_DoesNotDeadlock() { using var req = new BehaviorSubject(new PageRequest(1, 50)); @@ -240,4 +272,38 @@ [Fact] public async Task ThreeWayCircular_DoesNotDeadlock() (await Task.WhenAny(completed, Task.Delay(TimeSpan.FromSeconds(TimeoutSeconds)))).Should().BeSameAs(completed, "iteration " + iter); } } + + [Fact] public async Task TransformToTree_DoesNotDeadlock() + { + // Exercises TreeBuilder.cs:200 (_predicateChanged.SynchronizeSafe(queue).UnsynchronizedCombineLatest + // (reFilterObservable.SynchronizeSafe(queue), ...)). Cross-cache cycle is closed via a side-channel + // Subscribe that writes a marker into the other cache for every tree changeset. + for (var iter = 0; iter < Iterations; iter++) + { + using var sourceA = new SourceCache(p => p.UniqueKey); + using var sourceB = new SourceCache(p => p.UniqueKey); + + // The pivotOn function returns the parent's key (or the item's own key for roots). Half the + // items become children of "A-{iter}-0" / "B-{iter}-0", populating the inner tree structure. + using var aToB = sourceA.Connect() + .TransformToTree(p => p.Age == 0 ? p.UniqueKey : "A-" + iter + "-0") + .Subscribe(_ => sourceB.AddOrUpdate(new Person("from-a-tree-" + iter, 0))); + using var bToA = sourceB.Connect() + .TransformToTree(p => p.Age == 0 ? p.UniqueKey : "B-" + iter + "-0") + .Subscribe(_ => sourceA.AddOrUpdate(new Person("from-b-tree-" + iter, 0))); + + using var barrier = new Barrier(2); + var taskA = Task.Run(() => { barrier.SignalAndWait(); for (var i = 0; i < ItemCount; i++) sourceA.AddOrUpdate(new Person("A-" + iter + "-" + i, i)); }); + var taskB = Task.Run(() => { barrier.SignalAndWait(); for (var i = 0; i < ItemCount; i++) sourceB.AddOrUpdate(new Person("B-" + iter + "-" + i, i)); }); + + var completed = Task.WhenAll(taskA, taskB); + (await Task.WhenAny(completed, Task.Delay(TimeSpan.FromSeconds(TimeoutSeconds)))).Should().BeSameAs(completed, "iteration " + iter); + } + } + + [Fact] public async Task Switch_DoesNotDeadlock() => + // Exercises the refactored Switch.cs (SerialDisposable + UnsynchronizedMerge of destination.Connect() + // and the errors subject). Observable.Return(s).Switch() drives exactly one outer notification, which + // is enough to wire up the destination cache and exercise the gate-free merge on every inner change. + (await RunBidirectionalDeadlockTest(s => System.Reactive.Linq.Observable.Return(s).Switch())).Should().BeTrue(); } diff --git a/src/DynamicData.Tests/Internal/UnsynchronizedCombineLatestFixture.cs b/src/DynamicData.Tests/Internal/UnsynchronizedCombineLatestFixture.cs new file mode 100644 index 00000000..25693721 --- /dev/null +++ b/src/DynamicData.Tests/Internal/UnsynchronizedCombineLatestFixture.cs @@ -0,0 +1,217 @@ +// Copyright (c) 2011-2025 Roland Pheasant. All rights reserved. +// Roland Pheasant licenses this file to you under the MIT license. +// See the LICENSE file in the project root for full license information. + +using System; +using System.Collections.Generic; +using System.Reactive.Subjects; + +using DynamicData.Internal; + +using FluentAssertions; + +using Xunit; + +namespace DynamicData.Tests.Internal; + +/// +/// Focused behavioural tests for . +/// Covers the contract the helper has to honour as a drop-in +/// replacement: emits only after both sources have produced at least one value, then on every subsequent OnNext from either side; first error terminates; +/// completes only after both sources complete. +/// +public sealed class UnsynchronizedCombineLatestFixture +{ + [Fact] + public void OnNext_DoesNotEmit_UntilBothSourcesHaveProduced() + { + using var a = new Subject(); + using var b = new Subject(); + + var received = new List(); + using var sub = a.UnsynchronizedCombineLatest(b, (x, y) => $"{x}:{y}").Subscribe(received.Add); + + a.OnNext(1); + received.Should().BeEmpty("only the first source has produced"); + + a.OnNext(2); + received.Should().BeEmpty("the second source still has not produced"); + + b.OnNext("first"); + received.Should().Equal("2:first"); + } + + [Fact] + public void OnNext_AfterBothHaveProduced_EmitsOnEverySubsequentValue() + { + using var a = new Subject(); + using var b = new Subject(); + + var received = new List(); + using var sub = a.UnsynchronizedCombineLatest(b, (x, y) => $"{x}:{y}").Subscribe(received.Add); + + a.OnNext(1); + b.OnNext("x"); + a.OnNext(2); + b.OnNext("y"); + b.OnNext("z"); + a.OnNext(3); + + received.Should().Equal("1:x", "2:x", "2:y", "2:z", "3:z"); + } + + [Fact] + public void OnCompleted_FiresOnlyAfterBothSourcesComplete() + { + using var a = new Subject(); + using var b = new Subject(); + + var completed = false; + using var sub = a.UnsynchronizedCombineLatest(b, (x, y) => $"{x}:{y}").Subscribe(_ => { }, () => completed = true); + + a.OnCompleted(); + completed.Should().BeFalse("the second source is still live"); + + b.OnCompleted(); + completed.Should().BeTrue(); + } + + [Fact] + public void OnError_FromAnySource_TerminatesImmediately() + { + using var a = new Subject(); + using var b = new Subject(); + + Exception? captured = null; + var completed = false; + using var sub = a.UnsynchronizedCombineLatest(b, (x, y) => $"{x}:{y}").Subscribe(_ => { }, e => captured = e, () => completed = true); + + var error = new InvalidOperationException("first"); + b.OnError(error); + + captured.Should().BeSameAs(error); + completed.Should().BeFalse("OnCompleted must not fire after OnError"); + } + + [Fact] + public void OnError_AfterFirstError_IsIgnored() + { + using var a = new Subject(); + using var b = new Subject(); + + Exception? captured = null; + using var sub = a.UnsynchronizedCombineLatest(b, (x, y) => $"{x}:{y}").Subscribe(_ => { }, e => captured = e, () => { }); + + var first = new InvalidOperationException("first"); + var second = new InvalidOperationException("second"); + a.OnError(first); + b.OnError(second); + + captured.Should().BeSameAs(first, "first error wins; subsequent errors from other sources must be dropped"); + } + + [Fact] + public void OnNext_AfterError_IsIgnored() + { + using var a = new Subject(); + using var b = new Subject(); + + var received = new List(); + Exception? captured = null; + using var sub = a.UnsynchronizedCombineLatest(b, (x, y) => $"{x}:{y}").Subscribe(received.Add, e => captured = e); + + a.OnNext(1); + b.OnNext("x"); + received.Should().Equal("1:x"); + + var error = new InvalidOperationException(); + a.OnError(error); + + a.OnNext(2); + b.OnNext("y"); + received.Should().Equal(new[] { "1:x" }, "no further OnNext must arrive after OnError has fired"); + captured.Should().BeSameAs(error); + } + + [Fact] + public void OnCompleted_AfterError_IsIgnored() + { + using var a = new Subject(); + using var b = new Subject(); + + Exception? captured = null; + var completed = false; + using var sub = a.UnsynchronizedCombineLatest(b, (x, y) => $"{x}:{y}").Subscribe(_ => { }, e => captured = e, () => completed = true); + + var error = new InvalidOperationException(); + a.OnError(error); + b.OnCompleted(); + + captured.Should().BeSameAs(error); + completed.Should().BeFalse("a late OnCompleted from a surviving source must not arrive after OnError"); + } + + [Fact] + public void ResultSelector_ReceivesMostRecentValueFromEachSource() + { + using var a = new Subject(); + using var b = new Subject(); + + var received = new List(); + using var sub = a.UnsynchronizedCombineLatest(b, (x, y) => x * 10 + y).Subscribe(received.Add); + + a.OnNext(1); + b.OnNext(2); + received.Should().Equal(12); + + a.OnNext(3); + received.Should().Equal(new[] { 12, 32 }, "the second source's most recent value (2) must still be in effect"); + + b.OnNext(4); + received.Should().Equal(12, 32, 34); + } + + [Fact] + public void SynchronousValues_AtSubscribeTime_AreCombinedCorrectly() + { + // Behaviour subjects deliver their initial value synchronously at Subscribe time. + // The helper must capture the first source's value before subscribing to the second, + // and immediately emit when the second source's initial value arrives. + using var a = new System.Reactive.Subjects.BehaviorSubject(7); + using var b = new System.Reactive.Subjects.BehaviorSubject(11); + + var received = new List(); + using var sub = a.UnsynchronizedCombineLatest(b, (x, y) => x + y).Subscribe(received.Add); + + received.Should().Equal(new[] { 18 }, "both subjects delivered synchronously at subscribe time"); + } + + [Fact] + public void SynchronousCompletion_BeforeOther_StillCompletesOnlyAfterBoth() + { + var immediate = System.Reactive.Linq.Observable.Empty(); + using var live = new Subject(); + + var completed = false; + using var sub = immediate.UnsynchronizedCombineLatest(live, (x, y) => x + y).Subscribe(_ => { }, () => completed = true); + + completed.Should().BeFalse("the live source has not completed yet"); + + live.OnCompleted(); + + completed.Should().BeTrue(); + } + + [Fact] + public void SynchronousError_BeforeOther_TerminatesImmediately() + { + var error = new InvalidOperationException(); + var immediate = System.Reactive.Linq.Observable.Throw(error); + using var live = new Subject(); + + Exception? captured = null; + using var sub = immediate.UnsynchronizedCombineLatest(live, (x, y) => x + y).Subscribe(_ => { }, e => captured = e); + + captured.Should().BeSameAs(error); + } +} diff --git a/src/DynamicData/Cache/Internal/EditDiffChangeSetOptional.cs b/src/DynamicData/Cache/Internal/EditDiffChangeSetOptional.cs index f65df1b9..17afafeb 100644 --- a/src/DynamicData/Cache/Internal/EditDiffChangeSetOptional.cs +++ b/src/DynamicData/Cache/Internal/EditDiffChangeSetOptional.cs @@ -17,35 +17,35 @@ internal sealed class EditDiffChangeSetOptional(IObservable _keySelector = keySelector ?? throw new ArgumentNullException(nameof(keySelector)); public IObservable> Run() => Observable.Create>(observer => - { - var previous = Optional.None(); + { + var previous = Optional.None(); - return _source.Synchronize().Subscribe( - nextValue => - { - var current = nextValue.Convert(val => new ValueContainer(val, _keySelector(val))); + return _source.Subscribe( + nextValue => + { + var current = nextValue.Convert(val => new ValueContainer(val, _keySelector(val))); - // Determine the changes - var changes = (previous.HasValue, current.HasValue) switch - { - (true, true) => CreateUpdateChanges(previous.Value, current.Value), - (false, true) => [new Change(ChangeReason.Add, current.Value.Key, current.Value.Object)], - (true, false) => [new Change(ChangeReason.Remove, previous.Value.Key, previous.Value.Object)], - (false, false) => [], - }; + // Determine the changes + var changes = (previous.HasValue, current.HasValue) switch + { + (true, true) => CreateUpdateChanges(previous.Value, current.Value), + (false, true) => [new Change(ChangeReason.Add, current.Value.Key, current.Value.Object)], + (true, false) => [new Change(ChangeReason.Remove, previous.Value.Key, previous.Value.Object)], + (false, false) => [], + }; - // Save the value for the next round - previous = current; + // Save the value for the next round + previous = current; - // If there are changes, emit as a ChangeSet - if (changes.Length > 0) - { - observer.OnNext(new ChangeSet(changes)); - } - }, - observer.OnError, - observer.OnCompleted); - }); + // If there are changes, emit as a ChangeSet + if (changes.Length > 0) + { + observer.OnNext(new ChangeSet(changes)); + } + }, + observer.OnError, + observer.OnCompleted); + }); private Change[] CreateUpdateChanges(in ValueContainer prev, in ValueContainer curr) { diff --git a/src/DynamicData/Cache/Internal/GroupOn.cs b/src/DynamicData/Cache/Internal/GroupOn.cs index fd0936fe..546e2e8f 100644 --- a/src/DynamicData/Cache/Internal/GroupOn.cs +++ b/src/DynamicData/Cache/Internal/GroupOn.cs @@ -6,6 +6,8 @@ using System.Reactive.Disposables; using System.Reactive.Linq; +using DynamicData.Internal; + namespace DynamicData.Cache.Internal; internal sealed class GroupOn(IObservable> source, Func groupSelectorKey, IObservable? regrouper) @@ -29,7 +31,7 @@ public IObservable> Run() => Observabl var regroup = _regrouper.SynchronizeSafe(queue).Select(_ => grouper.Regroup()).Where(changes => changes.Count != 0); - var published = groups.Merge(regroup).Publish(); + var published = groups.UnsynchronizedMerge(regroup).Publish(); var subscriber = published.SubscribeSafe(observer); var disposer = published.DisposeMany().Subscribe(); diff --git a/src/DynamicData/Cache/Internal/GroupOnDynamic.cs b/src/DynamicData/Cache/Internal/GroupOnDynamic.cs index 0e11bc59..91efad22 100644 --- a/src/DynamicData/Cache/Internal/GroupOnDynamic.cs +++ b/src/DynamicData/Cache/Internal/GroupOnDynamic.cs @@ -6,6 +6,8 @@ using System.Reactive.Disposables; using System.Reactive.Linq; +using DynamicData.Internal; + namespace DynamicData.Cache.Internal; internal sealed class GroupOnDynamic(IObservable> source, IObservable> selectGroupObservable, IObservable? regrouper = null) @@ -71,8 +73,11 @@ public IObservable> Run() => Observabl }, onError: observer.OnError); - // Create an observable that completes when all 3 inputs complete so the downstream can be completed as well - var subOnComplete = Observable.Merge(sharedSource.ToUnit(), sharedGroupSelector.ToUnit(), sharedRegrouper) + // All three inputs are routed through the same SharedDeliveryQueue so their notifications + // are already serialized; the merge is only here to coalesce their completion into a single + // downstream OnCompleted. UnsynchronizedMerge avoids the ABBA-prone gate that Observable.Merge + // would hold across the downstream observer.OnCompleted/observer.OnError call. + var subOnComplete = sharedSource.ToUnit().UnsynchronizedMerge(sharedGroupSelector.ToUnit(), sharedRegrouper) .IgnoreElements() .SubscribeSafe(observer.OnError, observer.OnCompleted); diff --git a/src/DynamicData/Cache/Internal/Switch.cs b/src/DynamicData/Cache/Internal/Switch.cs index 766f9f14..d5c2302d 100644 --- a/src/DynamicData/Cache/Internal/Switch.cs +++ b/src/DynamicData/Cache/Internal/Switch.cs @@ -6,6 +6,8 @@ using System.Reactive.Linq; using System.Reactive.Subjects; +using DynamicData.Internal; + namespace DynamicData.Cache.Internal; internal sealed class Switch(IObservable>> sources) @@ -18,29 +20,35 @@ public IObservable> Run() => Observable.Create { var queue = new SharedDeliveryQueue(); - var destination = new LockFreeObservableCache(); - var errors = new Subject>(); - - var populator = Observable.Switch( - _sources - .SynchronizeSafe(queue) - .Do(onNext: _ => destination.Clear(), - onError: error => errors.OnError(error))) + var innerSubscription = new SerialDisposable(); + + // The outer (sources) and every inner are routed through the same SharedDeliveryQueue. + // Both the per-source clear and the per-changeset destination write happen on the drain + // thread, so destination.Connect() emissions and any errors.OnError calls also originate + // from inside the drain. The downstream merge therefore sees pre-serialized inputs and + // uses UnsynchronizedMerge to avoid the ABBA-prone Observable.Merge gate. + var sourcesSubscription = _sources .SynchronizeSafe(queue) - .Do(onNext: static _ => { }, - onError: error => errors.OnError(error)) - .PopulateInto(destination); + .SubscribeSafe( + onNext: newSource => + { + destination.Clear(); + innerSubscription.Disposable = newSource + .SynchronizeSafe(queue) + .SubscribeSafe( + onNext: changes => destination.Edit(updater => updater.Clone(changes)), + onError: errors.OnError); + }, + onError: errors.OnError); return new CompositeDisposable( destination, errors, - populator, - destination - .Connect() - .Merge(errors) - .SubscribeSafe(observer), + sourcesSubscription, + innerSubscription, + destination.Connect().UnsynchronizedMerge(errors).SubscribeSafe(observer), queue); }); } diff --git a/src/DynamicData/Cache/Internal/TransformAsync.cs b/src/DynamicData/Cache/Internal/TransformAsync.cs index 73b6f62a..66918d9e 100644 --- a/src/DynamicData/Cache/Internal/TransformAsync.cs +++ b/src/DynamicData/Cache/Internal/TransformAsync.cs @@ -6,6 +6,8 @@ using System.Reactive.Linq; using System.Reactive.Threading.Tasks; +using DynamicData.Internal; + namespace DynamicData.Cache.Internal; internal class TransformAsync( @@ -32,7 +34,7 @@ public IObservable> Run() => var forced = forceTransform.SynchronizeSafe(queue) .Select(shouldTransform => DoTransform(cache, shouldTransform)).Concat(); - transformer = transformer.SynchronizeSafe(queue).Merge(forced); + transformer = transformer.SynchronizeSafe(queue).UnsynchronizedMerge(forced); return new CompositeDisposable(transformer.SubscribeSafe(observer), queue); } diff --git a/src/DynamicData/Cache/Internal/TransformMany.cs b/src/DynamicData/Cache/Internal/TransformMany.cs index f38853c0..a356d09c 100644 --- a/src/DynamicData/Cache/Internal/TransformMany.cs +++ b/src/DynamicData/Cache/Internal/TransformMany.cs @@ -8,6 +8,7 @@ using System.Reactive.Linq; using DynamicData.Binding; +using DynamicData.Internal; namespace DynamicData.Cache.Internal; @@ -122,7 +123,7 @@ private IObservable> CreateWithChangeS var subsequent = transformed.MergeMany(x => x.Changes).SynchronizeSafe(queue); - var allChanges = initial.Merge(subsequent).Select( + var allChanges = initial.UnsynchronizedMerge(subsequent).Select( changes => { result.Clone(changes); diff --git a/src/DynamicData/Cache/Internal/TreeBuilder.cs b/src/DynamicData/Cache/Internal/TreeBuilder.cs index bf379ef9..d94783e8 100644 --- a/src/DynamicData/Cache/Internal/TreeBuilder.cs +++ b/src/DynamicData/Cache/Internal/TreeBuilder.cs @@ -7,6 +7,8 @@ using System.Reactive.Linq; using System.Reactive.Subjects; +using DynamicData.Internal; + namespace DynamicData.Cache.Internal; internal sealed class TreeBuilder(IObservable> source, Func pivotOn, IObservable, bool>>? predicateChanged) @@ -197,7 +199,10 @@ void UpdateChildren(Node parentNode) reFilterObservable.OnNext(Unit.Default); }).DisposeMany().Subscribe(); - var filter = _predicateChanged.SynchronizeSafe(queue).CombineLatest(reFilterObservable, (predicate, _) => predicate); + // Both inputs are routed through the same SharedDeliveryQueue so their delivery is + // serialized; UnsynchronizedCombineLatest avoids the ABBA-prone gate that + // Observable.CombineLatest would hold across downstream delivery. + var filter = _predicateChanged.SynchronizeSafe(queue).UnsynchronizedCombineLatest(reFilterObservable.SynchronizeSafe(queue), (predicate, _) => predicate); var result = allNodes.Connect().Filter(filter).SubscribeSafe(observer); return new CompositeDisposable(result, parentSetter, allData, allNodes, groupedByPivot, Disposable.Create(() => reFilterObservable.OnCompleted()), queue); diff --git a/src/DynamicData/Internal/SynchronizeSafeExtensions.cs b/src/DynamicData/Internal/SynchronizeSafeExtensions.cs index 7f1e06f9..603f545c 100644 --- a/src/DynamicData/Internal/SynchronizeSafeExtensions.cs +++ b/src/DynamicData/Internal/SynchronizeSafeExtensions.cs @@ -122,6 +122,88 @@ void OnErrorSafe(Exception error) } } + void OnCompletedSafe() + { + if (Interlocked.Decrement(ref remainingSources) == 0 && Interlocked.Exchange(ref terminated, 1) == 0) + { + observer.OnCompleted(); + } + } + }); + + // Two-input CombineLatest variant that does NOT install a gate. Functionally equivalent + // to Observable.CombineLatest: holds the most-recent value from each source, emits a + // resultSelector output whenever either source fires (provided the other has also fired + // at least once), the first error terminates, completes when both sources complete. + // + // Same precondition as UnsynchronizedMerge: delivery from BOTH sources must already be + // serialized through the same external gate before reaching this operator. In this library + // that is satisfied by routing both inputs through the same SharedDeliveryQueue via + // SynchronizeSafe(queue). Under that precondition no two OnNext calls overlap, so the + // latest-value state needs no internal locking, and the gate that + // Observable.CombineLatest installs becomes redundant. + // + // The Rx gate matters here for the same reason as Merge: Observable.CombineLatest holds + // its private _gate for the entire downstream delivery, and any operator-level lock held + // across a cross-cache write reconstructs the ABBA cycle the queue-drain design eliminated. + // + // Without the external serialization precondition, concurrent OnNext calls would race the + // latest-value state and could produce torn reads. Do not use as a general-purpose + // Observable.CombineLatest replacement. + public static IObservable UnsynchronizedCombineLatest( + this IObservable first, + IObservable second, + Func resultSelector) + where TFirst : notnull + where TSecond : notnull => + Observable.Create(observer => + { + var firstLatest = Optional.None(); + var secondLatest = Optional.None(); + var remainingSources = 2; + var terminated = 0; + + var subscriptions = new CompositeDisposable(2); + subscriptions.Add(first.SubscribeSafe(Observer.Create(OnFirstNext, OnErrorSafe, OnCompletedSafe))); + subscriptions.Add(second.SubscribeSafe(Observer.Create(OnSecondNext, OnErrorSafe, OnCompletedSafe))); + return subscriptions; + + void OnFirstNext(TFirst value) + { + if (Volatile.Read(ref terminated) != 0) + { + return; + } + + firstLatest = value; + if (secondLatest.HasValue) + { + observer.OnNext(resultSelector(value, secondLatest.Value)); + } + } + + void OnSecondNext(TSecond value) + { + if (Volatile.Read(ref terminated) != 0) + { + return; + } + + secondLatest = value; + if (firstLatest.HasValue) + { + observer.OnNext(resultSelector(firstLatest.Value, value)); + } + } + + void OnErrorSafe(Exception error) + { + if (Interlocked.Exchange(ref terminated, 1) == 0) + { + observer.OnError(error); + } + } + void OnCompletedSafe() { if (Interlocked.Decrement(ref remainingSources) == 0 && Interlocked.Exchange(ref terminated, 1) == 0) From baa049e888842bc6d5d8c8c8131dcc809b0e9fe4 Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Sun, 14 Jun 2026 11:04:36 -0700 Subject: [PATCH 12/24] Add canonical Rx contract rules reference Adds .github/instructions/rx-contract.instructions.md, the canonical rule reference distilled from Microsoft Rx Design Guidelines v1.0 (October 2010). Rules use the original document's section numbers (SS X.Y) so citations in PRs, code reviews, and commit messages are traceable to the source spec. Cross-linked from the existing rx.instructions.md and the root copilot-instructions.md so contributors and agents discover it. --- .github/copilot-instructions.md | 2 +- .../instructions/rx-contract.instructions.md | 273 ++++++++++++++++++ .github/instructions/rx.instructions.md | 2 + 3 files changed, 276 insertions(+), 1 deletion(-) create mode 100644 .github/instructions/rx-contract.instructions.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 95d5c3c4..a5bcdb75 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -39,7 +39,7 @@ When optimizing, measure allocation rates and lock contention, not just wall-clo DynamicData operators compose — the output of one is the input of the next. If any operator violates the Rx contract (e.g., concurrent `OnNext` calls, calls after `OnCompleted`), every downstream operator can corrupt its internal state. This is not a crash — it's silent data corruption that manifests as wrong results, missing items, or phantom entries. In a reactive UI, this means the user sees stale or incorrect data with no error message. -See `.github/instructions/rx.instructions.md` for comprehensive Rx contract rules, scheduler usage, disposable patterns, and a complete standard Rx operator reference. +See `.github/instructions/rx.instructions.md` for comprehensive Rx contract rules, scheduler usage, disposable patterns, and a complete standard Rx operator reference. See `.github/instructions/rx-contract.instructions.md` for the canonical rule reference using the original Microsoft Rx Design Guidelines numbering (cite by `§X.Y` in PRs and code reviews, e.g. "§6.6", "§5.2"). ## Breaking Changes diff --git a/.github/instructions/rx-contract.instructions.md b/.github/instructions/rx-contract.instructions.md new file mode 100644 index 00000000..8bb9ddb5 --- /dev/null +++ b/.github/instructions/rx-contract.instructions.md @@ -0,0 +1,273 @@ +--- +applyTo: "**/*.cs" +--- +# Rx Contract Canonical Rules + +Distilled from **Microsoft Rx Design Guidelines v1.0 (October 2010)** and supplemented by the codebase-specific guidance in `rx.instructions.md`. This is the authoritative reference for what is and is not an Rx contract violation in DynamicData operators. + +**Every operator added or modified must self-audit against these rules. Every bug fix must explicitly state which rules were verified.** + +Rules use the **original Microsoft document's `§X.Y` numbering** throughout so citations are unambiguous and traceable to the source spec. Cite rule IDs in code review comments, PR descriptions, and commit messages (e.g., "Fixes §6.6 violation in `BatchIf`"). + +Sections preserve the original A/B/C ordering of this document (consumer contract first, then operator authors, then consumer-side usage), which is the order most useful for a contributor working in this repo: +- **§4** is the Observable Contract from the consumer's perspective (what callers can rely on) +- **§6** is rules for operator authors (what your operator must guarantee) +- **§5** is rules for code that uses Rx (consumer-side correctness) + +--- + +## Quick reference by consumer type + +**If you're a downstream subscriber relying on Rx outputs:** read §4. These are the guarantees you can count on from any well-behaved `IObservable`. + +**If you're writing code that uses Rx operators:** read §5. These are the rules about correct use of Rx in calling code (`ObserveOn` placement, when `Synchronize` is appropriate, subscribing with all three handlers, etc.). + +**If you're writing or modifying an operator:** read §6. Also apply §5 recursively inside the operator (per §6.20) because operator internals are themselves Rx-consuming code. + +**Reviewing a PR:** start with the "Common violation patterns" table at the bottom; it's organized by symptom. + +--- + +## Modernization notes + +This document is derived from a 2010 specification but uses **current Rx.NET API names throughout**. Where the original PDF references obsolete names, this document uses the modern equivalents. Do NOT use the obsolete names in new code or fixes: + +| PDF / 2010 name (obsolete) | Modern equivalent | +|---|---| +| `Observable.CreateWithDisposable(Func, IDisposable>)` | `Observable.Create(Func, IDisposable>)` (same signature, name unified) | +| `Observable.Create` (2010 variant taking `Func, Action>`) | Still exists, but prefer the `IDisposable`-returning overload for resource lifetime correctness | +| `MutableDisposable` | `SerialDisposable` | +| `Scheduler.Dispatcher` | `DispatcherScheduler.Current` (WPF/WinForms) or platform-specific | +| `Scheduler.ThreadPool` | `Scheduler.Default` or `TaskPoolScheduler.Default` | +| `Scheduler.NewThread` | `NewThreadScheduler.Default` | +| `Observable.FromEvent(d => d.Invoke, add, remove)` (3-arg form) | `Observable.FromEventPattern(add, remove)` (typed event pattern) | +| `Observable.Start(Func)` for async work | `Observable.FromAsync(Func>)` for proper async/await integration | +| `IScheduler.Schedule(Action)` (interface method) | `IScheduler.Schedule(this IScheduler, Action)` extension method (same usage, now an extension) | + +Modern additions worth knowing (not in the 2010 PDF): +- **`SingleAssignmentDisposable`**: assign once, throws on second assignment. Useful when a disposable is wired up asynchronously after subscription returns. +- **`RefCountDisposable`**: reference-counted disposable; underlying resource disposed only when all dependents released. +- **`IScheduler.Schedule(TState, Func)`** generic overload: avoids closure allocation in hot paths. Prefer this in performance-sensitive operators. +- **`ValueTask` / `IAsyncEnumerable` interop**: `ToObservable()`, `ToAsyncEnumerable()` bridges. +- **`System.Threading.Lock`** (NET9+): modern lock type used by DynamicData's `Rxx.Synchronize` on NET9+. + +**If you find an obsolete API name used anywhere in this codebase, that is itself a finding to surface.** + +--- + +## §4: The Observable Contract (consumer-facing) + +### §4.1. The Rx Grammar +``` +OnNext* (OnCompleted | OnError)? +``` +- Zero or more `OnNext`, optionally followed by exactly ONE terminal notification. +- `OnError` and `OnCompleted` are **mutually exclusive**: emit one or the other, never both. +- After a terminal notification, **no further notifications of any kind**: not even another OnError or OnCompleted. + +### §4.2. Serialized Notifications +- `OnNext`, `OnError`, `OnCompleted` calls to a single observer instance **MUST never execute concurrently**. +- This is the most-violated rule in practice; downstream operators and consumer code assume serialization. +- Violation produces silent state corruption, not exceptions. + +### §4.3. Resource Cleanup on Terminal +- After `OnError` or `OnCompleted`, the operator MUST immediately release its resources. +- Side effects bound to subscription lifetime (`Observable.Using`, `Finally`) MUST fire deterministically. + +### §4.4. Unsubscribe Semantics +- Calling `Dispose`: queued-but-not-yet-started work is cancelled. +- Work already in progress MAY complete, BUT **its results MUST NOT be signaled** to the unsubscribed observer. +- Messages MAY arrive during the `Dispose` call itself (Dispose can race with OnNext). +- After `Dispose` returns control to the caller: **no more messages arrive**. +- The unsubscription process MAY continue asynchronously on a different context after `Dispose` returns. + +--- + +## §6: Operator Author Rules + +### §6.1. Prefer Composition Over Observable.Create +- The Rx contracts are axioms, not guidelines. Trust them completely. +- Before reaching for `Observable.Create`, ask: can this be expressed via existing operators? +- Manual observer forwarding inside `Observable.Create` reimplements what `Subscribe` already guarantees, and tends to introduce bugs the existing operators would have prevented. +- See `rx.instructions.md` "Composition First" section for the rationale and the Defer pattern as an alternative for per-subscription state. + +### §6.2. Use Observable.Create (not raw IObservable) when you must +- `Observable.Create` ensures grammar compliance (auto-unsubscribe on terminal, single-terminal enforcement). +- Custom `IObservable` implementations are higher-risk and only justified for non-standard contracts (e.g., when the return type must implement `ISubject` or a richer interface). + +### §6.4. Protect Calls to User Code +**Wrap every user-provided delegate in try/catch and route exceptions to `observer.OnError`:** +- Selector functions (Transform, Select, etc.) +- Predicates (Filter, Where, etc.) +- Comparers (Sort, GroupOn, etc.) +- Key selectors (GroupOn, ToObservableChangeSet, etc.) +- Action callbacks (Do, OnItemRemoved, SubscribeMany, etc.) +- Calls to dictionaries/lists/hashsets that use a user-provided comparer + +**DO NOT wrap these (they are "edge of the monad"):** Subscribe, Dispose, OnNext, OnError, OnCompleted. Calling OnError from these places leads to undefined behavior. + +**Exception:** `IScheduler` implementations protect inside the scheduler itself, not at every call site. + +### §6.5. Subscribe Implementations Must Not Throw +- Subscribe may be called asynchronously (e.g., second source in `Concat` is subscribed after first completes). +- A throw crashes the application: no observer in scope to route to. +- Error conditions detected in Subscribe MUST be routed via `observer.OnError(...)` followed by `return Disposable.Empty;`. + +### §6.6. OnError Has Abort Semantics +- Once a source emits OnError, the operator MUST emit no further messages, not even buffered or aggregated state. +- **Example: a buffering operator must DISCARD its buffer on source error, not flush it.** The semantic is "abort", not "graceful degradation". +- Operators that aggregate state across multiple OnNext (Sort, Group, Page, etc.) must not "salvage" that state into a final OnNext when OnError arrives. + +### §6.7. Multi-Source Operators MUST Serialize +When combining multiple sources into one output observer: +```csharp +var gate = new object(); +source1.Synchronize(gate).Subscribe(observer); +source2.Synchronize(gate).Subscribe(observer); +``` +OR equivalent gate-based pattern (DynamicData's `CacheParentSubscription._synchronize` is the canonical example for cache operators). + +**All three notification types (OnNext / OnError / OnCompleted) must be serialized through the same gate**, not just OnNext. + +### §6.8. Single-Source Operators MUST NOT Redundantly Serialize +- Per §6.7 every operator already serializes; downstream operators can ASSUME serialized input. +- Adding `Synchronize` "just in case" is an anti-pattern: it clutters, harms performance, and signals misunderstanding of the contract. +- `Synchronize` at the consumer-facing boundary (per §5.8) is reserved for fixing genuinely non-conforming external sources. + +### §6.9 / §6.10 / §6.11. Parameterize Concurrency via Scheduler +- Concurrency-introducing operators take an `IScheduler` parameter (§6.9). +- Provide an overload with a sensible default scheduler (§6.10; prefer Immediate where possible per §6.12). +- The `IScheduler` parameter is the **LAST** argument for fluent IntelliSense (§6.11). +- Exception: `params T[]` operators take the scheduler as the second-to-last argument. + +### §6.12. Avoid Introducing Concurrency Where Possible +- Adding concurrency skews timing data: delivery time is itself observable data. +- Default schedulers should be the minimal-concurrency option (often `Scheduler.Immediate`). +- Only introduce concurrency when it is essential to the operator's semantics. + +### §6.13. Hand Out All Disposables +- Every `IDisposable` created inside an operator (subscription disposables, scheduled-action disposables, resource handles) MUST be reachable through the disposable returned to the subscriber. +- Use `CompositeDisposable`, `SerialDisposable`, `SingleAssignmentDisposable`, `RefCountDisposable` to compose. +- Hidden disposables = leaks at unsubscribe time and broken backpressure-style cleanup. + +### §6.14. Operators Must Not Block +- Return `IObservable`, never `T`. +- Even aggregation operators (Sum, Count, etc.) return `IObservable`: the caller can use `First*`/`Last*`/`Single*` to escape the observable world if blocking is genuinely needed at the call site. + +### §6.15. Avoid Deep Recursion in Operators +- Stack depth at operator invocation is unknown; recursive operators can blow the stack faster than expected. +- Use `IScheduler.Schedule(self => ...)` recursive overload OR `IEnumerable>` + `Concat` with a `yield` iterator. + +### §6.16. Argument Validation Outside Observable.Create +- Per §6.5, `Observable.Create`'s subscribe lambda must not throw. +- Therefore: `ArgumentNullException` and other validation checks belong BEFORE the `Observable.Create(...)` call. +- Exception: validation that genuinely requires the subscription to be live (rare). + +### §6.17. Idempotent Dispose +- Multiple `Dispose()` calls on the same disposable: first runs cleanup, subsequent are no-ops. +- Required because consumers don't know subscription state and may dispose defensively. +- Use a `_disposedValue` flag or equivalent guard. + +### §6.18. Dispose Must Not Throw +- Dispose chains cascade through compositions. +- A throw crashes the app, and the observer is already unsubscribed, so OnError cannot route the exception. +- If cleanup can fail, swallow + log; never propagate. +- When disposing multiple children, wrap each in try/catch so one failure doesn't skip the rest. + +### §6.19. Custom IObservable Implementations MUST Follow the Contract +- If you can't use `Observable.Create`, you take on the burden of contract compliance yourself. +- All of §4 applies to your implementation. + +### §6.20. Operator Implementations MUST Follow Usage Guidelines +- Operators internally compose other operators (per §6.1). +- §5 ("Using Rx") rules apply recursively to operator internals. + +--- + +## §5: Using Rx (consumer-side rules) + +These rules apply recursively inside operator implementations too, per §6.20. + +### §5.2. Provide All Three Subscribe Arguments +- Default `Subscribe(onNext)` overload rethrows OnError on the source thread, crashing the app. +- Always provide onError and onCompleted handlers unless: + - The source is guaranteed not to complete + - The source is guaranteed not to error + - The default behavior is genuinely desired + +### §5.4. Pass Specific Scheduler to Concurrency-Introducing Operators +- Prefer `source.Throttle(timeSpan, specificScheduler)` over `source.Throttle(timeSpan).ObserveOn(specificScheduler)`. +- Creates the work on the right scheduler from the start; saves a scheduler hop. + +### §5.5. ObserveOn Late, Few +- Each `ObserveOn` schedules per-message work and is expensive. +- Apply filters first, then `ObserveOn`, to avoid scheduling work for messages that get filtered out. + +### §5.6. Limit Buffers +- `Replay`, `Buffer`, etc. without size/time limits cause unbounded memory growth. +- Always provide a limit: `Replay(10000, TimeSpan.FromHours(1))`. + +### §5.7. Make Side Effects Explicit via Do +- Side effects (logging, mutation, etc.) buried in selector/predicate lambdas are unauditable. +- Hoist them into `Do(...)` so the side-effect is visible in the pipeline. +- Note: per Rx semantics, `Do` runs for each subscriber unless the pipeline is shared via `Publish`. + +### §5.8. Use Synchronize Only for Non-Conforming Sources +- Operators created by Rx (and DynamicData) already follow the contract. +- `Synchronize()` (consumer-facing, no gate) is for external non-conforming `IObservable` implementations only. +- NOTE: the **gate-based** `Synchronize(gate)` inside multi-source operators (per §6.7) is a DIFFERENT pattern and IS valid. + +### §5.10. Share Side Effects via Publish +- Cold observables re-run their side effects on every subscribe. +- If sharing is needed, use `Publish(shared => ...)` or `Publish().RefCount()`. + +--- + +## Common violation patterns (quick scan during code review) + +| Violation shape | Rule | Detection cue | +|---|---|---| +| Buffering operator flushes buffer on OnError | §6.6 | `OnError` handler calls OnNext before propagating | +| Multi-source operator forgets to serialize OnCompleted (only serializes OnNext) | §6.7 | `Synchronize` applied to OnNext but raw OnCompleted/OnError | +| Single-source operator wraps OnNext in unnecessary `lock(gate)` | §6.8 | `lock` inside a Transform/Filter/Select implementation | +| `null` check inside `Observable.Create(obs => { if (x == null) throw ... })` | §6.16 | Validation inside the subscribe lambda | +| Subscribe lambda throws on error condition rather than routing to OnError | §6.5 | `throw` inside `Observable.Create(observer => { ... throw ... })` | +| Dispose method throws (or contains code that can throw without catch) | §6.18 | IDisposable.Dispose without try/finally; multi-child dispose without per-child try/catch | +| Re-entrant Dispose runs cleanup twice | §6.17 | Missing `_disposedValue` flag or equivalent idempotency guard | +| Scheduler-queued work fires OnNext after Dispose | §4.4 | `IScheduler.Schedule(() => observer.OnNext(x))` without checking subscription state | +| User callback (selector, comparer, predicate, action) not protected by try/catch | §6.4 | Direct invocation of user delegate inside operator without wrapping | +| Recursive operator without `Schedule(self)` or `Concat`+yield | §6.15 | Direct self-recursion in OnNext handler | +| Hidden disposable not exposed to caller | §6.13 | `_ = scheduler.Schedule(...)` discarding the return value | +| Sum/Aggregate returns `T` instead of `IObservable` | §6.14 | Blocking return type on what should be an operator | +| Argument validation missing entirely on a public extension method | §6.16 | No null check on extension method parameters | + +--- + +## How to use this reference + +### When writing or modifying an operator +1. After implementing the operator, walk every rule in §4 and §6. +2. For each rule, confirm the operator does not violate it. +3. Document the audit in the PR description (e.g., "Audited against §4.1-§4.4, §6.1, §6.4-§6.8, §6.13, §6.16-§6.18; §6.7 N/A (single-source operator)"). + +### When reviewing a PR +1. Use the "Common violation patterns" table as a scanning checklist. +2. Cite rule IDs in review comments (e.g., "§6.6: buffer should be discarded on error, not flushed"). +3. Block merge on any unaddressed rule violation. + +### When debugging an Rx-related bug +1. Identify the rule(s) most likely to explain the symptom. +2. Add tests that exercise the contract boundary (use `Tests/Utilities/ValidateSynchronization` for §4.2, `TestSourceCache.SetError` for §4.4 / §6.6, etc.). +3. Reference the rule ID in the fix commit message. + +--- + +## Maintaining this document + +This file is a derivative of the Microsoft Rx Design Guidelines v1.0 (October 2010). The Microsoft document has not been republished since v1.0 and the underlying Rx contract is stable. + +**Do NOT modify rule IDs.** External code reviews, PR descriptions, and commit messages cite the `§X.Y` IDs; renumbering breaks those references. The IDs come from the original PDF and changing them severs traceability to the source spec. + +**DO add new rules** in a new section (e.g., a DynamicData-specific section that goes beyond standard Rx) if the codebase discovers patterns the Microsoft document doesn't cover. Use a non-numeric prefix (e.g., `DD-1`, `DD-2`) to make clear they are not from the original spec. + +**Cross-references:** see `rx.instructions.md` for prose-style teaching of the same concepts plus DynamicData-specific patterns (Defer for per-subscription state, Observable.Create when truly needed, disposable family reference, common pitfalls). diff --git a/.github/instructions/rx.instructions.md b/.github/instructions/rx.instructions.md index f5f3a21c..e82ab560 100644 --- a/.github/instructions/rx.instructions.md +++ b/.github/instructions/rx.instructions.md @@ -5,6 +5,8 @@ applyTo: "**/*.cs" Reference: [ReactiveX Observable Contract](http://reactivex.io/documentation/contract.html) | [Rx.NET GitHub](https://github.com/dotnet/reactive) | [IntroToRx.com](http://introtorx.com/) +**See also:** [`rx-contract.instructions.md`](./rx-contract.instructions.md) is the canonical rule reference using the original Microsoft Rx Design Guidelines numbering (`§X.Y` IDs like `§6.6`, `§5.2`). Cite those IDs in PR descriptions, code reviews, and commit messages. + ## Core Concepts ### Observables are Composable From 103f851b30ca6f6276257d7f44a77a0d85e50b41 Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Sun, 14 Jun 2026 11:08:06 -0700 Subject: [PATCH 13/24] Document citation format for Rx contract rule IDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a "Citing rules" subsection clarifying that the literal § character must be used in commit messages, PR descriptions, code reviews, and code comments when referring to a rule (e.g., §6.6, §5.8, §4.2). ASCII substitutes are explicitly disallowed. Includes an example commit subject demonstrating the format. --- .github/instructions/rx-contract.instructions.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.github/instructions/rx-contract.instructions.md b/.github/instructions/rx-contract.instructions.md index 8bb9ddb5..b7bee9ac 100644 --- a/.github/instructions/rx-contract.instructions.md +++ b/.github/instructions/rx-contract.instructions.md @@ -260,6 +260,18 @@ These rules apply recursively inside operator implementations too, per §6.20. 2. Add tests that exercise the contract boundary (use `Tests/Utilities/ValidateSynchronization` for §4.2, `TestSourceCache.SetError` for §4.4 / §6.6, etc.). 3. Reference the rule ID in the fix commit message. +### Citing rules + +Always use the literal `§` character when citing rule IDs (in commit messages, PR descriptions, code review comments, and code comments). Do NOT use ASCII substitutes (`S`, `SS`, `sec`, `§§`, etc.). Example commit subject: + +``` +Fix §6.6 violation in BatchIf + +BatchIf was flushing its buffered changeset on source OnError, which +violates §6.6's abort semantic. Discard the buffer instead so error +deliveries are not preceded by stale OnNext. +``` + --- ## Maintaining this document From 9383308880f3c78a76fea05c323b4c44c0fab4e9 Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Sun, 14 Jun 2026 11:17:07 -0700 Subject: [PATCH 14/24] Link to original Rx Design Guidelines PDF in rx-contract reference The intro and "Maintaining this document" section now link to the canonical Microsoft fwlink (https://go.microsoft.com/fwlink/?LinkID=205219) so contributors can consult the source PDF when revising rule text, adding new rules, or resolving questions about original intent. --- .github/instructions/rx-contract.instructions.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/instructions/rx-contract.instructions.md b/.github/instructions/rx-contract.instructions.md index b7bee9ac..4aa29ed3 100644 --- a/.github/instructions/rx-contract.instructions.md +++ b/.github/instructions/rx-contract.instructions.md @@ -3,7 +3,7 @@ applyTo: "**/*.cs" --- # Rx Contract Canonical Rules -Distilled from **Microsoft Rx Design Guidelines v1.0 (October 2010)** and supplemented by the codebase-specific guidance in `rx.instructions.md`. This is the authoritative reference for what is and is not an Rx contract violation in DynamicData operators. +Distilled from **[Microsoft Rx Design Guidelines v1.0 (October 2010)](https://go.microsoft.com/fwlink/?LinkID=205219)** and supplemented by the codebase-specific guidance in `rx.instructions.md`. This is the authoritative reference for what is and is not an Rx contract violation in DynamicData operators. **Every operator added or modified must self-audit against these rules. Every bug fix must explicitly state which rules were verified.** @@ -276,7 +276,7 @@ deliveries are not preceded by stale OnNext. ## Maintaining this document -This file is a derivative of the Microsoft Rx Design Guidelines v1.0 (October 2010). The Microsoft document has not been republished since v1.0 and the underlying Rx contract is stable. +This file is a derivative of the [Microsoft Rx Design Guidelines v1.0 (October 2010)](https://go.microsoft.com/fwlink/?LinkID=205219) (canonical fwlink; resolves to `download.microsoft.com/.../Rx Design Guidelines.pdf`). The Microsoft document has not been republished since v1.0 and the underlying Rx contract is stable. **Consult the PDF directly when revising rule text, adding new rules, or resolving disputes about intent.** **Do NOT modify rule IDs.** External code reviews, PR descriptions, and commit messages cite the `§X.Y` IDs; renumbering breaks those references. The IDs come from the original PDF and changing them severs traceability to the source spec. From d9c6a4e66dbb5c76f8a95b873f004d939f5061ba Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Sun, 14 Jun 2026 11:19:22 -0700 Subject: [PATCH 15/24] Dedupe section-structure blurb in rx-contract reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The intro had a "§4/§6/§5" bullet list and the "Quick reference by consumer type" section had a separate, wordier "if you're a..." prose block covering the same ground. Move the concise bullet list into the Quick reference section and drop it from the intro. Net effect: intro is now three paragraphs (derivation, audit requirement, numbering note); the Quick reference section carries the section-by-section summary. --- .github/instructions/rx-contract.instructions.md | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/.github/instructions/rx-contract.instructions.md b/.github/instructions/rx-contract.instructions.md index 4aa29ed3..5badac35 100644 --- a/.github/instructions/rx-contract.instructions.md +++ b/.github/instructions/rx-contract.instructions.md @@ -9,22 +9,14 @@ Distilled from **[Microsoft Rx Design Guidelines v1.0 (October 2010)](https://go Rules use the **original Microsoft document's `§X.Y` numbering** throughout so citations are unambiguous and traceable to the source spec. Cite rule IDs in code review comments, PR descriptions, and commit messages (e.g., "Fixes §6.6 violation in `BatchIf`"). -Sections preserve the original A/B/C ordering of this document (consumer contract first, then operator authors, then consumer-side usage), which is the order most useful for a contributor working in this repo: -- **§4** is the Observable Contract from the consumer's perspective (what callers can rely on) -- **§6** is rules for operator authors (what your operator must guarantee) -- **§5** is rules for code that uses Rx (consumer-side correctness) - --- ## Quick reference by consumer type -**If you're a downstream subscriber relying on Rx outputs:** read §4. These are the guarantees you can count on from any well-behaved `IObservable`. - -**If you're writing code that uses Rx operators:** read §5. These are the rules about correct use of Rx in calling code (`ObserveOn` placement, when `Synchronize` is appropriate, subscribing with all three handlers, etc.). - -**If you're writing or modifying an operator:** read §6. Also apply §5 recursively inside the operator (per §6.20) because operator internals are themselves Rx-consuming code. - -**Reviewing a PR:** start with the "Common violation patterns" table at the bottom; it's organized by symptom. +Sections preserve the original A/B/C ordering of this document (consumer contract first, then operator authors, then consumer-side usage), which is the order most useful for a contributor working in this repo: +- **§4** is the Observable Contract from the consumer's perspective (what callers can rely on) +- **§6** is rules for operator authors (what your operator must guarantee) +- **§5** is rules for code that uses Rx (consumer-side correctness) --- From 2fc623ac47707589bcb4d2a3fa0abe6ca6739e50 Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Sun, 14 Jun 2026 11:20:38 -0700 Subject: [PATCH 16/24] Rename "Quick reference by consumer type" to "Document structure" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After the prior commit moved the §4/§6/§5 bullets into that section, the title no longer matched the content (bullets are organized by section number, not by consumer type). "Document structure" reflects what is actually there. --- .github/instructions/rx-contract.instructions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/instructions/rx-contract.instructions.md b/.github/instructions/rx-contract.instructions.md index 5badac35..abbb8ab5 100644 --- a/.github/instructions/rx-contract.instructions.md +++ b/.github/instructions/rx-contract.instructions.md @@ -11,7 +11,7 @@ Rules use the **original Microsoft document's `§X.Y` numbering** throughout so --- -## Quick reference by consumer type +## Document structure Sections preserve the original A/B/C ordering of this document (consumer contract first, then operator authors, then consumer-side usage), which is the order most useful for a contributor working in this repo: - **§4** is the Observable Contract from the consumer's perspective (what callers can rely on) From a243b18ea1e15edbef8a9244530a17f41fdaf918 Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Sun, 14 Jun 2026 11:36:18 -0700 Subject: [PATCH 17/24] Restore PDF section order in rx-contract reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related fixes: 1. Section order: §4 -> §5 -> §6 (matches the original Microsoft Rx Design Guidelines PDF). An earlier commit had §6 ahead of §5 based on a misread of an instruction to "not reorder sections" (the instruction was to preserve PDF order, not the pre-existing in-repo order). 2. Document structure section: drop the "A/B/C ordering" reference, which was a private notation no reader of this file would know. Just describe what §4, §5, and §6 each contain, in PDF order. --- .../instructions/rx-contract.instructions.md | 82 +++++++++---------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/.github/instructions/rx-contract.instructions.md b/.github/instructions/rx-contract.instructions.md index abbb8ab5..cebf23c1 100644 --- a/.github/instructions/rx-contract.instructions.md +++ b/.github/instructions/rx-contract.instructions.md @@ -13,10 +13,10 @@ Rules use the **original Microsoft document's `§X.Y` numbering** throughout so ## Document structure -Sections preserve the original A/B/C ordering of this document (consumer contract first, then operator authors, then consumer-side usage), which is the order most useful for a contributor working in this repo: +The file follows the original Microsoft document's section order: - **§4** is the Observable Contract from the consumer's perspective (what callers can rely on) -- **§6** is rules for operator authors (what your operator must guarantee) - **§5** is rules for code that uses Rx (consumer-side correctness) +- **§6** is rules for operator authors (what your operator must guarantee) --- @@ -75,6 +75,45 @@ OnNext* (OnCompleted | OnError)? --- +## §5: Using Rx (consumer-side rules) + +These rules apply recursively inside operator implementations too, per §6.20. + +### §5.2. Provide All Three Subscribe Arguments +- Default `Subscribe(onNext)` overload rethrows OnError on the source thread, crashing the app. +- Always provide onError and onCompleted handlers unless: + - The source is guaranteed not to complete + - The source is guaranteed not to error + - The default behavior is genuinely desired + +### §5.4. Pass Specific Scheduler to Concurrency-Introducing Operators +- Prefer `source.Throttle(timeSpan, specificScheduler)` over `source.Throttle(timeSpan).ObserveOn(specificScheduler)`. +- Creates the work on the right scheduler from the start; saves a scheduler hop. + +### §5.5. ObserveOn Late, Few +- Each `ObserveOn` schedules per-message work and is expensive. +- Apply filters first, then `ObserveOn`, to avoid scheduling work for messages that get filtered out. + +### §5.6. Limit Buffers +- `Replay`, `Buffer`, etc. without size/time limits cause unbounded memory growth. +- Always provide a limit: `Replay(10000, TimeSpan.FromHours(1))`. + +### §5.7. Make Side Effects Explicit via Do +- Side effects (logging, mutation, etc.) buried in selector/predicate lambdas are unauditable. +- Hoist them into `Do(...)` so the side-effect is visible in the pipeline. +- Note: per Rx semantics, `Do` runs for each subscriber unless the pipeline is shared via `Publish`. + +### §5.8. Use Synchronize Only for Non-Conforming Sources +- Operators created by Rx (and DynamicData) already follow the contract. +- `Synchronize()` (consumer-facing, no gate) is for external non-conforming `IObservable` implementations only. +- NOTE: the **gate-based** `Synchronize(gate)` inside multi-source operators (per §6.7) is a DIFFERENT pattern and IS valid. + +### §5.10. Share Side Effects via Publish +- Cold observables re-run their side effects on every subscribe. +- If sharing is needed, use `Publish(shared => ...)` or `Publish().RefCount()`. + +--- + ## §6: Operator Author Rules ### §6.1. Prefer Composition Over Observable.Create @@ -176,45 +215,6 @@ OR equivalent gate-based pattern (DynamicData's `CacheParentSubscription._synchr --- -## §5: Using Rx (consumer-side rules) - -These rules apply recursively inside operator implementations too, per §6.20. - -### §5.2. Provide All Three Subscribe Arguments -- Default `Subscribe(onNext)` overload rethrows OnError on the source thread, crashing the app. -- Always provide onError and onCompleted handlers unless: - - The source is guaranteed not to complete - - The source is guaranteed not to error - - The default behavior is genuinely desired - -### §5.4. Pass Specific Scheduler to Concurrency-Introducing Operators -- Prefer `source.Throttle(timeSpan, specificScheduler)` over `source.Throttle(timeSpan).ObserveOn(specificScheduler)`. -- Creates the work on the right scheduler from the start; saves a scheduler hop. - -### §5.5. ObserveOn Late, Few -- Each `ObserveOn` schedules per-message work and is expensive. -- Apply filters first, then `ObserveOn`, to avoid scheduling work for messages that get filtered out. - -### §5.6. Limit Buffers -- `Replay`, `Buffer`, etc. without size/time limits cause unbounded memory growth. -- Always provide a limit: `Replay(10000, TimeSpan.FromHours(1))`. - -### §5.7. Make Side Effects Explicit via Do -- Side effects (logging, mutation, etc.) buried in selector/predicate lambdas are unauditable. -- Hoist them into `Do(...)` so the side-effect is visible in the pipeline. -- Note: per Rx semantics, `Do` runs for each subscriber unless the pipeline is shared via `Publish`. - -### §5.8. Use Synchronize Only for Non-Conforming Sources -- Operators created by Rx (and DynamicData) already follow the contract. -- `Synchronize()` (consumer-facing, no gate) is for external non-conforming `IObservable` implementations only. -- NOTE: the **gate-based** `Synchronize(gate)` inside multi-source operators (per §6.7) is a DIFFERENT pattern and IS valid. - -### §5.10. Share Side Effects via Publish -- Cold observables re-run their side effects on every subscribe. -- If sharing is needed, use `Publish(shared => ...)` or `Publish().RefCount()`. - ---- - ## Common violation patterns (quick scan during code review) | Violation shape | Rule | Detection cue | From 4eb91ebf02dbfc7f0667574cca15d0826937f4ac Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Sun, 14 Jun 2026 12:07:12 -0700 Subject: [PATCH 18/24] Rename rx-contract to rx-design-guide; complete PDF distillation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renames rx-contract.instructions.md to rx-design-guide.instructions.md and expands it to a complete distillation of the Microsoft Rx Design Guidelines v1.0 (October 2010). The goal is that contributors and agents don't have to re-consume the source PDF: the entire spec (every rule, every "When to ignore" caveat, the materially-clarifying samples) is preserved here. What's new vs the prior rx-contract file: - Adds §2 Introduction (explains that the guidelines are patterns the Rx team found helpful, not absolute truths, and that there is no partial ordering between them). - Adds §3 (3.1, 3.2) "When to use Rx". These were missing entirely. - Adds §4.3 and §4.4 with the PDF's own clarifying samples (resource cleanup, unsubscribe sample 1 / sample 2). - Adds the PDF's "When to ignore this guideline" caveats verbatim (or near-verbatim, modernized for current Rx.NET API names) on every rule that has one. Previously many were dropped. - Adds §5.1 (Marble-diagram), §5.3 (LINQ query syntax), §5.9 (unsubscribe in-flight messages). Previously missing. - Adds §6.3 (extension methods, generics). Previously missing. - Splits §6.9/6.10/6.11 into separate rule entries instead of a combined heading (matches the PDF; preserves citation granularity). - Code samples modernized to current Rx.NET API names with a Modernization-notes table at the top; the table is now flagged as a living document to keep current. rx.instructions.md is restructured to be the DynamicData-flavored practical companion (hot/cold semantics, scheduler reference, disposable family with code samples, modern operator catalog, custom-operator patterns specific to this codebase, common pitfalls). Content duplicated in the design guide is removed and replaced with cross-links to specific §X.Y anchors. copilot-instructions.md updated to reference both files. PR #1097 description and inline citations still resolve: the §X.Y rule IDs are stable and the anchor structure of the renamed file preserves the (rule-id)-(headline) format that the existing links use. --- .github/copilot-instructions.md | 2 +- .../instructions/rx-contract.instructions.md | 277 ------ .../rx-design-guide.instructions.md | 798 ++++++++++++++++++ .github/instructions/rx.instructions.md | 144 ++-- 4 files changed, 857 insertions(+), 364 deletions(-) delete mode 100644 .github/instructions/rx-contract.instructions.md create mode 100644 .github/instructions/rx-design-guide.instructions.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index a5bcdb75..8d9261e1 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -39,7 +39,7 @@ When optimizing, measure allocation rates and lock contention, not just wall-clo DynamicData operators compose — the output of one is the input of the next. If any operator violates the Rx contract (e.g., concurrent `OnNext` calls, calls after `OnCompleted`), every downstream operator can corrupt its internal state. This is not a crash — it's silent data corruption that manifests as wrong results, missing items, or phantom entries. In a reactive UI, this means the user sees stale or incorrect data with no error message. -See `.github/instructions/rx.instructions.md` for comprehensive Rx contract rules, scheduler usage, disposable patterns, and a complete standard Rx operator reference. See `.github/instructions/rx-contract.instructions.md` for the canonical rule reference using the original Microsoft Rx Design Guidelines numbering (cite by `§X.Y` in PRs and code reviews, e.g. "§6.6", "§5.2"). +See `.github/instructions/rx.instructions.md` for the DynamicData-flavored practical guide (hot/cold, schedulers, disposable helpers, custom operator patterns, common pitfalls), and `.github/instructions/rx-design-guide.instructions.md` for the canonical rule reference (a complete distillation of the Microsoft Rx Design Guidelines, with stable `§X.Y` IDs to cite in PRs, code reviews, and commit messages, e.g. "§6.6", "§5.2"). ## Breaking Changes diff --git a/.github/instructions/rx-contract.instructions.md b/.github/instructions/rx-contract.instructions.md deleted file mode 100644 index cebf23c1..00000000 --- a/.github/instructions/rx-contract.instructions.md +++ /dev/null @@ -1,277 +0,0 @@ ---- -applyTo: "**/*.cs" ---- -# Rx Contract Canonical Rules - -Distilled from **[Microsoft Rx Design Guidelines v1.0 (October 2010)](https://go.microsoft.com/fwlink/?LinkID=205219)** and supplemented by the codebase-specific guidance in `rx.instructions.md`. This is the authoritative reference for what is and is not an Rx contract violation in DynamicData operators. - -**Every operator added or modified must self-audit against these rules. Every bug fix must explicitly state which rules were verified.** - -Rules use the **original Microsoft document's `§X.Y` numbering** throughout so citations are unambiguous and traceable to the source spec. Cite rule IDs in code review comments, PR descriptions, and commit messages (e.g., "Fixes §6.6 violation in `BatchIf`"). - ---- - -## Document structure - -The file follows the original Microsoft document's section order: -- **§4** is the Observable Contract from the consumer's perspective (what callers can rely on) -- **§5** is rules for code that uses Rx (consumer-side correctness) -- **§6** is rules for operator authors (what your operator must guarantee) - ---- - -## Modernization notes - -This document is derived from a 2010 specification but uses **current Rx.NET API names throughout**. Where the original PDF references obsolete names, this document uses the modern equivalents. Do NOT use the obsolete names in new code or fixes: - -| PDF / 2010 name (obsolete) | Modern equivalent | -|---|---| -| `Observable.CreateWithDisposable(Func, IDisposable>)` | `Observable.Create(Func, IDisposable>)` (same signature, name unified) | -| `Observable.Create` (2010 variant taking `Func, Action>`) | Still exists, but prefer the `IDisposable`-returning overload for resource lifetime correctness | -| `MutableDisposable` | `SerialDisposable` | -| `Scheduler.Dispatcher` | `DispatcherScheduler.Current` (WPF/WinForms) or platform-specific | -| `Scheduler.ThreadPool` | `Scheduler.Default` or `TaskPoolScheduler.Default` | -| `Scheduler.NewThread` | `NewThreadScheduler.Default` | -| `Observable.FromEvent(d => d.Invoke, add, remove)` (3-arg form) | `Observable.FromEventPattern(add, remove)` (typed event pattern) | -| `Observable.Start(Func)` for async work | `Observable.FromAsync(Func>)` for proper async/await integration | -| `IScheduler.Schedule(Action)` (interface method) | `IScheduler.Schedule(this IScheduler, Action)` extension method (same usage, now an extension) | - -Modern additions worth knowing (not in the 2010 PDF): -- **`SingleAssignmentDisposable`**: assign once, throws on second assignment. Useful when a disposable is wired up asynchronously after subscription returns. -- **`RefCountDisposable`**: reference-counted disposable; underlying resource disposed only when all dependents released. -- **`IScheduler.Schedule(TState, Func)`** generic overload: avoids closure allocation in hot paths. Prefer this in performance-sensitive operators. -- **`ValueTask` / `IAsyncEnumerable` interop**: `ToObservable()`, `ToAsyncEnumerable()` bridges. -- **`System.Threading.Lock`** (NET9+): modern lock type used by DynamicData's `Rxx.Synchronize` on NET9+. - -**If you find an obsolete API name used anywhere in this codebase, that is itself a finding to surface.** - ---- - -## §4: The Observable Contract (consumer-facing) - -### §4.1. The Rx Grammar -``` -OnNext* (OnCompleted | OnError)? -``` -- Zero or more `OnNext`, optionally followed by exactly ONE terminal notification. -- `OnError` and `OnCompleted` are **mutually exclusive**: emit one or the other, never both. -- After a terminal notification, **no further notifications of any kind**: not even another OnError or OnCompleted. - -### §4.2. Serialized Notifications -- `OnNext`, `OnError`, `OnCompleted` calls to a single observer instance **MUST never execute concurrently**. -- This is the most-violated rule in practice; downstream operators and consumer code assume serialization. -- Violation produces silent state corruption, not exceptions. - -### §4.3. Resource Cleanup on Terminal -- After `OnError` or `OnCompleted`, the operator MUST immediately release its resources. -- Side effects bound to subscription lifetime (`Observable.Using`, `Finally`) MUST fire deterministically. - -### §4.4. Unsubscribe Semantics -- Calling `Dispose`: queued-but-not-yet-started work is cancelled. -- Work already in progress MAY complete, BUT **its results MUST NOT be signaled** to the unsubscribed observer. -- Messages MAY arrive during the `Dispose` call itself (Dispose can race with OnNext). -- After `Dispose` returns control to the caller: **no more messages arrive**. -- The unsubscription process MAY continue asynchronously on a different context after `Dispose` returns. - ---- - -## §5: Using Rx (consumer-side rules) - -These rules apply recursively inside operator implementations too, per §6.20. - -### §5.2. Provide All Three Subscribe Arguments -- Default `Subscribe(onNext)` overload rethrows OnError on the source thread, crashing the app. -- Always provide onError and onCompleted handlers unless: - - The source is guaranteed not to complete - - The source is guaranteed not to error - - The default behavior is genuinely desired - -### §5.4. Pass Specific Scheduler to Concurrency-Introducing Operators -- Prefer `source.Throttle(timeSpan, specificScheduler)` over `source.Throttle(timeSpan).ObserveOn(specificScheduler)`. -- Creates the work on the right scheduler from the start; saves a scheduler hop. - -### §5.5. ObserveOn Late, Few -- Each `ObserveOn` schedules per-message work and is expensive. -- Apply filters first, then `ObserveOn`, to avoid scheduling work for messages that get filtered out. - -### §5.6. Limit Buffers -- `Replay`, `Buffer`, etc. without size/time limits cause unbounded memory growth. -- Always provide a limit: `Replay(10000, TimeSpan.FromHours(1))`. - -### §5.7. Make Side Effects Explicit via Do -- Side effects (logging, mutation, etc.) buried in selector/predicate lambdas are unauditable. -- Hoist them into `Do(...)` so the side-effect is visible in the pipeline. -- Note: per Rx semantics, `Do` runs for each subscriber unless the pipeline is shared via `Publish`. - -### §5.8. Use Synchronize Only for Non-Conforming Sources -- Operators created by Rx (and DynamicData) already follow the contract. -- `Synchronize()` (consumer-facing, no gate) is for external non-conforming `IObservable` implementations only. -- NOTE: the **gate-based** `Synchronize(gate)` inside multi-source operators (per §6.7) is a DIFFERENT pattern and IS valid. - -### §5.10. Share Side Effects via Publish -- Cold observables re-run their side effects on every subscribe. -- If sharing is needed, use `Publish(shared => ...)` or `Publish().RefCount()`. - ---- - -## §6: Operator Author Rules - -### §6.1. Prefer Composition Over Observable.Create -- The Rx contracts are axioms, not guidelines. Trust them completely. -- Before reaching for `Observable.Create`, ask: can this be expressed via existing operators? -- Manual observer forwarding inside `Observable.Create` reimplements what `Subscribe` already guarantees, and tends to introduce bugs the existing operators would have prevented. -- See `rx.instructions.md` "Composition First" section for the rationale and the Defer pattern as an alternative for per-subscription state. - -### §6.2. Use Observable.Create (not raw IObservable) when you must -- `Observable.Create` ensures grammar compliance (auto-unsubscribe on terminal, single-terminal enforcement). -- Custom `IObservable` implementations are higher-risk and only justified for non-standard contracts (e.g., when the return type must implement `ISubject` or a richer interface). - -### §6.4. Protect Calls to User Code -**Wrap every user-provided delegate in try/catch and route exceptions to `observer.OnError`:** -- Selector functions (Transform, Select, etc.) -- Predicates (Filter, Where, etc.) -- Comparers (Sort, GroupOn, etc.) -- Key selectors (GroupOn, ToObservableChangeSet, etc.) -- Action callbacks (Do, OnItemRemoved, SubscribeMany, etc.) -- Calls to dictionaries/lists/hashsets that use a user-provided comparer - -**DO NOT wrap these (they are "edge of the monad"):** Subscribe, Dispose, OnNext, OnError, OnCompleted. Calling OnError from these places leads to undefined behavior. - -**Exception:** `IScheduler` implementations protect inside the scheduler itself, not at every call site. - -### §6.5. Subscribe Implementations Must Not Throw -- Subscribe may be called asynchronously (e.g., second source in `Concat` is subscribed after first completes). -- A throw crashes the application: no observer in scope to route to. -- Error conditions detected in Subscribe MUST be routed via `observer.OnError(...)` followed by `return Disposable.Empty;`. - -### §6.6. OnError Has Abort Semantics -- Once a source emits OnError, the operator MUST emit no further messages, not even buffered or aggregated state. -- **Example: a buffering operator must DISCARD its buffer on source error, not flush it.** The semantic is "abort", not "graceful degradation". -- Operators that aggregate state across multiple OnNext (Sort, Group, Page, etc.) must not "salvage" that state into a final OnNext when OnError arrives. - -### §6.7. Multi-Source Operators MUST Serialize -When combining multiple sources into one output observer: -```csharp -var gate = new object(); -source1.Synchronize(gate).Subscribe(observer); -source2.Synchronize(gate).Subscribe(observer); -``` -OR equivalent gate-based pattern (DynamicData's `CacheParentSubscription._synchronize` is the canonical example for cache operators). - -**All three notification types (OnNext / OnError / OnCompleted) must be serialized through the same gate**, not just OnNext. - -### §6.8. Single-Source Operators MUST NOT Redundantly Serialize -- Per §6.7 every operator already serializes; downstream operators can ASSUME serialized input. -- Adding `Synchronize` "just in case" is an anti-pattern: it clutters, harms performance, and signals misunderstanding of the contract. -- `Synchronize` at the consumer-facing boundary (per §5.8) is reserved for fixing genuinely non-conforming external sources. - -### §6.9 / §6.10 / §6.11. Parameterize Concurrency via Scheduler -- Concurrency-introducing operators take an `IScheduler` parameter (§6.9). -- Provide an overload with a sensible default scheduler (§6.10; prefer Immediate where possible per §6.12). -- The `IScheduler` parameter is the **LAST** argument for fluent IntelliSense (§6.11). -- Exception: `params T[]` operators take the scheduler as the second-to-last argument. - -### §6.12. Avoid Introducing Concurrency Where Possible -- Adding concurrency skews timing data: delivery time is itself observable data. -- Default schedulers should be the minimal-concurrency option (often `Scheduler.Immediate`). -- Only introduce concurrency when it is essential to the operator's semantics. - -### §6.13. Hand Out All Disposables -- Every `IDisposable` created inside an operator (subscription disposables, scheduled-action disposables, resource handles) MUST be reachable through the disposable returned to the subscriber. -- Use `CompositeDisposable`, `SerialDisposable`, `SingleAssignmentDisposable`, `RefCountDisposable` to compose. -- Hidden disposables = leaks at unsubscribe time and broken backpressure-style cleanup. - -### §6.14. Operators Must Not Block -- Return `IObservable`, never `T`. -- Even aggregation operators (Sum, Count, etc.) return `IObservable`: the caller can use `First*`/`Last*`/`Single*` to escape the observable world if blocking is genuinely needed at the call site. - -### §6.15. Avoid Deep Recursion in Operators -- Stack depth at operator invocation is unknown; recursive operators can blow the stack faster than expected. -- Use `IScheduler.Schedule(self => ...)` recursive overload OR `IEnumerable>` + `Concat` with a `yield` iterator. - -### §6.16. Argument Validation Outside Observable.Create -- Per §6.5, `Observable.Create`'s subscribe lambda must not throw. -- Therefore: `ArgumentNullException` and other validation checks belong BEFORE the `Observable.Create(...)` call. -- Exception: validation that genuinely requires the subscription to be live (rare). - -### §6.17. Idempotent Dispose -- Multiple `Dispose()` calls on the same disposable: first runs cleanup, subsequent are no-ops. -- Required because consumers don't know subscription state and may dispose defensively. -- Use a `_disposedValue` flag or equivalent guard. - -### §6.18. Dispose Must Not Throw -- Dispose chains cascade through compositions. -- A throw crashes the app, and the observer is already unsubscribed, so OnError cannot route the exception. -- If cleanup can fail, swallow + log; never propagate. -- When disposing multiple children, wrap each in try/catch so one failure doesn't skip the rest. - -### §6.19. Custom IObservable Implementations MUST Follow the Contract -- If you can't use `Observable.Create`, you take on the burden of contract compliance yourself. -- All of §4 applies to your implementation. - -### §6.20. Operator Implementations MUST Follow Usage Guidelines -- Operators internally compose other operators (per §6.1). -- §5 ("Using Rx") rules apply recursively to operator internals. - ---- - -## Common violation patterns (quick scan during code review) - -| Violation shape | Rule | Detection cue | -|---|---|---| -| Buffering operator flushes buffer on OnError | §6.6 | `OnError` handler calls OnNext before propagating | -| Multi-source operator forgets to serialize OnCompleted (only serializes OnNext) | §6.7 | `Synchronize` applied to OnNext but raw OnCompleted/OnError | -| Single-source operator wraps OnNext in unnecessary `lock(gate)` | §6.8 | `lock` inside a Transform/Filter/Select implementation | -| `null` check inside `Observable.Create(obs => { if (x == null) throw ... })` | §6.16 | Validation inside the subscribe lambda | -| Subscribe lambda throws on error condition rather than routing to OnError | §6.5 | `throw` inside `Observable.Create(observer => { ... throw ... })` | -| Dispose method throws (or contains code that can throw without catch) | §6.18 | IDisposable.Dispose without try/finally; multi-child dispose without per-child try/catch | -| Re-entrant Dispose runs cleanup twice | §6.17 | Missing `_disposedValue` flag or equivalent idempotency guard | -| Scheduler-queued work fires OnNext after Dispose | §4.4 | `IScheduler.Schedule(() => observer.OnNext(x))` without checking subscription state | -| User callback (selector, comparer, predicate, action) not protected by try/catch | §6.4 | Direct invocation of user delegate inside operator without wrapping | -| Recursive operator without `Schedule(self)` or `Concat`+yield | §6.15 | Direct self-recursion in OnNext handler | -| Hidden disposable not exposed to caller | §6.13 | `_ = scheduler.Schedule(...)` discarding the return value | -| Sum/Aggregate returns `T` instead of `IObservable` | §6.14 | Blocking return type on what should be an operator | -| Argument validation missing entirely on a public extension method | §6.16 | No null check on extension method parameters | - ---- - -## How to use this reference - -### When writing or modifying an operator -1. After implementing the operator, walk every rule in §4 and §6. -2. For each rule, confirm the operator does not violate it. -3. Document the audit in the PR description (e.g., "Audited against §4.1-§4.4, §6.1, §6.4-§6.8, §6.13, §6.16-§6.18; §6.7 N/A (single-source operator)"). - -### When reviewing a PR -1. Use the "Common violation patterns" table as a scanning checklist. -2. Cite rule IDs in review comments (e.g., "§6.6: buffer should be discarded on error, not flushed"). -3. Block merge on any unaddressed rule violation. - -### When debugging an Rx-related bug -1. Identify the rule(s) most likely to explain the symptom. -2. Add tests that exercise the contract boundary (use `Tests/Utilities/ValidateSynchronization` for §4.2, `TestSourceCache.SetError` for §4.4 / §6.6, etc.). -3. Reference the rule ID in the fix commit message. - -### Citing rules - -Always use the literal `§` character when citing rule IDs (in commit messages, PR descriptions, code review comments, and code comments). Do NOT use ASCII substitutes (`S`, `SS`, `sec`, `§§`, etc.). Example commit subject: - -``` -Fix §6.6 violation in BatchIf - -BatchIf was flushing its buffered changeset on source OnError, which -violates §6.6's abort semantic. Discard the buffer instead so error -deliveries are not preceded by stale OnNext. -``` - ---- - -## Maintaining this document - -This file is a derivative of the [Microsoft Rx Design Guidelines v1.0 (October 2010)](https://go.microsoft.com/fwlink/?LinkID=205219) (canonical fwlink; resolves to `download.microsoft.com/.../Rx Design Guidelines.pdf`). The Microsoft document has not been republished since v1.0 and the underlying Rx contract is stable. **Consult the PDF directly when revising rule text, adding new rules, or resolving disputes about intent.** - -**Do NOT modify rule IDs.** External code reviews, PR descriptions, and commit messages cite the `§X.Y` IDs; renumbering breaks those references. The IDs come from the original PDF and changing them severs traceability to the source spec. - -**DO add new rules** in a new section (e.g., a DynamicData-specific section that goes beyond standard Rx) if the codebase discovers patterns the Microsoft document doesn't cover. Use a non-numeric prefix (e.g., `DD-1`, `DD-2`) to make clear they are not from the original spec. - -**Cross-references:** see `rx.instructions.md` for prose-style teaching of the same concepts plus DynamicData-specific patterns (Defer for per-subscription state, Observable.Create when truly needed, disposable family reference, common pitfalls). diff --git a/.github/instructions/rx-design-guide.instructions.md b/.github/instructions/rx-design-guide.instructions.md new file mode 100644 index 00000000..c6787aa2 --- /dev/null +++ b/.github/instructions/rx-design-guide.instructions.md @@ -0,0 +1,798 @@ +--- +applyTo: "**/*.cs" +--- +# Rx Design Guide + +A complete distillation of the **[Microsoft Rx Design Guidelines v1.0 (October 2010)](https://go.microsoft.com/fwlink/?LinkID=205219)** (canonical fwlink; resolves to `download.microsoft.com/.../Rx Design Guidelines.pdf`). This is the authoritative reference for the Rx contract and the rules for using Rx and implementing Rx operators in DynamicData. + +**This document is meant to be self-contained.** Future contributors and agents should not have to re-consume the PDF: the entire spec (every rule, every "When to ignore" caveat, the clarifying samples) is preserved here. Samples have been modernized to current Rx.NET API names; the PDF's original 2010 names appear in the Modernization notes table below. + +**Every operator added or modified must self-audit against these rules. Every bug fix must explicitly state which rules were verified.** + +Rules use the **original Microsoft document's `§X.Y` numbering** throughout so citations are unambiguous and traceable to the source spec. Cite rule IDs in code review comments, PR descriptions, and commit messages (e.g., "Fixes §6.6 violation in `BatchIf`"). Always use the literal `§` character — never ASCII substitutes (`S`, `SS`, `sec`, etc.). + +--- + +## Document structure + +The file follows the original Microsoft document's section order: +- **§2** introduces what the guidelines are and how to read them. +- **§3** is "When to use Rx" — applicability guidelines for choosing Rx in the first place. +- **§4** is the Observable Contract from the consumer's perspective (what callers can rely on). +- **§5** is rules for code that uses Rx (consumer-side correctness). +- **§6** is rules for operator authors (what your operator must guarantee). + +§1 (the PDF's table of contents) is omitted here as it carries no rules. + +--- + +## Modernization notes + +This document is derived from a 2010 specification but uses **current Rx.NET API names throughout**. Where the original PDF references obsolete names, this document uses the modern equivalents. Do NOT use the obsolete names in new code or fixes: + +| PDF / 2010 name (obsolete) | Modern equivalent | +|---|---| +| `Observable.CreateWithDisposable(Func, IDisposable>)` | `Observable.Create(Func, IDisposable>)` (same signature, name unified) | +| `Observable.Create` (2010 variant taking `Func, Action>`) | Still exists, but prefer the `IDisposable`-returning overload for resource lifetime correctness | +| `MutableDisposable` | `SerialDisposable` | +| `Scheduler.Dispatcher` | `DispatcherScheduler.Current` (WPF/WinForms) or platform-specific | +| `Scheduler.ThreadPool` | `Scheduler.Default` or `TaskPoolScheduler.Default` | +| `Scheduler.NewThread` | `NewThreadScheduler.Default` | +| `Observable.FromEvent(d => d.Invoke, add, remove)` (3-arg form) | `Observable.FromEventPattern(add, remove)` (typed event pattern) | +| `Observable.Start(Func)` for async work | `Observable.FromAsync(Func>)` for proper async/await integration | +| `IScheduler.Schedule(Action)` (interface method) | `IScheduler.Schedule(this IScheduler, Action)` extension method (same usage, now an extension) | + +Modern additions worth knowing (not in the 2010 PDF): +- **`SingleAssignmentDisposable`**: assign once, throws on second assignment. Useful when a disposable is wired up asynchronously after subscription returns. +- **`RefCountDisposable`**: reference-counted disposable; underlying resource disposed only when all dependents released. +- **`IScheduler.Schedule(TState, Func)`** generic overload: avoids closure allocation in hot paths. Prefer this in performance-sensitive operators. +- **`ValueTask` / `IAsyncEnumerable` interop**: `ToObservable()`, `ToAsyncEnumerable()` bridges. +- **`System.Threading.Lock`** (NET9+): modern lock type used by DynamicData's `Rxx.Synchronize` on NET9+. + +**Keep this table current.** When you encounter a 2010 API name in the PDF that doesn't yet appear here (because Rx.NET has since renamed, removed, or supplemented it), add a row. If you find an obsolete API name used anywhere in this codebase, that is itself a finding to surface. + +--- + +## §2: Introduction + +These guidelines aid in developing applications and libraries that use Rx. They have evolved over time during the development of the Rx library, and continue to evolve as Rx evolves. + +The guidelines are **not absolute truths.** They are patterns the Rx team found helpful, not rules to be followed blindly. There are situations where certain guidelines do not apply. The PDF lists known exceptions for each rule in its "When to ignore this guideline" sections, which are preserved here verbatim. + +The guidelines are listed in **no particular order.** There is neither total nor partial ordering between them. The §X.Y numbering is a stable ID for citation purposes only, not a priority ranking. + +--- + +## §3: When to use Rx + +### §3.1. Use Rx for orchestrating asynchronous and event-based computations + +Code that deals with more than one event or asynchronous computation gets complicated quickly: it needs a state machine for ordering, and explicit handling of success and failure termination for every separate computation. The result is code that doesn't follow normal control flow, is hard to understand, and is hard to maintain. + +Rx makes these computations first-class citizens. This provides a model for readable, composable APIs over asynchronous computations. + +**Sample (modernized to current Rx.NET API)** — autocomplete-style "dictionary suggest" that throttles user input and cancels stale lookups: +```csharp +var keyUp = Observable.FromEventPattern(textBox, nameof(textBox.KeyUp)); + +var dictionarySuggest = keyUp + .Select(_ => textBox.Text) + .Where(text => !string.IsNullOrEmpty(text)) + .DistinctUntilChanged() + .Throttle(TimeSpan.FromMilliseconds(250), uiScheduler) + .SelectMany(text => AsyncLookupInDictionary(text).TakeUntil(keyUp)); + +dictionarySuggest.Subscribe( + results => listView.Items.AddRange(results.Select(r => new ListViewItem(r)).ToArray()), + error => LogError(error)); +``` +`Throttle` collapses bursts of keystrokes, `SelectMany` flattens the per-text async lookups, `TakeUntil(keyUp)` cancels in-flight lookups when the user types again. In imperatively-written code each of these would be a separate timer or callback with explicit exception bookkeeping. + +**When to ignore this guideline:** if the application has very few asynchronous/event-based operations or few places where they need to be composed, the cost of depending on Rx (redistribution and the learning curve) might outweigh the cost of coding the operations manually. + +### §3.2. Use Rx to deal with asynchronous sequences of data + +Several other .NET asynchronous libraries exist, but most work best for operations that return a single message. They usually do not support operations that produce **multiple** messages over the lifetime of the operation. + +Rx follows the grammar `OnNext* (OnCompleted | OnError)?` (see §4.1). This makes Rx suitable both for operations that produce a single message and operations that produce multiple messages. + +**Sample (modernized)** — pipelined encryption of a 4 GB file using 64K blocks (no full-file buffering): +```csharp +inFile.AsyncRead(blockSize: 2 << 15) + .Select(Encrypt) + .WriteToStream(outFile) + .Subscribe( + _ => Console.WriteLine("Successfully encrypted the file."), + error => Console.WriteLine($"An error occurred while encrypting: {error.Message}")); +``` +Each 64K block flows through the pipeline as a separate `OnNext`. Encryption runs per block; writes happen as soon as a block is encrypted. Memory usage stays bounded, throughput stays high. + +**When to ignore this guideline:** if the application has very few operations with multiple messages, the cost of depending on Rx might outweigh the cost of coding them manually. + +--- + +## §4: The Rx contract + +`IObservable` and `IObserver` only specify their methods' arguments and return types. The Rx library makes additional assumptions about these interfaces that are not expressible in the .NET type system. These assumptions form a contract that **all producers and consumers** of Rx types must follow. The contract makes it possible to reason about and prove the correctness of operators and user code. + +### §4.1. Assume the Rx Grammar + +Messages sent to instances of `IObserver` follow the grammar: +``` +OnNext* (OnCompleted | OnError)? +``` +- Zero or more `OnNext`, optionally followed by exactly one terminal notification. +- `OnError` and `OnCompleted` are **mutually exclusive**: emit one or the other, never both. +- After a terminal notification, **no further notifications of any kind**, not even another terminal. + +The single terminal message ensures that consumers can deterministically establish when it is safe to perform cleanup. A single failure further ensures that abort semantics can be maintained for operators that work on multiple observable sequences (see §6.6). + +**When to ignore this guideline:** only when working with a non-conforming `IObservable` implementation. Such a source can be made conforming by calling `Synchronize()` on it (per §5.8). + +### §4.2. Assume observer instances are called in a serialized fashion + +Rx uses a push model and .NET supports multithreading, so without serialization different messages could arrive on different execution contexts at the same time. Forcing every consumer to handle this would require pervasive housekeeping, hurt maintainability, and harm performance. + +Only the operators that produce multi-source observable sequences are required to perform serialization (see §6.7). **Consumers can safely assume that messages on a single observer arrive in a serialized fashion.** + +```csharp +var count = 0; +xs.Subscribe(v => +{ + count++; + Console.WriteLine($"OnNext has been called {count} times."); +}); +``` +No locking or interlocking is required to read or write `count`: only one call to `OnNext` can be in-flight at a time. + +**When to ignore this guideline:** if you have to consume a custom `IObservable` implementation that doesn't follow the contract for serialization, use `Synchronize()` to restore the guarantee. + +### §4.3. Assume resources are cleaned up after an OnError or OnCompleted message + +§4.1 states that no more messages should arrive after a terminal. This makes it possible to clean up any resources used by the subscription the moment the terminal arrives. Cleaning up immediately makes side effects predictable and lets the runtime reclaim resources promptly. + +```csharp +Observable.Using( + () => new FileStream(path, FileMode.Create), + fs => Observable.Range(0, 10000) + .Select(v => Encoding.ASCII.GetBytes(v.ToString())) + .WriteToStream(fs)) + .Subscribe(); +``` +`Using` creates a resource that will be disposed upon unsubscription. The cleanup guarantee ensures unsubscription is called automatically once a terminal arrives. + +**When to ignore this guideline:** none known. + +### §4.4. Assume a best effort to stop all outstanding work on Unsubscribe + +When `Dispose` is called on a subscription, the source makes a best-effort attempt to stop all outstanding work. +- Queued work that has not yet started is cancelled. +- Work already in progress **may** still complete (it is not always safe to abort), but its results **MUST NOT** be signaled to the unsubscribed observer. +- Messages may arrive during the `Dispose` call itself (Dispose can race with `OnNext`). +- After `Dispose` returns control to the caller: no more messages arrive. +- The unsubscription process may continue asynchronously on a different context after `Dispose` returns. + +```csharp +// Sample 1: queued work is cancelled — observer never fires +Observable.Timer(TimeSpan.FromSeconds(2)).Subscribe(...).Dispose(); + +// Sample 2: in-progress work runs to completion, but its result is dropped +Observable.Start(() => { Thread.Sleep(TimeSpan.FromSeconds(2)); return 5; }) + .Subscribe(...).Dispose(); +``` + +--- + +## §5: Using Rx + +These rules govern code that consumes Rx. They apply recursively inside operator implementations too, per §6.20. + +### §5.1. Consider drawing a Marble-diagram + +Draw a marble diagram of the observable sequence you want to create. By drawing the diagram, you get a clearer picture of which operators to use. + +A marble diagram shows events occurring over time, with both input and output sequences. Sketching one often makes the answer obvious: a "delay then call" pattern maps to `Throttle`, a "create a new sequence per input" pattern maps to `SelectMany`, and so on. + +**When to ignore this guideline:** when you are comfortable enough with the sequence you want to write. Even Rx team members still reach for a whiteboard occasionally. + +### §5.2. Consider passing multiple arguments to Subscribe + +Rx provides Subscribe overloads that take delegates instead of an `IObserver`, because C# and VB do not support anonymous inner classes. These overloads use defaults for any method you omit. + +The single-argument `Subscribe(onNext)` overload **rethrows OnError on the thread the message arrives on**, crashing the application. The no-argument `OnCompleted` default is a no-op. In most situations, dealing with the exception (either recovering or aborting gracefully) and knowing the sequence completed successfully are both important, so it is best to provide all three arguments. + +**When to ignore this guideline:** +- The observable sequence is guaranteed not to complete (e.g. a UI event like `KeyUp`). +- The observable sequence is guaranteed not to error (e.g. an event, a materialized sequence, etc.). +- The default behavior is the desired behavior. + +### §5.3. Consider using LINQ query expression syntax + +Rx implements the query expression pattern, so LINQ query syntax can be used over observable sequences: +```csharp +// Method syntax +var r = xs.SelectMany(x => ys, (x, y) => x + y); + +// Equivalent query syntax +var r1 = from x in xs + from y in ys + select x + y; +``` + +**When to ignore this guideline:** if your query uses many operators that don't have query-syntax equivalents, the mixed style may negate the readability benefit. + +### §5.4. Consider passing a specific scheduler to concurrency-introducing operators + +Rather than using `ObserveOn` to change execution context, create concurrency on the right scheduler from the start. Operators that introduce concurrency provide a scheduler argument overload. Passing the right scheduler upfront eliminates downstream `ObserveOn` hops. + +```csharp +var keyUp = Observable.FromEventPattern(textBox, nameof(textBox.KeyUp)); +var throttled = keyUp.Throttle(TimeSpan.FromSeconds(1), DispatcherScheduler.Current); +``` +Without the explicit scheduler, the default `Throttle` overload would deliver `OnNext` from a ThreadPool timer. Passing the dispatcher scheduler keeps all messages on the UI thread. + +**When to ignore this guideline:** when combining several events that originate on different execution contexts, use §5.5 to put all messages on a specific context as late as possible. + +### §5.5. Call the ObserveOn operator as late and in as few places as possible + +`ObserveOn` schedules an action per message. This changes timing and adds load. Placing it later in the query (after filtering) reduces both concerns. + +```csharp +var result = + (from x in xs.Throttle(TimeSpan.FromSeconds(1)) + from y in ys.TakeUntil(zs).Sample(TimeSpan.FromMilliseconds(250)) + select x + y) + .Merge(ws) + .Where(x => x.Length < 10) + .ObserveOn(DispatcherScheduler.Current); +``` +Placing `ObserveOn` earlier would do scheduling work for messages that the `Where` filter throws away. + +**When to ignore this guideline:** if your use of the observable sequence is not bound to a specific execution context, do not use `ObserveOn` at all. + +### §5.6. Consider limiting buffers + +Rx comes with operators that buffer over observable sequences (e.g. `Replay`). The buffer size scales with the input sequence. If unbounded, the buffer causes memory pressure. + +Most buffering operators provide policies to limit the buffer by time or size: +```csharp +var result = xs.Replay(bufferSize: 10000, window: TimeSpan.FromHours(1)); +``` + +**When to ignore this guideline:** when the number of messages populating the buffer is small, or when the buffer is already bounded by its surrounding context. + +### §5.7. Make side-effects explicit using the Do operator + +Rx operators take delegates as arguments, and it is possible to put side-effecting code (writes to disk, mutation of global state, logging) into those delegates. Because Rx composition runs each operator for each subscription (except sharing operators like `Publish`), every side-effect occurs for every subscription. + +If you want side effects per subscription, **make that explicit by putting the side-effecting code in a `Do` operator** so it's visible in the pipeline. + +```csharp +var result = xs.Where(x => x.Failed).Do(x => Log(x)).Subscribe(...); +``` + +**When to ignore this guideline:** when the side effect needs data from an operator that is not available to a separate `Do`. + +### §5.8. Use the Synchronize operator only to "fix" custom IObservable implementations + +Observable sequences created by Rx operators already follow the contract for grammar (§4.1) and serialization (§4.2). Adding `Synchronize` to one of them is redundant and counterproductive. **Only use `Synchronize` on observable sequences created by other sources that do not follow the contract.** + +```csharp +var result = from x in customNonConformingSource.Synchronize() + from y in ys + where x > y + select y; +``` + +**When to ignore this guideline:** none known. + +> NOTE: this guideline refers to the **single-argument `Synchronize()`** that wraps a non-conforming source. The **gate-based `Synchronize(gate)`** used inside multi-source operators to satisfy §6.7 is a different pattern and is valid. + +### §5.9. Assume messages can come through until unsubscribe has completed + +Because Rx uses a push model, messages can be sent from different execution contexts and can be in flight while `Dispose` is called. These messages **may still come through during the call to `Dispose`**. After `Dispose` returns control to the caller, no more messages will arrive. The unsubscription process itself may still be in progress on a different context. + +**When to ignore this guideline:** once `OnCompleted` or `OnError` has been received, the Rx grammar guarantees the subscription is finished, so this concern doesn't apply. + +### §5.10. Use the Publish operator to share side-effects + +Most observable sequences are cold: each subscription gets its own set of side effects. Some situations require side effects to happen only once. `Publish` shares a single underlying subscription among multiple subscribers via multicast. + +The most convenient overloads are the ones that accept a function with a shared observable sequence as an argument: +```csharp +var xs = Observable.Create(observer => +{ + Console.WriteLine("Side effect"); + observer.OnNext("Hi"); + observer.OnCompleted(); + return Disposable.Empty; +}); + +xs.Publish(sharedXs => +{ + sharedXs.Subscribe(Console.WriteLine); + sharedXs.Subscribe(Console.WriteLine); + return sharedXs; +}).Run(); +``` +The "Side effect" line prints once, not twice, because both subscribers share a single subscription to `xs`. + +**When to ignore this guideline:** only use `Publish` when sharing is required. In most situations either the subscriptions have no side effects, or the side effects can execute multiple times without issues, and the extra machinery of `Publish` is unnecessary. + +--- + +## §6: Operator implementations + +### §6.1. Implement new operators by composing existing operators + +Many operations can be composed from existing operators. This leads to smaller, easier-to-maintain code. The Rx team has put significant effort into corner cases in the base operators; composing them reuses that work for free. + +```csharp +public static IObservable SelectMany( + this IObservable source, + Func> selector) +{ + return source.Select(selector).Merge(); +} +``` +`Select` already handles selector-thrown exceptions; `Merge` already handles serialization of inner sequences firing concurrently. The composed `SelectMany` gets both for free. + +**When to ignore this guideline:** +- No appropriate set of base operators exists. +- Performance analysis proves the composed implementation has performance issues. + +### §6.2. Implement custom operators using Observable.Create + +When you cannot follow §6.1, use `Observable.Create` to create the observable sequence. `Observable.Create` provides several contract-protection benefits: +- When the sequence finishes (`OnError` or `OnCompleted`), any subscription is automatically unsubscribed. +- Any subscribed observer instance will only see a single terminal message. No further messages are sent, enforcing the grammar of §4.1. + +> **Modernization note:** the PDF refers to `Observable.CreateWithDisposable(Func, IDisposable>)`. In modern Rx.NET this is `Observable.Create(Func, IDisposable>)` (same signature, name unified). There is also an `Action`-returning overload; prefer the `IDisposable`-returning one for resource lifetime correctness. + +**When to ignore this guideline:** +- The operator needs to return an observable sequence that doesn't follow the Rx contract (rare; usually only for testing how code behaves under broken contracts). +- The returned object must implement more than `IObservable` (e.g. `ISubject` or a custom class). + +### §6.3. Implement operators for existing observable sequences as generic extension methods + +An operator becomes more powerful when it can be applied widely. Implement operators as **extension methods** so they appear in IntelliSense on any existing observable sequence, and make them **generic** so they work regardless of element type. + +**When to ignore this guideline:** +- The operator doesn't work on a source observable sequence. +- The operator works on a specific data shape and cannot be made generic. + +### §6.4. Protect calls to user code from within an operator + +When user code is called from within an operator, the call may happen outside the execution context of the original operator invocation (e.g. asynchronously on a scheduler). Any exception that escapes will terminate the program unexpectedly. Instead, **catch and route to `observer.OnError`** so subscribers can handle it. + +Common kinds of user code that must be protected: +- Selector functions passed to the operator. +- Predicates passed to the operator. +- Comparers passed to the operator. +- Key selectors passed to the operator. +- Action callbacks (`Do`, `OnItemRemoved`, `SubscribeMany`, etc.) +- Calls to dictionaries, lists, and hashsets that use a user-provided comparer. + +```csharp +return Observable.Create(observer => source.Subscribe( + x => + { + TResult result; + try { result = selector(x); } + catch (Exception exception) + { + observer.OnError(exception); + return; + } + observer.OnNext(result); + }, + observer.OnError, + observer.OnCompleted)); +``` + +> NOTE: calls to `IScheduler` implementations are **not** considered for this guideline. Most schedulers deal with asynchronous calls, so only a small set of issues would be caught at the call site. Instead, protect the arguments passed to schedulers **inside each scheduler implementation**. + +**Edge of the monad:** do **not** wrap calls to `Subscribe`, `Dispose`, `OnNext`, `OnError`, or `OnCompleted`. Calling `OnError` from these places leads to undefined behavior. + +**When to ignore this guideline:** for calls to user code made before creating the observable sequence (outside `Observable.Create`). Those calls are on the current execution context and follow normal control flow. + +### §6.5. Subscribe implementations should not throw + +Subscribe may be called asynchronously (e.g. the second source argument to `Concat` is subscribed to only after the first source completes). A throw at that moment would bring down the program because there is no observer in scope to route to. Instead, **error conditions detected inside `Subscribe` must be routed via `observer.OnError(...)` followed by `return Disposable.Empty;`**. + +```csharp +public IObservable ReadSocket(Socket socket) => + Observable.Create(observer => + { + if (!socket.Connected) + { + observer.OnError(new InvalidOperationException("the socket is no longer connected")); + return Disposable.Empty; + } + // ... rest of subscribe ... + }); +``` + +**When to ignore this guideline:** when a catastrophic error occurs that should bring the whole program down anyway. + +### §6.6. OnError messages should have abort semantics + +Normal .NET control flow uses abort semantics for exceptions: the stack is unwound, current code is interrupted. Rx mimics this. Once a source emits `OnError`, **the operator MUST emit no further messages — not even buffered or aggregated state.** + +The canonical violation: a buffering operator that "salvages" its buffer into a final `OnNext` on `OnError`. The buffer must be **discarded**: +```csharp +public static IObservable MinimumBuffer(this IObservable source, int bufferSize) => + Observable.Create(observer => + { + var data = new List(); + return source.Subscribe( + value => + { + data.AddRange(value); + if (data.Count > bufferSize) + { + observer.OnNext(data.ToArray()); + data.Clear(); + } + }, + observer.OnError, + () => + { + if (data.Count > 0) + observer.OnNext(data.ToArray()); + observer.OnCompleted(); + }); + }); +``` +The `OnCompleted` branch flushes the buffer (success path), but the `OnError` branch is direct passthrough: it abandons the buffer. The semantic is **abort**, not graceful degradation. + +Operators that aggregate state across multiple `OnNext` (Sort, Group, Page, etc.) must not salvage that state on `OnError`. + +**When to ignore this guideline:** none known. + +### §6.7. Serialize calls to IObserver methods within observable sequence implementations + +Rx is composable: many operators play together. If every operator had to deal with concurrency individually, they would become very complex. Concurrency is best controlled at the place it first occurs. Consuming Rx would become harder if every consumer had to deal with concurrency too. + +**When combining multiple sources into one output observer, serialize all three notification types through a shared gate**: +```csharp +public static IObservable ZipEx( + this IObservable left, + IObservable right, + Func resultSelector) +{ + return Observable.Create(observer => + { + var group = new CompositeDisposable(); + var gate = new object(); + var leftQ = new Queue(); + var rightQ = new Queue(); + + group.Add(left.Subscribe( + value => + { + lock (gate) + { + if (rightQ.Count > 0) + { + var rightValue = rightQ.Dequeue(); + TResult result; + try { result = resultSelector(value, rightValue); } + catch (Exception e) { observer.OnError(e); return; } + observer.OnNext(result); + } + else + { + leftQ.Enqueue(value); + } + } + }, + observer.OnError, + observer.OnCompleted)); + + // ... symmetric handler for `right` using the same `gate` ... + + return group; + }); +} +``` +Two sources, one gate. **All three notification types — `OnNext`, `OnError`, `OnCompleted` — must be serialized through the same gate**, not just `OnNext`. Without this, the operator's internal state (the two queues) could be corrupted by interleaved deliveries. + +The equivalent pattern using `Synchronize(gate)` is more concise: +```csharp +var gate = new object(); +source1.Synchronize(gate).Subscribe(observer); +source2.Synchronize(gate).Subscribe(observer); +``` +DynamicData's `CacheParentSubscription._synchronize` is the canonical example for cache operators. + +**When to ignore this guideline:** +- The operator works on a single source observable sequence (then §6.8 applies). +- The operator does not introduce concurrency. +- Other constraints guarantee no concurrency is in play. + +> NOTE: If a source observable sequence breaks the contract, a developer can fix it before passing it to an operator by calling `Synchronize()` (per §5.8). + +### §6.8. Avoid serializing operators + +Per §6.7 every operator already serializes; downstream operators can **assume** serialized input. Adding `Synchronize` "just in case" clutters the code, harms performance, and signals misunderstanding of the contract. + +If a source isn't following the contract, fix it at the consumer boundary with `Synchronize()` (per §5.8). The scope of additional synchronization should be limited to where it is needed. + +```csharp +// Select assumes its source follows §6.7 and requires no additional locking +public static IObservable Select( + this IObservable source, Func selector) => + Observable.Create(observer => source.Subscribe( + x => + { + TResult result; + try { result = selector(x); } + catch (Exception e) { observer.OnError(e); return; } + observer.OnNext(result); + }, + observer.OnError, + observer.OnCompleted)); +``` + +**When to ignore this guideline:** none known. + +### §6.9. Parameterize concurrency by providing a scheduler argument + +There are many different notions of concurrency, and no scenario fits all, so it is best to **parameterize the concurrency an operator introduces** via the `IScheduler` interface. + +```csharp +public static IObservable Return(TValue value, IScheduler scheduler) => + Observable.Create(observer => + scheduler.Schedule(() => + { + observer.OnNext(value); + observer.OnCompleted(); + })); +``` + +**When to ignore this guideline:** +- The operator is not in control of creating the concurrency (e.g. an operator converting an event to an observable: the event source is in control, not the operator). +- The operator is in control but needs to use a specific scheduler for introducing concurrency. + +### §6.10. Provide a default scheduler + +In most cases there is a good default scheduler. Providing it as an overload makes calling code more succinct. + +```csharp +public static IObservable Return(TValue value) => + Return(value, Scheduler.Immediate); +``` + +> NOTE: when choosing the default, follow §6.12 — use `Scheduler.Immediate` where possible, only choose a scheduler with more concurrency when needed. + +**When to ignore this guideline:** when no good default can be chosen. + +### §6.11. The scheduler should be the last argument to the operator + +Putting the scheduler last makes the operator fluent in IntelliSense. Combined with §6.10's default-providing overload, adding or omitting a scheduler becomes natural without changing argument order. + +```csharp +public static IObservable Return(TValue value) => Return(value, Scheduler.Immediate); +public static IObservable Return(TValue value, IScheduler scheduler) => /* ... */; +``` + +**When to ignore this guideline:** C# and VB `params` syntax requires the `params` argument to be last. For `params T[]` operators, make the scheduler the **second-to-last** argument instead. + +### §6.12. Avoid introducing concurrency + +Adding concurrency changes the timeliness of a sequence: messages are scheduled to arrive later, and **delivery time is itself observable data**. Adding concurrency skews that data. + +This guideline includes not transferring control to a different context such as the UI context. + +```csharp +// Select does not use a scheduler — it stays on the source's OnNext call, +// staying in the same time window. +public static IObservable Select( + this IObservable source, Func selector) => + Observable.Create(observer => source.Subscribe( + x => + { + try { observer.OnNext(selector(x)); } + catch (Exception e) { observer.OnError(e); } + }, + observer.OnError, + observer.OnCompleted)); + +// Return defaults to Scheduler.Immediate — no concurrency introduced. +public static IObservable Return(TValue value) => Return(value, Scheduler.Immediate); +``` + +**When to ignore this guideline:** when introducing concurrency is an essential part of what the operator does. + +> NOTE: When using `Scheduler.Immediate` or calling the observer directly from within `Subscribe`, the `Subscribe` call becomes blocking. Any expensive computation in that situation is a candidate for introducing concurrency. + +### §6.13. Hand out all disposable instances created inside the operator to consumers + +Disposable instances control subscription lifetime and cancellation of scheduled actions. Rx gives users an opportunity to unsubscribe via disposable instances. After a subscription has ended, no more messages are allowed through, and any leftover state inside the sequence is inefficient and can lead to unexpected semantics. + +To compose multiple disposable instances, Rx provides the `System.Reactive.Disposables` namespace: + +| Name | Description | +|---|---| +| `CompositeDisposable` | Composes and disposes a group of disposable instances together. | +| `SerialDisposable` *(modern; PDF says `MutableDisposable`)* | A holder for a replaceable disposable; assigning a new disposable disposes the previous. | +| `BooleanDisposable` | Maintains state on whether disposing has occurred. | +| `CancellationDisposable` | Wraps `CancellationToken` into the disposable pattern. | +| `ContextDisposable` | Disposes an underlying disposable in a specified `SynchronizationContext`. | +| `ScheduledDisposable` | Uses a scheduler to dispose an underlying disposable. | +| `SingleAssignmentDisposable` *(modern, not in PDF)* | Assignable once; throws on second assignment. Useful when the disposable is wired up asynchronously after subscription returns. | +| `RefCountDisposable` *(modern, not in PDF)* | Reference-counted disposable; underlying resource disposed only when all dependents released. | + +```csharp +// Hand the group of internal subscriptions back to the subscriber via a CompositeDisposable +return Observable.Create(observer => +{ + var group = new CompositeDisposable(); + group.Add(left.Subscribe(/* ... */)); + group.Add(right.Subscribe(/* ... */)); + return group; +}); +``` + +**When to ignore this guideline:** none known. + +### §6.14. Operators should not block + +Rx is a library for composing asynchronous and event-based programs over observable collections. A blocking operator loses those characteristics, and potentially loses composability (e.g. by returning `T` instead of `IObservable`). + +```csharp +public static IObservable Sum(this IObservable source) => + source.Aggregate(0, (prev, curr) => checked(prev + curr)); +``` +`Sum` returns `IObservable` rather than `int`. It does not block, and the result remains composable. If the developer wants to escape the observable world, they can use `First*`, `Last*`, or `Single*`. + +**When to ignore this guideline:** none known. + +### §6.15. Avoid deep stacks caused by recursion in operators + +Code inside Rx operators can be called from different execution contexts in many different scenarios. It is nearly impossible to establish how deep the stack already is. A recursive operator can trigger stack overflow much sooner than expected. + +Two recommended ways to avoid this: +- Use the **recursive `Schedule` extension method** on `IScheduler`. +- Implement an **infinite-looping `IEnumerable>`** using `yield` and convert it via `Concat`. + +```csharp +// Sample 1: scheduler-based recursion +public static IObservable Repeat(TSource value, IScheduler scheduler) => + Observable.Create(observer => + scheduler.Schedule(self => + { + observer.OnNext(value); + self(); + })); + +// Sample 2: yield iterator + Concat +public static IObservable Repeat(TSource value) => + RepeatHelper(value).Concat(); + +private static IEnumerable> RepeatHelper(TSource value) +{ + while (true) + yield return Observable.Return(value); +} +``` +Schedulers such as the current-thread scheduler do not rely on stack semantics. The yield-iterator pattern ensures stack depth does not grow per iteration. + +**When to ignore this guideline:** none known. + +### §6.16. Argument validation should occur outside Observable.Create + +§6.5 requires that `Observable.Create`'s subscribe lambda not throw. Therefore **argument validation that may throw belongs outside the `Observable.Create(...)` call**: + +```csharp +public static IObservable Select( + this IObservable source, Func selector) +{ + if (source == null) throw new ArgumentNullException(nameof(source)); + if (selector == null) throw new ArgumentNullException(nameof(selector)); + + return Observable.Create(observer => source.Subscribe(/* ... */)); +} +``` + +**When to ignore this guideline:** when some aspect of the argument cannot be checked until the subscription is active (rare). + +### §6.17. Unsubscription should be idempotent + +The `IDisposable` returned from `Subscribe` doesn't expose subscription state. Consumers don't know whether they've already disposed, and may dispose defensively. Multiple `Dispose` calls **must be allowed**: the first runs the cleanup, subsequent calls are no-ops. + +```csharp +var subscription = xs.Subscribe(Console.WriteLine); +subscription.Dispose(); +subscription.Dispose(); // no-op, must not throw +``` +Use a `_disposedValue` flag or equivalent guard. + +**When to ignore this guideline:** none known. + +### §6.18. Unsubscription should not throw + +Rx composition chains subscriptions, which means it also chains unsubscriptions. Any operator can trigger an unsubscription at any time. A throw will crash the application, and because the observer is already unsubscribed, `OnError` cannot route the exception. + +- If cleanup can fail, swallow + log; never propagate. +- When disposing multiple children, wrap each in try/catch so one failure doesn't skip the rest. + +**When to ignore this guideline:** none known. + +### §6.19. Custom IObservable implementations should follow the Rx contract + +When you cannot follow §6.2 (use `Observable.Create`), the custom `IObservable` must still follow the Rx contract (§4) to behave correctly with Rx operators. + +**When to ignore this guideline:** only when writing observable sequences that need to break the contract on purpose (e.g. for testing how code behaves when the contract is broken). + +### §6.20. Operator implementations should follow guidelines for Rx usage + +Operators internally compose other operators (per §6.1). The §5 ("Using Rx") guidelines apply **recursively to operator internals**. + +**When to ignore this guideline:** as described in §2, only follow a guideline when it makes sense in that specific situation. + +--- + +## Common violation patterns (quick scan during code review) + +| Violation shape | Rule | Detection cue | +|---|---|---| +| Buffering operator flushes buffer on OnError | §6.6 | `OnError` handler calls OnNext before propagating | +| Multi-source operator forgets to serialize OnCompleted (only serializes OnNext) | §6.7 | `Synchronize` applied to OnNext but raw OnCompleted/OnError | +| Single-source operator wraps OnNext in unnecessary `lock(gate)` | §6.8 | `lock` inside a Transform/Filter/Select implementation | +| `null` check inside `Observable.Create(obs => { if (x == null) throw ... })` | §6.16 | Validation inside the subscribe lambda | +| Subscribe lambda throws on error condition rather than routing to OnError | §6.5 | `throw` inside `Observable.Create(observer => { ... throw ... })` | +| Dispose method throws (or contains code that can throw without catch) | §6.18 | `IDisposable.Dispose` without try/finally; multi-child dispose without per-child try/catch | +| Re-entrant Dispose runs cleanup twice | §6.17 | Missing `_disposedValue` flag or equivalent idempotency guard | +| Scheduler-queued work fires OnNext after Dispose | §4.4 | `IScheduler.Schedule(() => observer.OnNext(x))` without checking subscription state | +| User callback (selector, comparer, predicate, action) not protected by try/catch | §6.4 | Direct invocation of user delegate inside operator without wrapping | +| Recursive operator without `Schedule(self)` or `Concat`+yield | §6.15 | Direct self-recursion in OnNext handler | +| Hidden disposable not exposed to caller | §6.13 | `_ = scheduler.Schedule(...)` discarding the return value | +| Sum/Aggregate returns `T` instead of `IObservable` | §6.14 | Blocking return type on what should be an operator | +| Argument validation missing entirely on a public extension method | §6.16 | No null check on extension method parameters | + +--- + +## How to use this reference + +### When writing or modifying an operator +1. After implementing the operator, walk every rule in §4 and §6. +2. For each rule, confirm the operator does not violate it. +3. Document the audit in the PR description (e.g., "Audited against §4.1-§4.4, §6.1, §6.4-§6.8, §6.13, §6.16-§6.18; §6.7 N/A (single-source operator)"). + +### When reviewing a PR +1. Use the "Common violation patterns" table as a scanning checklist. +2. Cite rule IDs in review comments (e.g., "§6.6: buffer should be discarded on error, not flushed"). +3. Block merge on any unaddressed rule violation. + +### When debugging an Rx-related bug +1. Identify the rule(s) most likely to explain the symptom. +2. Add tests that exercise the contract boundary (use `Tests/Utilities/ValidateSynchronization` for §4.2, `TestSourceCache.SetError` for §4.4 / §6.6, etc.). +3. Reference the rule ID in the fix commit message. + +### Citing rules + +Always use the literal `§` character when citing rule IDs (in commit messages, PR descriptions, code review comments, and code comments). Do NOT use ASCII substitutes (`S`, `SS`, `sec`, `§§`, etc.). Example commit subject: + +``` +Fix §6.6 violation in BatchIf + +BatchIf was flushing its buffered changeset on source OnError, which +violates §6.6's abort semantic. Discard the buffer instead so error +deliveries are not preceded by stale OnNext. +``` + +--- + +## Maintaining this document + +This file is a derivative of the [Microsoft Rx Design Guidelines v1.0 (October 2010)](https://go.microsoft.com/fwlink/?LinkID=205219). The Microsoft document has not been republished since v1.0 and the underlying Rx contract is stable. **Consult the PDF directly when revising rule text, adding new rules, or resolving disputes about intent.** + +**Do NOT modify rule IDs.** External code reviews, PR descriptions, and commit messages cite the `§X.Y` IDs; renumbering breaks those references. The IDs come from the original PDF and changing them severs traceability. + +**DO add new rules** in a new section (e.g., a DynamicData-specific section that goes beyond standard Rx) if the codebase discovers patterns the Microsoft document doesn't cover. Use a non-numeric prefix (e.g., `DD-1`, `DD-2`) to make clear they are not from the original spec. + +**Keep the Modernization notes table current.** When a 2010 API name appears in the PDF that has since been renamed, removed, or supplemented in Rx.NET, add a row. + +**Cross-references:** see `rx.instructions.md` for DynamicData-specific patterns and practical Rx.NET reference material that complements this design guide (Defer pattern for per-subscription state, disposable family practical examples, the modern Rx.NET operator catalog, common pitfalls). That file links back here for any rule it touches on. diff --git a/.github/instructions/rx.instructions.md b/.github/instructions/rx.instructions.md index e82ab560..edd557f0 100644 --- a/.github/instructions/rx.instructions.md +++ b/.github/instructions/rx.instructions.md @@ -1,31 +1,33 @@ --- applyTo: "**/*.cs" --- -# Reactive Extensions (Rx) — Comprehensive Guide +# Reactive Extensions (Rx) — DynamicData Practical Guide Reference: [ReactiveX Observable Contract](http://reactivex.io/documentation/contract.html) | [Rx.NET GitHub](https://github.com/dotnet/reactive) | [IntroToRx.com](http://introtorx.com/) -**See also:** [`rx-contract.instructions.md`](./rx-contract.instructions.md) is the canonical rule reference using the original Microsoft Rx Design Guidelines numbering (`§X.Y` IDs like `§6.6`, `§5.2`). Cite those IDs in PR descriptions, code reviews, and commit messages. +**Authoritative reference: [`rx-design-guide.instructions.md`](./rx-design-guide.instructions.md)** is the complete distillation of the Microsoft Rx Design Guidelines (October 2010), with stable `§X.Y` rule IDs. Cite those IDs in PR descriptions, code reviews, and commit messages. This file covers practical, DynamicData-flavored material that complements the design guide: hot vs cold semantics, the modern Rx.NET scheduler/disposable APIs, an operator quick-reference, custom-operator patterns specific to this codebase, and common pitfalls. ## Core Concepts -### Observables are Composable +### Composition -Rx's power comes from composition. Every operator returns a new `IObservable`, enabling fluent chaining: +Rx's power comes from composition (per [§6.1](./rx-design-guide.instructions.md#61-implement-new-operators-by-composing-existing-operators)). Every operator returns a new `IObservable`, enabling fluent chaining: ```csharp source - .Where(x => x.IsValid) // filter - .Select(x => x.Transform()) // project - .DistinctUntilChanged() // deduplicate + .Where(x => x.IsValid) // filter + .Select(x => x.Transform()) // project + .DistinctUntilChanged() // deduplicate .ObserveOn(RxApp.MainThreadScheduler) // marshal to UI thread - .Subscribe(x => UpdateUI(x)); // consume + .Subscribe(x => UpdateUI(x)); // consume ``` -Each operator in the chain is a separate subscription. Disposing the final subscription cascades disposal upstream through the entire chain. This composability is what makes Rx powerful — and what makes contract violations devastating, since a bug in any operator corrupts the entire downstream chain. +Each operator is a separate subscription. Disposing the final subscription cascades disposal upstream through the entire chain. This composability is what makes Rx powerful, and what makes contract violations devastating: a bug in any operator corrupts the entire downstream chain. ### Hot vs Cold Observables +Not covered by the PDF. Critical to understand for DynamicData consumers because most cache pipelines are cold and incorrectly assuming hot semantics is a common source of duplicated work. + **Cold**: starts producing items when subscribed to. Each subscriber gets its own sequence. Created with `Observable.Create`, `Observable.Defer`, `Observable.Return`, etc. ```csharp @@ -40,7 +42,7 @@ var cold = Observable.FromAsync(() => httpClient.GetAsync(url)); var hot = Observable.FromEventPattern(button, nameof(button.Click)); ``` -**Converting**: `Publish()` + `Connect()` or `Publish().RefCount()` converts cold to hot (shared). +**Converting**: `Publish()` + `Connect()` or `Publish().RefCount()` converts cold to hot (shared, see [§5.10](./rx-design-guide.instructions.md#510-use-the-publish-operator-to-share-side-effects)). ```csharp var shared = coldSource.Publish().RefCount(); // auto-connect on first sub, auto-disconnect on last unsub @@ -48,52 +50,14 @@ var shared = coldSource.Publish().RefCount(); // auto-connect on first sub, auto ## The Observable Contract -### 1. Serialized Notifications (THE critical rule) - -`OnNext`, `OnError`, and `OnCompleted` calls MUST be serialized — they must never execute concurrently. This is the most commonly violated rule and causes the most insidious bugs. - -```csharp -// WRONG: two sources can call OnNext concurrently -source1.Subscribe(x => observer.OnNext(Process(x))); // thread A -source2.Subscribe(x => observer.OnNext(Process(x))); // thread B — RACE! - -// RIGHT: use Synchronize to serialize -source1.Synchronize(gate).Subscribe(observer); -source2.Synchronize(gate).Subscribe(observer); - -// RIGHT: use Merge (serializes internally) -source1.Merge(source2).Subscribe(observer); - -// RIGHT: use Subject (serializes OnNext calls via Synchronize) -var subject = new Subject(); -source1.Subscribe(subject); // Subject.OnNext is NOT thread-safe by default! -// Use Subject with Synchronize if multiple threads call OnNext -``` - -**Why it matters**: operators maintain mutable internal state (caches, dictionaries, counters). Concurrent `OnNext` calls corrupt this state silently — no exception, just wrong data. - -### 2. Terminal Notifications +The full contract lives in [§4](./rx-design-guide.instructions.md#4-the-rx-contract) of the design guide. Highlights: -``` -Grammar: OnNext* (OnError | OnCompleted)? -``` - -- Zero or more `OnNext`, followed by at most one terminal notification -- `OnError` and `OnCompleted` are **mutually exclusive** — emit one or neither, never both -- After a terminal notification, **no further notifications** of any kind -- Operators receiving a terminal notification should release resources - -### 3. Subscription Lifecycle - -- `Subscribe` returns `IDisposable` — disposing it **unsubscribes** -- After disposal, no further notifications should be delivered -- Disposal must be **idempotent** (safe to call multiple times) and **thread-safe** -- Operators should stop producing when their subscription is disposed +- **[§4.1](./rx-design-guide.instructions.md#41-assume-the-rx-grammar)** Grammar: `OnNext* (OnCompleted | OnError)?`. Mutually-exclusive terminals. No notifications after a terminal. +- **[§4.2](./rx-design-guide.instructions.md#42-assume-observer-instances-are-called-in-a-serialized-fashion)** Serialized notifications. The single most-violated rule in practice. Violation produces silent state corruption. +- **[§4.3](./rx-design-guide.instructions.md#43-assume-resources-are-cleaned-up-after-an-onerror-or-oncompleted-message)** Resource cleanup on terminal. +- **[§4.4](./rx-design-guide.instructions.md#44-assume-a-best-effort-to-stop-all-outstanding-work-on-unsubscribe)** Unsubscribe semantics: best-effort stop, in-flight results suppressed. -### 4. Error Handling - -- Exceptions thrown inside `OnNext` handlers propagate synchronously to the producing operator -- Use `SubscribeSafe` instead of `Subscribe` to route subscriber exceptions to `OnError`: +DynamicData-specific: subscribers that throw inside `OnNext` propagate exceptions to the producing operator. Use `SubscribeSafe` to route subscriber exceptions to `OnError`: ```csharp // Subscribe: exception in handler crashes the source @@ -107,7 +71,7 @@ source.SubscribeSafe(Observer.Create( ## Schedulers -Schedulers control **when** and **where** work executes. They are Rx's abstraction over threading. +Schedulers control **when** and **where** work executes; they are Rx's abstraction over threading. The design guide's [§5.4](./rx-design-guide.instructions.md#54-consider-passing-a-specific-scheduler-to-concurrency-introducing-operators), [§5.5](./rx-design-guide.instructions.md#55-call-the-observeon-operator-as-late-and-in-as-few-places-as-possible), [§6.9–6.12](./rx-design-guide.instructions.md#69-parameterize-concurrency-by-providing-a-scheduler-argument) cover the rules. This section is the modern Rx.NET reference for which scheduler to actually use. ### Common Schedulers @@ -142,7 +106,7 @@ source.SubscribeOn(TaskPoolScheduler.Default) // subscribe on background thread ### Scheduler Injection for Testability -**Always inject schedulers** instead of using defaults. This enables deterministic testing: +DynamicData convention (not in the PDF): **always inject schedulers** instead of using defaults. This enables deterministic testing via `TestScheduler`: ```csharp // WRONG: hardcoded scheduler — untestable time-dependent behavior @@ -163,7 +127,7 @@ results.Should().HaveCount(1); ## Disposable Helpers -Rx provides several `IDisposable` implementations for managing subscription lifecycles: +The design guide's [§6.13](./rx-design-guide.instructions.md#613-hand-out-all-disposable-instances-created-inside-the-operator-to-consumers) lists the disposable family and the requirement to hand all internal disposables back to consumers. This section is the practical reference with code samples for each. ### Disposable.Create @@ -180,7 +144,7 @@ var cleanup = Disposable.Create(() => ### Disposable.Empty -A no-op disposable. Useful as a default or placeholder. +A no-op disposable. Useful as a default or placeholder (required by [§6.5](./rx-design-guide.instructions.md#65-subscribe-implementations-should-not-throw) when routing an error from `Subscribe`). ```csharp public IDisposable Subscribe(IObservable source) => @@ -217,7 +181,7 @@ Observable.Create(observer => ### SerialDisposable -Holds a single disposable that can be **replaced**. Disposing the previous value when a new one is set. Useful for "switch" patterns. +Holds a single disposable that can be **replaced**. Disposing the previous value when a new one is set. Useful for "switch" patterns. (PDF calls this `MutableDisposable`.) ```csharp var serial = new SerialDisposable(); @@ -233,7 +197,7 @@ serial.Dispose(); ### SingleAssignmentDisposable -Like SerialDisposable but can only be assigned **once**. Throws on second assignment. Useful when a subscription is created asynchronously but disposal might happen before it's ready. +Like SerialDisposable but can only be assigned **once**. Throws on second assignment. Useful when a subscription is created asynchronously but disposal might happen before it's ready. (Modern addition, not in the PDF.) ```csharp var holder = new SingleAssignmentDisposable(); @@ -251,7 +215,7 @@ holder.Dispose(); ### RefCountDisposable -Tracks multiple "dependent" disposables. The underlying resource is only disposed when **all** dependents (plus the primary) are disposed. +Tracks multiple "dependent" disposables. The underlying resource is only disposed when **all** dependents (plus the primary) are disposed. (Modern addition, not in the PDF.) ```csharp var primary = new RefCountDisposable(expensiveResource); @@ -281,6 +245,8 @@ cd.Dispose(); // triggers cancellation ## Standard Rx Operators Reference +Not in the PDF. A quick catalog of modern Rx.NET operators by category. For each operator, the design guide rules in [§5](./rx-design-guide.instructions.md#5-using-rx) and [§6](./rx-design-guide.instructions.md#6-operator-implementations) tell you how to use it correctly. + ### Creation | Operator | Description | @@ -389,38 +355,40 @@ cd.Dispose(); // triggers cancellation | `SubscribeOn(scheduler)` | Subscribe (and produce) on specified scheduler | | `Delay(timeSpan)` | Delay each notification by a time span | | `Timeout(timeSpan)` | Error if no notification within timeout | -| `Synchronize()` | Serialize notifications with internal gate | -| `Synchronize(gate)` | Serialize notifications with external gate object | +| `Synchronize()` | Serialize notifications with internal gate (per [§5.8](./rx-design-guide.instructions.md#58-use-the-synchronize-operator-only-to-fix-custom-iobservable-implementations) — only for non-conforming sources) | +| `Synchronize(gate)` | Serialize notifications with external gate object (the multi-source pattern from [§6.7](./rx-design-guide.instructions.md#67-serialize-calls-to-iobserver-methods-within-observable-sequence-implementations)) | ### Utility | Operator | Description | |----------|-------------| -| `Do(action)` | Perform side effect for each notification | +| `Do(action)` | Perform side effect for each notification (see [§5.7](./rx-design-guide.instructions.md#57-make-side-effects-explicit-using-the-do-operator)) | | `Publish().RefCount()` | Share a subscription among multiple subscribers | | `Replay(bufferSize).RefCount()` | Share with replay | | `AsObservable()` | Hide the implementation type (e.g., Subject → IObservable) | | `Subscribe(observer)` | Subscribe with an IObserver | -| `Subscribe(onNext, onError, onCompleted)` | Subscribe with callbacks | +| `Subscribe(onNext, onError, onCompleted)` | Subscribe with callbacks (see [§5.2](./rx-design-guide.instructions.md#52-consider-passing-multiple-arguments-to-subscribe)) | | `SubscribeSafe(observer)` | Subscribe with exception routing to OnError | | `ForEachAsync(action)` | Async iteration (returns Task) | -| `Wait()` | Block until complete (avoid on UI thread) | +| `Wait()` | Block until complete (avoid on UI thread; see [§6.14](./rx-design-guide.instructions.md#614-operators-should-not-block)) | | `ToTask()` | Convert to Task (last value) | ## Writing Custom Operators +[§6.1](./rx-design-guide.instructions.md#61-implement-new-operators-by-composing-existing-operators) requires composition over `Observable.Create`. [§6.2](./rx-design-guide.instructions.md#62-implement-custom-operators-using-observablecreate) governs how to use `Observable.Create` when composition isn't enough. This section is the DynamicData-specific elaboration of those rules: the Defer pattern for per-subscription state without `Observable.Create`, the canonical `Observable.Create` skeleton, and a multi-source pattern. + ### Composition First — Observable.Create is a Last Resort -**The Rx contracts are axioms, not guidelines.** `Merge` subscribes sequentially. `Defer` evaluates at subscription time. `Do` fires synchronously during delivery. `Concat` subscribes to the second source only after the first completes. These guarantees are unconditional — they hold in every case, on every scheduler, under every threading model. If they didn't, nothing in Rx would work. +**The Rx contracts are axioms, not guidelines.** `Merge` subscribes sequentially. `Defer` evaluates at subscription time. `Do` fires synchronously during delivery. `Concat` subscribes to the second source only after the first completes. These guarantees are unconditional: they hold in every case, on every scheduler, under every threading model. If they didn't, nothing in Rx would work. **Trust the contracts completely.** When you compose operators, you can reason about ordering, state, and lifecycle *because* the contracts are absolute. The moment you doubt them and add "safety" wrappers, you've abandoned the very thing that makes Rx code correct by construction. -**Before reaching for `Observable.Create`, ask: can this be expressed as a composition of existing operators?** Rx operators already handle subscription lifecycle, error propagation, disposal, and serialization. Manual observer forwarding inside `Observable.Create` reimplements all of that — and introduces bugs that the operators would have prevented. +**Before reaching for `Observable.Create`, ask: can this be expressed as a composition of existing operators?** Rx operators already handle subscription lifecycle, error propagation, disposal, and serialization. Manual observer forwarding inside `Observable.Create` reimplements all of that, and introduces bugs that the operators would have prevented. -**The smell:** If you're writing `observer.OnNext(x)` / `observer.OnError(ex)` / `observer.OnCompleted()` inside `Observable.Create`, you're manually reimplementing what `Subscribe` already does. Stop and look for the composition. +**The smell:** if you're writing `observer.OnNext(x)` / `observer.OnError(ex)` / `observer.OnCompleted()` inside `Observable.Create`, you're manually reimplementing what `Subscribe` already does. Stop and look for the composition. ```csharp -// ❌ WRONG: imperative Observable.Create with manual forwarding +// WRONG: imperative Observable.Create with manual forwarding // This reimplements Subscribe, adds boilerplate, and is harder to reason about. return Observable.Create>(observer => { @@ -441,7 +409,7 @@ return Observable.Create>(observer => return sub; }); -// ✅ RIGHT: declarative composition using existing operators +// RIGHT: declarative composition using existing operators // Each operator does one thing. The intent is immediately clear. return Observable.Defer(() => { @@ -498,7 +466,7 @@ Each subscription gets its own `index` / `hasEmitted` — cold observable semant ### The Observable.Create Pattern -When you genuinely need `Observable.Create`, follow this structure: +When you genuinely need `Observable.Create`, follow this structure. Notice the [§6.4](./rx-design-guide.instructions.md#64-protect-calls-to-user-code-from-within-an-operator) try/catch around the user selector and the [§6.16](./rx-design-guide.instructions.md#616-argument-validation-should-occur-outside-observablecreate) argument validation that would be required at the public extension method boundary. ```csharp public static IObservable MyOperator( @@ -528,7 +496,7 @@ public static IObservable MyOperator( ### Multi-Source Operator Pattern -When combining multiple sources, serialize their notifications: +When combining multiple sources, serialize their notifications through a shared gate (per [§6.7](./rx-design-guide.instructions.md#67-serialize-calls-to-iobserver-methods-within-observable-sequence-implementations)): ```csharp public static IObservable MyMerge( @@ -547,23 +515,27 @@ public static IObservable MyMerge( } ``` -**Note**: `Synchronize(gate)` holds the lock during downstream `OnNext` delivery. This ensures serialization but means the lock is held for the duration of all downstream processing. Keep downstream chains lightweight when using shared gates. +**Note**: `Synchronize(gate)` holds the lock during downstream `OnNext` delivery. This ensures serialization but means the lock is held for the duration of all downstream processing. Keep downstream chains lightweight when using shared gates. DynamicData's `SynchronizeSafe` + `SharedDeliveryQueue` is the in-library alternative that releases the lock before downstream delivery (used to prevent cross-cache deadlocks). ### Operator Checklist -When writing or reviewing an Rx operator: - -- [ ] **Serialized delivery**: can `OnNext` be called concurrently? If multiple sources, are they serialized? -- [ ] **Terminal semantics**: does `OnError`/`OnCompleted` propagate correctly? No notifications after terminal? -- [ ] **Disposal**: does disposing the subscription clean up all resources? Is it idempotent? -- [ ] **Error handling**: does `SubscribeSafe` catch subscriber exceptions? Are errors propagated, not swallowed? -- [ ] **Back-pressure**: does the operator buffer unboundedly? Could it cause memory issues? -- [ ] **Scheduler**: are time-dependent operations using an injectable scheduler? -- [ ] **Cold/Hot**: is the observable cold (deferred via `Observable.Create`)? If hot, is sharing handled correctly? +When writing or reviewing an Rx operator, walk this checklist alongside the rule audits in the design guide: + +- [ ] **Serialized delivery** ([§4.2](./rx-design-guide.instructions.md#42-assume-observer-instances-are-called-in-a-serialized-fashion) / [§6.7](./rx-design-guide.instructions.md#67-serialize-calls-to-iobserver-methods-within-observable-sequence-implementations)): can `OnNext` be called concurrently? If multiple sources, are they serialized through the same gate? +- [ ] **Terminal semantics** ([§4.1](./rx-design-guide.instructions.md#41-assume-the-rx-grammar) / [§6.6](./rx-design-guide.instructions.md#66-onerror-messages-should-have-abort-semantics)): does `OnError`/`OnCompleted` propagate correctly? No notifications after terminal? No buffer flush on error? +- [ ] **Disposal** ([§4.4](./rx-design-guide.instructions.md#44-assume-a-best-effort-to-stop-all-outstanding-work-on-unsubscribe) / [§6.13](./rx-design-guide.instructions.md#613-hand-out-all-disposable-instances-created-inside-the-operator-to-consumers) / [§6.17](./rx-design-guide.instructions.md#617-unsubscription-should-be-idempotent) / [§6.18](./rx-design-guide.instructions.md#618-unsubscription-should-not-throw)): does disposing the subscription clean up all resources? Are all internal disposables exposed to the subscriber? Idempotent and non-throwing? +- [ ] **User code protection** ([§6.4](./rx-design-guide.instructions.md#64-protect-calls-to-user-code-from-within-an-operator)): are user-provided selectors / predicates / comparers wrapped in try/catch with errors routed to `OnError`? +- [ ] **Subscribe doesn't throw** ([§6.5](./rx-design-guide.instructions.md#65-subscribe-implementations-should-not-throw)): error conditions detected in subscribe go through `observer.OnError(...)` + `return Disposable.Empty;`? +- [ ] **Argument validation** ([§6.16](./rx-design-guide.instructions.md#616-argument-validation-should-occur-outside-observablecreate)): null checks happen before `Observable.Create`, not inside the subscribe lambda? +- [ ] **Back-pressure / buffers** ([§5.6](./rx-design-guide.instructions.md#56-consider-limiting-buffers)): does the operator buffer unboundedly? Could it cause memory issues? +- [ ] **Scheduler** ([§5.4](./rx-design-guide.instructions.md#54-consider-passing-a-specific-scheduler-to-concurrency-introducing-operators) / [§6.9–6.12](./rx-design-guide.instructions.md#69-parameterize-concurrency-by-providing-a-scheduler-argument)): time-dependent operations take a scheduler argument? Default uses `Scheduler.Immediate` where possible? +- [ ] **Cold/Hot**: is the observable cold (deferred via `Observable.Create` / `Observable.Defer`)? If hot, is sharing handled correctly via `Publish().RefCount()` (per [§5.10](./rx-design-guide.instructions.md#510-use-the-publish-operator-to-share-side-effects))? - [ ] **Thread safety**: is mutable state protected? Are there race conditions between subscribe/dispose/OnNext? ## Common Pitfalls +Practical pitfalls that don't have direct PDF analogues but appear repeatedly in DynamicData code review. + ### 1. Subscribing Multiple Times to a Cold Observable ```csharp @@ -572,7 +544,7 @@ var data = Observable.FromAsync(() => httpClient.GetAsync(url)); data.Subscribe(handler1); // call 1 data.Subscribe(handler2); // call 2 — probably not intended -// RIGHT: share the result +// RIGHT: share the result (per §5.10) var shared = data.Publish().RefCount(); shared.Subscribe(handler1); // shares shared.Subscribe(handler2); // same result @@ -592,7 +564,7 @@ _cleanup.Add(source.Subscribe(x => UpdateUI(x))); ### 3. Blocking on Rx (sync-over-async) ```csharp -// WRONG: blocks the thread, can hang on UI thread +// WRONG: blocks the thread, can hang on UI thread (see §6.14) var result = source.FirstAsync().Wait(); // RIGHT: use async/await @@ -613,7 +585,7 @@ public IObservable Values => _values.AsObservable(); ### 5. Not Handling OnError ```csharp -// WRONG: unhandled OnError crashes the app (routes to DefaultExceptionHandler) +// WRONG: unhandled OnError crashes the app (routes to DefaultExceptionHandler — see §5.2) source.Subscribe(x => Process(x)); // RIGHT: always handle errors From a4df581ee2651f3e41e833ffd888d60f79249a7b Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Sun, 14 Jun 2026 12:40:08 -0700 Subject: [PATCH 19/24] Tighten rx-design-guide.instructions.md from 798 to 301 lines GHCP instructions files load into agent context on every C# edit, so size matters. Tightened the file by: - Dropping section 3 (When to use Rx). Not actionable for contributors already in DynamicData; the decision to use Rx is settled. - Modernizing samples and API names in place; removing the Modernization-notes table and the per-sample "Modernization note" callouts. A single sentence at the top now states that everything is current Rx.NET. - Dropping "When to ignore: none known" entries (8 of them); the absence already conveys the same information. - Collapsing oversized samples (notably the section 6.7 ZipEx sample, which mostly demonstrated locking mechanics already conveyed by the shorter Synchronize(gate) form). - Removing the Common-violation-patterns table and the How-to-use section; the rules themselves carry the same information. All 34 rules from sections 4, 5, and 6 preserved. Every substantive "When to ignore" caveat preserved (only the empty "none known" ones removed). Rule IDs unchanged so existing citations still resolve. --- .../rx-design-guide.instructions.md | 717 +++--------------- 1 file changed, 110 insertions(+), 607 deletions(-) diff --git a/.github/instructions/rx-design-guide.instructions.md b/.github/instructions/rx-design-guide.instructions.md index c6787aa2..a43e6d8e 100644 --- a/.github/instructions/rx-design-guide.instructions.md +++ b/.github/instructions/rx-design-guide.instructions.md @@ -3,321 +3,117 @@ applyTo: "**/*.cs" --- # Rx Design Guide -A complete distillation of the **[Microsoft Rx Design Guidelines v1.0 (October 2010)](https://go.microsoft.com/fwlink/?LinkID=205219)** (canonical fwlink; resolves to `download.microsoft.com/.../Rx Design Guidelines.pdf`). This is the authoritative reference for the Rx contract and the rules for using Rx and implementing Rx operators in DynamicData. +Complete distillation of the **[Microsoft Rx Design Guidelines v1.0 (October 2010)](https://go.microsoft.com/fwlink/?LinkID=205219)**. This is the authoritative reference for the Rx contract and the rules for using Rx and implementing Rx operators in DynamicData. -**This document is meant to be self-contained.** Future contributors and agents should not have to re-consume the PDF: the entire spec (every rule, every "When to ignore" caveat, the clarifying samples) is preserved here. Samples have been modernized to current Rx.NET API names; the PDF's original 2010 names appear in the Modernization notes table below. +**Code samples and API names have been updated to current Rx.NET (modernized from the 2010 spec).** Treat the conventions here as current. **Every operator added or modified must self-audit against these rules. Every bug fix must explicitly state which rules were verified.** -Rules use the **original Microsoft document's `§X.Y` numbering** throughout so citations are unambiguous and traceable to the source spec. Cite rule IDs in code review comments, PR descriptions, and commit messages (e.g., "Fixes §6.6 violation in `BatchIf`"). Always use the literal `§` character — never ASCII substitutes (`S`, `SS`, `sec`, etc.). - ---- - -## Document structure - -The file follows the original Microsoft document's section order: -- **§2** introduces what the guidelines are and how to read them. -- **§3** is "When to use Rx" — applicability guidelines for choosing Rx in the first place. -- **§4** is the Observable Contract from the consumer's perspective (what callers can rely on). -- **§5** is rules for code that uses Rx (consumer-side correctness). -- **§6** is rules for operator authors (what your operator must guarantee). - -§1 (the PDF's table of contents) is omitted here as it carries no rules. - ---- - -## Modernization notes - -This document is derived from a 2010 specification but uses **current Rx.NET API names throughout**. Where the original PDF references obsolete names, this document uses the modern equivalents. Do NOT use the obsolete names in new code or fixes: - -| PDF / 2010 name (obsolete) | Modern equivalent | -|---|---| -| `Observable.CreateWithDisposable(Func, IDisposable>)` | `Observable.Create(Func, IDisposable>)` (same signature, name unified) | -| `Observable.Create` (2010 variant taking `Func, Action>`) | Still exists, but prefer the `IDisposable`-returning overload for resource lifetime correctness | -| `MutableDisposable` | `SerialDisposable` | -| `Scheduler.Dispatcher` | `DispatcherScheduler.Current` (WPF/WinForms) or platform-specific | -| `Scheduler.ThreadPool` | `Scheduler.Default` or `TaskPoolScheduler.Default` | -| `Scheduler.NewThread` | `NewThreadScheduler.Default` | -| `Observable.FromEvent(d => d.Invoke, add, remove)` (3-arg form) | `Observable.FromEventPattern(add, remove)` (typed event pattern) | -| `Observable.Start(Func)` for async work | `Observable.FromAsync(Func>)` for proper async/await integration | -| `IScheduler.Schedule(Action)` (interface method) | `IScheduler.Schedule(this IScheduler, Action)` extension method (same usage, now an extension) | - -Modern additions worth knowing (not in the 2010 PDF): -- **`SingleAssignmentDisposable`**: assign once, throws on second assignment. Useful when a disposable is wired up asynchronously after subscription returns. -- **`RefCountDisposable`**: reference-counted disposable; underlying resource disposed only when all dependents released. -- **`IScheduler.Schedule(TState, Func)`** generic overload: avoids closure allocation in hot paths. Prefer this in performance-sensitive operators. -- **`ValueTask` / `IAsyncEnumerable` interop**: `ToObservable()`, `ToAsyncEnumerable()` bridges. -- **`System.Threading.Lock`** (NET9+): modern lock type used by DynamicData's `Rxx.Synchronize` on NET9+. - -**Keep this table current.** When you encounter a 2010 API name in the PDF that doesn't yet appear here (because Rx.NET has since renamed, removed, or supplemented it), add a row. If you find an obsolete API name used anywhere in this codebase, that is itself a finding to surface. - ---- - -## §2: Introduction - -These guidelines aid in developing applications and libraries that use Rx. They have evolved over time during the development of the Rx library, and continue to evolve as Rx evolves. - -The guidelines are **not absolute truths.** They are patterns the Rx team found helpful, not rules to be followed blindly. There are situations where certain guidelines do not apply. The PDF lists known exceptions for each rule in its "When to ignore this guideline" sections, which are preserved here verbatim. - -The guidelines are listed in **no particular order.** There is neither total nor partial ordering between them. The §X.Y numbering is a stable ID for citation purposes only, not a priority ranking. - ---- - -## §3: When to use Rx - -### §3.1. Use Rx for orchestrating asynchronous and event-based computations - -Code that deals with more than one event or asynchronous computation gets complicated quickly: it needs a state machine for ordering, and explicit handling of success and failure termination for every separate computation. The result is code that doesn't follow normal control flow, is hard to understand, and is hard to maintain. - -Rx makes these computations first-class citizens. This provides a model for readable, composable APIs over asynchronous computations. - -**Sample (modernized to current Rx.NET API)** — autocomplete-style "dictionary suggest" that throttles user input and cancels stale lookups: -```csharp -var keyUp = Observable.FromEventPattern(textBox, nameof(textBox.KeyUp)); - -var dictionarySuggest = keyUp - .Select(_ => textBox.Text) - .Where(text => !string.IsNullOrEmpty(text)) - .DistinctUntilChanged() - .Throttle(TimeSpan.FromMilliseconds(250), uiScheduler) - .SelectMany(text => AsyncLookupInDictionary(text).TakeUntil(keyUp)); - -dictionarySuggest.Subscribe( - results => listView.Items.AddRange(results.Select(r => new ListViewItem(r)).ToArray()), - error => LogError(error)); -``` -`Throttle` collapses bursts of keystrokes, `SelectMany` flattens the per-text async lookups, `TakeUntil(keyUp)` cancels in-flight lookups when the user types again. In imperatively-written code each of these would be a separate timer or callback with explicit exception bookkeeping. - -**When to ignore this guideline:** if the application has very few asynchronous/event-based operations or few places where they need to be composed, the cost of depending on Rx (redistribution and the learning curve) might outweigh the cost of coding the operations manually. - -### §3.2. Use Rx to deal with asynchronous sequences of data - -Several other .NET asynchronous libraries exist, but most work best for operations that return a single message. They usually do not support operations that produce **multiple** messages over the lifetime of the operation. - -Rx follows the grammar `OnNext* (OnCompleted | OnError)?` (see §4.1). This makes Rx suitable both for operations that produce a single message and operations that produce multiple messages. - -**Sample (modernized)** — pipelined encryption of a 4 GB file using 64K blocks (no full-file buffering): -```csharp -inFile.AsyncRead(blockSize: 2 << 15) - .Select(Encrypt) - .WriteToStream(outFile) - .Subscribe( - _ => Console.WriteLine("Successfully encrypted the file."), - error => Console.WriteLine($"An error occurred while encrypting: {error.Message}")); -``` -Each 64K block flows through the pipeline as a separate `OnNext`. Encryption runs per block; writes happen as soon as a block is encrypted. Memory usage stays bounded, throughput stays high. - -**When to ignore this guideline:** if the application has very few operations with multiple messages, the cost of depending on Rx might outweigh the cost of coding them manually. +Rules use the original document's `§X.Y` numbering. Cite rule IDs in code reviews, PR descriptions, and commit messages (e.g., "Fixes §6.6 violation in `BatchIf`"). Always use the literal `§` character — never ASCII substitutes (`S`, `SS`, `sec`, etc.). --- ## §4: The Rx contract -`IObservable` and `IObserver` only specify their methods' arguments and return types. The Rx library makes additional assumptions about these interfaces that are not expressible in the .NET type system. These assumptions form a contract that **all producers and consumers** of Rx types must follow. The contract makes it possible to reason about and prove the correctness of operators and user code. +`IObservable` and `IObserver` only specify their methods' arguments and return types. The Rx library makes additional assumptions about these interfaces that are not expressible in the .NET type system. These assumptions form a contract that **all producers and consumers** of Rx types must follow. ### §4.1. Assume the Rx Grammar -Messages sent to instances of `IObserver` follow the grammar: -``` -OnNext* (OnCompleted | OnError)? -``` +Messages follow `OnNext* (OnCompleted | OnError)?`: - Zero or more `OnNext`, optionally followed by exactly one terminal notification. -- `OnError` and `OnCompleted` are **mutually exclusive**: emit one or the other, never both. -- After a terminal notification, **no further notifications of any kind**, not even another terminal. +- `OnError` and `OnCompleted` are **mutually exclusive**. +- After a terminal, **no further notifications of any kind**, not even another terminal. -The single terminal message ensures that consumers can deterministically establish when it is safe to perform cleanup. A single failure further ensures that abort semantics can be maintained for operators that work on multiple observable sequences (see §6.6). +The single terminal lets consumers deterministically clean up; the single-failure rule supports abort semantics in multi-source operators (§6.6). -**When to ignore this guideline:** only when working with a non-conforming `IObservable` implementation. Such a source can be made conforming by calling `Synchronize()` on it (per §5.8). +**When to ignore:** only for non-conforming `IObservable` sources. Restore conformance with `Synchronize()` (§5.8). ### §4.2. Assume observer instances are called in a serialized fashion -Rx uses a push model and .NET supports multithreading, so without serialization different messages could arrive on different execution contexts at the same time. Forcing every consumer to handle this would require pervasive housekeeping, hurt maintainability, and harm performance. +`OnNext`, `OnError`, and `OnCompleted` calls to a single observer **MUST never execute concurrently.** Only the operators that produce multi-source sequences are required to serialize (§6.7); consumers can safely assume serialization. -Only the operators that produce multi-source observable sequences are required to perform serialization (see §6.7). **Consumers can safely assume that messages on a single observer arrive in a serialized fashion.** +This is the most-violated rule in practice. Violations produce silent state corruption, not exceptions. -```csharp -var count = 0; -xs.Subscribe(v => -{ - count++; - Console.WriteLine($"OnNext has been called {count} times."); -}); -``` -No locking or interlocking is required to read or write `count`: only one call to `OnNext` can be in-flight at a time. - -**When to ignore this guideline:** if you have to consume a custom `IObservable` implementation that doesn't follow the contract for serialization, use `Synchronize()` to restore the guarantee. +**When to ignore:** for a custom `IObservable` that doesn't serialize, wrap with `Synchronize()` to restore the guarantee. ### §4.3. Assume resources are cleaned up after an OnError or OnCompleted message -§4.1 states that no more messages should arrive after a terminal. This makes it possible to clean up any resources used by the subscription the moment the terminal arrives. Cleaning up immediately makes side effects predictable and lets the runtime reclaim resources promptly. - -```csharp -Observable.Using( - () => new FileStream(path, FileMode.Create), - fs => Observable.Range(0, 10000) - .Select(v => Encoding.ASCII.GetBytes(v.ToString())) - .WriteToStream(fs)) - .Subscribe(); -``` -`Using` creates a resource that will be disposed upon unsubscription. The cleanup guarantee ensures unsubscription is called automatically once a terminal arrives. - -**When to ignore this guideline:** none known. +After a terminal, the operator MUST immediately release its resources. `Observable.Using` / `Finally` fire deterministically. ### §4.4. Assume a best effort to stop all outstanding work on Unsubscribe -When `Dispose` is called on a subscription, the source makes a best-effort attempt to stop all outstanding work. -- Queued work that has not yet started is cancelled. -- Work already in progress **may** still complete (it is not always safe to abort), but its results **MUST NOT** be signaled to the unsubscribed observer. -- Messages may arrive during the `Dispose` call itself (Dispose can race with `OnNext`). -- After `Dispose` returns control to the caller: no more messages arrive. +When `Dispose` is called: +- Queued work that has not started is cancelled. +- Work already in progress may complete, but its results **MUST NOT** be signaled to the unsubscribed observer. +- Messages may arrive **during** the `Dispose` call itself (Dispose races with `OnNext`). +- After `Dispose` returns: no more messages arrive. - The unsubscription process may continue asynchronously on a different context after `Dispose` returns. -```csharp -// Sample 1: queued work is cancelled — observer never fires -Observable.Timer(TimeSpan.FromSeconds(2)).Subscribe(...).Dispose(); - -// Sample 2: in-progress work runs to completion, but its result is dropped -Observable.Start(() => { Thread.Sleep(TimeSpan.FromSeconds(2)); return 5; }) - .Subscribe(...).Dispose(); -``` - --- ## §5: Using Rx -These rules govern code that consumes Rx. They apply recursively inside operator implementations too, per §6.20. +Rules for code that consumes Rx. Apply recursively inside operator implementations (§6.20). ### §5.1. Consider drawing a Marble-diagram -Draw a marble diagram of the observable sequence you want to create. By drawing the diagram, you get a clearer picture of which operators to use. - -A marble diagram shows events occurring over time, with both input and output sequences. Sketching one often makes the answer obvious: a "delay then call" pattern maps to `Throttle`, a "create a new sequence per input" pattern maps to `SelectMany`, and so on. +Sketching the inputs and outputs over time often makes the operator choice obvious (delay-then-call → `Throttle`; new sequence per input → `SelectMany`). -**When to ignore this guideline:** when you are comfortable enough with the sequence you want to write. Even Rx team members still reach for a whiteboard occasionally. +**When to ignore:** when you're comfortable enough without one. ### §5.2. Consider passing multiple arguments to Subscribe -Rx provides Subscribe overloads that take delegates instead of an `IObserver`, because C# and VB do not support anonymous inner classes. These overloads use defaults for any method you omit. - -The single-argument `Subscribe(onNext)` overload **rethrows OnError on the thread the message arrives on**, crashing the application. The no-argument `OnCompleted` default is a no-op. In most situations, dealing with the exception (either recovering or aborting gracefully) and knowing the sequence completed successfully are both important, so it is best to provide all three arguments. - -**When to ignore this guideline:** -- The observable sequence is guaranteed not to complete (e.g. a UI event like `KeyUp`). -- The observable sequence is guaranteed not to error (e.g. an event, a materialized sequence, etc.). -- The default behavior is the desired behavior. +`Subscribe(onNext)` **rethrows OnError on the source thread, crashing the app.** The default `OnCompleted` is a no-op. Provide all three handlers unless: +- The sequence is guaranteed not to complete (e.g. a UI event) +- The sequence is guaranteed not to error +- The default behavior is genuinely desired ### §5.3. Consider using LINQ query expression syntax -Rx implements the query expression pattern, so LINQ query syntax can be used over observable sequences: -```csharp -// Method syntax -var r = xs.SelectMany(x => ys, (x, y) => x + y); - -// Equivalent query syntax -var r1 = from x in xs - from y in ys - select x + y; -``` - -**When to ignore this guideline:** if your query uses many operators that don't have query-syntax equivalents, the mixed style may negate the readability benefit. +Rx implements the query-expression pattern; `SelectMany`-based pipelines often read better as LINQ. Skip when many of your operators have no query-syntax equivalent. ### §5.4. Consider passing a specific scheduler to concurrency-introducing operators -Rather than using `ObserveOn` to change execution context, create concurrency on the right scheduler from the start. Operators that introduce concurrency provide a scheduler argument overload. Passing the right scheduler upfront eliminates downstream `ObserveOn` hops. - +Better to introduce concurrency on the right scheduler from the start than to fix it up with `ObserveOn`: ```csharp -var keyUp = Observable.FromEventPattern(textBox, nameof(textBox.KeyUp)); -var throttled = keyUp.Throttle(TimeSpan.FromSeconds(1), DispatcherScheduler.Current); +keyUp.Throttle(TimeSpan.FromSeconds(1), DispatcherScheduler.Current); ``` -Without the explicit scheduler, the default `Throttle` overload would deliver `OnNext` from a ThreadPool timer. Passing the dispatcher scheduler keeps all messages on the UI thread. +Without the scheduler, the default `Throttle` overload would deliver on the ThreadPool. -**When to ignore this guideline:** when combining several events that originate on different execution contexts, use §5.5 to put all messages on a specific context as late as possible. +**When to ignore:** when combining many sources from different contexts, use §5.5 (one `ObserveOn` at the end). ### §5.5. Call the ObserveOn operator as late and in as few places as possible -`ObserveOn` schedules an action per message. This changes timing and adds load. Placing it later in the query (after filtering) reduces both concerns. - -```csharp -var result = - (from x in xs.Throttle(TimeSpan.FromSeconds(1)) - from y in ys.TakeUntil(zs).Sample(TimeSpan.FromMilliseconds(250)) - select x + y) - .Merge(ws) - .Where(x => x.Length < 10) - .ObserveOn(DispatcherScheduler.Current); -``` -Placing `ObserveOn` earlier would do scheduling work for messages that the `Where` filter throws away. - -**When to ignore this guideline:** if your use of the observable sequence is not bound to a specific execution context, do not use `ObserveOn` at all. +`ObserveOn` schedules per-message work. Placing it after filters avoids scheduling work for messages that get filtered out. Skip entirely when no specific context is required. ### §5.6. Consider limiting buffers -Rx comes with operators that buffer over observable sequences (e.g. `Replay`). The buffer size scales with the input sequence. If unbounded, the buffer causes memory pressure. - -Most buffering operators provide policies to limit the buffer by time or size: -```csharp -var result = xs.Replay(bufferSize: 10000, window: TimeSpan.FromHours(1)); -``` - -**When to ignore this guideline:** when the number of messages populating the buffer is small, or when the buffer is already bounded by its surrounding context. +`Replay`, `Buffer`, etc. without size/time limits cause unbounded memory growth: `Replay(10000, TimeSpan.FromHours(1))`. ### §5.7. Make side-effects explicit using the Do operator -Rx operators take delegates as arguments, and it is possible to put side-effecting code (writes to disk, mutation of global state, logging) into those delegates. Because Rx composition runs each operator for each subscription (except sharing operators like `Publish`), every side-effect occurs for every subscription. - -If you want side effects per subscription, **make that explicit by putting the side-effecting code in a `Do` operator** so it's visible in the pipeline. - +Side effects buried in selector/predicate lambdas are unauditable, and they run **per subscription** (unless shared via §5.10). Hoist them into `Do(...)`: ```csharp -var result = xs.Where(x => x.Failed).Do(x => Log(x)).Subscribe(...); +xs.Where(x => x.Failed).Do(x => Log(x)).Subscribe(...); ``` -**When to ignore this guideline:** when the side effect needs data from an operator that is not available to a separate `Do`. +**When to ignore:** when the side effect needs data unavailable to `Do`. ### §5.8. Use the Synchronize operator only to "fix" custom IObservable implementations -Observable sequences created by Rx operators already follow the contract for grammar (§4.1) and serialization (§4.2). Adding `Synchronize` to one of them is redundant and counterproductive. **Only use `Synchronize` on observable sequences created by other sources that do not follow the contract.** - -```csharp -var result = from x in customNonConformingSource.Synchronize() - from y in ys - where x > y - select y; -``` - -**When to ignore this guideline:** none known. +Rx and DynamicData operators already satisfy §4.1 / §4.2. Calling `Synchronize()` on one of them is redundant and counterproductive. Only use it on external sources that don't follow the contract. -> NOTE: this guideline refers to the **single-argument `Synchronize()`** that wraps a non-conforming source. The **gate-based `Synchronize(gate)`** used inside multi-source operators to satisfy §6.7 is a different pattern and is valid. +> NOTE: this refers to the **single-argument `Synchronize()`** for non-conforming sources. The **gate-based `Synchronize(gate)`** in multi-source operators (§6.7) is a different pattern and is valid. ### §5.9. Assume messages can come through until unsubscribe has completed -Because Rx uses a push model, messages can be sent from different execution contexts and can be in flight while `Dispose` is called. These messages **may still come through during the call to `Dispose`**. After `Dispose` returns control to the caller, no more messages will arrive. The unsubscription process itself may still be in progress on a different context. - -**When to ignore this guideline:** once `OnCompleted` or `OnError` has been received, the Rx grammar guarantees the subscription is finished, so this concern doesn't apply. +Messages can be in flight while `Dispose` is being called and may still arrive during the `Dispose` call. After `Dispose` returns control, no more messages arrive. Unsubscription itself may still be running on another context. ### §5.10. Use the Publish operator to share side-effects -Most observable sequences are cold: each subscription gets its own set of side effects. Some situations require side effects to happen only once. `Publish` shares a single underlying subscription among multiple subscribers via multicast. - -The most convenient overloads are the ones that accept a function with a shared observable sequence as an argument: -```csharp -var xs = Observable.Create(observer => -{ - Console.WriteLine("Side effect"); - observer.OnNext("Hi"); - observer.OnCompleted(); - return Disposable.Empty; -}); - -xs.Publish(sharedXs => -{ - sharedXs.Subscribe(Console.WriteLine); - sharedXs.Subscribe(Console.WriteLine); - return sharedXs; -}).Run(); -``` -The "Side effect" line prints once, not twice, because both subscribers share a single subscription to `xs`. +Most observables are cold: each subscription replays side effects. When side effects must happen only once, share via `Publish(shared => ...)` or `Publish().RefCount()`. -**When to ignore this guideline:** only use `Publish` when sharing is required. In most situations either the subscriptions have no side effects, or the side effects can execute multiple times without issues, and the extra machinery of `Publish` is unnecessary. +**When to ignore:** when subscriptions have no side effects, or when repeating them is harmless. The extra machinery is unnecessary. --- @@ -325,180 +121,68 @@ The "Side effect" line prints once, not twice, because both subscribers share a ### §6.1. Implement new operators by composing existing operators -Many operations can be composed from existing operators. This leads to smaller, easier-to-maintain code. The Rx team has put significant effort into corner cases in the base operators; composing them reuses that work for free. - +Composition reuses the corner-case handling the Rx team built into base operators: ```csharp public static IObservable SelectMany( this IObservable source, Func> selector) -{ - return source.Select(selector).Merge(); -} + => source.Select(selector).Merge(); ``` -`Select` already handles selector-thrown exceptions; `Merge` already handles serialization of inner sequences firing concurrently. The composed `SelectMany` gets both for free. +`Select` already protects against selector exceptions (§6.4); `Merge` already serializes (§6.7). -**When to ignore this guideline:** -- No appropriate set of base operators exists. -- Performance analysis proves the composed implementation has performance issues. +**When to ignore:** no appropriate base operators exist, OR profiling proves the composed form is too slow. ### §6.2. Implement custom operators using Observable.Create -When you cannot follow §6.1, use `Observable.Create` to create the observable sequence. `Observable.Create` provides several contract-protection benefits: -- When the sequence finishes (`OnError` or `OnCompleted`), any subscription is automatically unsubscribed. -- Any subscribed observer instance will only see a single terminal message. No further messages are sent, enforcing the grammar of §4.1. - -> **Modernization note:** the PDF refers to `Observable.CreateWithDisposable(Func, IDisposable>)`. In modern Rx.NET this is `Observable.Create(Func, IDisposable>)` (same signature, name unified). There is also an `Action`-returning overload; prefer the `IDisposable`-returning one for resource lifetime correctness. +When composition isn't enough, use `Observable.Create`. It enforces grammar compliance: auto-unsubscribe on terminal, single-terminal enforcement. -**When to ignore this guideline:** -- The operator needs to return an observable sequence that doesn't follow the Rx contract (rare; usually only for testing how code behaves under broken contracts). -- The returned object must implement more than `IObservable` (e.g. `ISubject` or a custom class). +**When to ignore:** the operator must return a non-conforming sequence (rare; usually testing), or the return type must implement more than `IObservable` (e.g. `ISubject`). ### §6.3. Implement operators for existing observable sequences as generic extension methods -An operator becomes more powerful when it can be applied widely. Implement operators as **extension methods** so they appear in IntelliSense on any existing observable sequence, and make them **generic** so they work regardless of element type. +Extension methods → IntelliSense on every sequence. Generics → applicable to any element type. -**When to ignore this guideline:** -- The operator doesn't work on a source observable sequence. -- The operator works on a specific data shape and cannot be made generic. +**When to ignore:** the operator doesn't work on a source sequence, or genuinely cannot be generic. ### §6.4. Protect calls to user code from within an operator -When user code is called from within an operator, the call may happen outside the execution context of the original operator invocation (e.g. asynchronously on a scheduler). Any exception that escapes will terminate the program unexpectedly. Instead, **catch and route to `observer.OnError`** so subscribers can handle it. - -Common kinds of user code that must be protected: -- Selector functions passed to the operator. -- Predicates passed to the operator. -- Comparers passed to the operator. -- Key selectors passed to the operator. -- Action callbacks (`Do`, `OnItemRemoved`, `SubscribeMany`, etc.) -- Calls to dictionaries, lists, and hashsets that use a user-provided comparer. +Wrap every user-provided delegate in try/catch and route exceptions to `observer.OnError`: +- Selectors, predicates, comparers, key selectors +- Action callbacks (`Do`, `OnItemRemoved`, `SubscribeMany`) +- Calls to dictionaries / lists / hashsets that use a user-provided comparer ```csharp -return Observable.Create(observer => source.Subscribe( +source.Subscribe( x => { TResult result; try { result = selector(x); } - catch (Exception exception) - { - observer.OnError(exception); - return; - } + catch (Exception ex) { observer.OnError(ex); return; } observer.OnNext(result); }, observer.OnError, - observer.OnCompleted)); + observer.OnCompleted); ``` -> NOTE: calls to `IScheduler` implementations are **not** considered for this guideline. Most schedulers deal with asynchronous calls, so only a small set of issues would be caught at the call site. Instead, protect the arguments passed to schedulers **inside each scheduler implementation**. +**Edge of the monad** (do NOT wrap): `Subscribe`, `Dispose`, `OnNext`, `OnError`, `OnCompleted`. Calling `OnError` from these places is undefined behavior. -**Edge of the monad:** do **not** wrap calls to `Subscribe`, `Dispose`, `OnNext`, `OnError`, or `OnCompleted`. Calling `OnError` from these places leads to undefined behavior. +**Exception:** for `IScheduler` calls, protect inside the scheduler implementation, not at every call site. -**When to ignore this guideline:** for calls to user code made before creating the observable sequence (outside `Observable.Create`). Those calls are on the current execution context and follow normal control flow. +**When to ignore:** for calls to user code made **before** creating the observable (outside `Observable.Create`). Those run on the current execution context and follow normal control flow. ### §6.5. Subscribe implementations should not throw -Subscribe may be called asynchronously (e.g. the second source argument to `Concat` is subscribed to only after the first source completes). A throw at that moment would bring down the program because there is no observer in scope to route to. Instead, **error conditions detected inside `Subscribe` must be routed via `observer.OnError(...)` followed by `return Disposable.Empty;`**. - -```csharp -public IObservable ReadSocket(Socket socket) => - Observable.Create(observer => - { - if (!socket.Connected) - { - observer.OnError(new InvalidOperationException("the socket is no longer connected")); - return Disposable.Empty; - } - // ... rest of subscribe ... - }); -``` +`Subscribe` may be called asynchronously (e.g. the second source of `Concat` after the first completes). A throw crashes the app because no observer is in scope. Route errors via `observer.OnError(...)` then `return Disposable.Empty;`. -**When to ignore this guideline:** when a catastrophic error occurs that should bring the whole program down anyway. +**When to ignore:** when the error is catastrophic and should bring the program down anyway. ### §6.6. OnError messages should have abort semantics -Normal .NET control flow uses abort semantics for exceptions: the stack is unwound, current code is interrupted. Rx mimics this. Once a source emits `OnError`, **the operator MUST emit no further messages — not even buffered or aggregated state.** - -The canonical violation: a buffering operator that "salvages" its buffer into a final `OnNext` on `OnError`. The buffer must be **discarded**: -```csharp -public static IObservable MinimumBuffer(this IObservable source, int bufferSize) => - Observable.Create(observer => - { - var data = new List(); - return source.Subscribe( - value => - { - data.AddRange(value); - if (data.Count > bufferSize) - { - observer.OnNext(data.ToArray()); - data.Clear(); - } - }, - observer.OnError, - () => - { - if (data.Count > 0) - observer.OnNext(data.ToArray()); - observer.OnCompleted(); - }); - }); -``` -The `OnCompleted` branch flushes the buffer (success path), but the `OnError` branch is direct passthrough: it abandons the buffer. The semantic is **abort**, not graceful degradation. - -Operators that aggregate state across multiple `OnNext` (Sort, Group, Page, etc.) must not salvage that state on `OnError`. - -**When to ignore this guideline:** none known. +Once `OnError` arrives, the operator MUST emit no further messages — **not even buffered or aggregated state.** The canonical violation: a buffering operator that "salvages" its buffer into a final `OnNext` on error. The buffer must be **discarded**. Aggregating operators (Sort, Group, Page, etc.) must not salvage state on error. ### §6.7. Serialize calls to IObserver methods within observable sequence implementations -Rx is composable: many operators play together. If every operator had to deal with concurrency individually, they would become very complex. Concurrency is best controlled at the place it first occurs. Consuming Rx would become harder if every consumer had to deal with concurrency too. - -**When combining multiple sources into one output observer, serialize all three notification types through a shared gate**: -```csharp -public static IObservable ZipEx( - this IObservable left, - IObservable right, - Func resultSelector) -{ - return Observable.Create(observer => - { - var group = new CompositeDisposable(); - var gate = new object(); - var leftQ = new Queue(); - var rightQ = new Queue(); - - group.Add(left.Subscribe( - value => - { - lock (gate) - { - if (rightQ.Count > 0) - { - var rightValue = rightQ.Dequeue(); - TResult result; - try { result = resultSelector(value, rightValue); } - catch (Exception e) { observer.OnError(e); return; } - observer.OnNext(result); - } - else - { - leftQ.Enqueue(value); - } - } - }, - observer.OnError, - observer.OnCompleted)); - - // ... symmetric handler for `right` using the same `gate` ... - - return group; - }); -} -``` -Two sources, one gate. **All three notification types — `OnNext`, `OnError`, `OnCompleted` — must be serialized through the same gate**, not just `OnNext`. Without this, the operator's internal state (the two queues) could be corrupted by interleaved deliveries. - -The equivalent pattern using `Synchronize(gate)` is more concise: +When combining multiple sources into one output, serialize **all three notification types** (OnNext, OnError, OnCompleted) through a shared gate: ```csharp var gate = new object(); source1.Synchronize(gate).Subscribe(observer); @@ -506,293 +190,112 @@ source2.Synchronize(gate).Subscribe(observer); ``` DynamicData's `CacheParentSubscription._synchronize` is the canonical example for cache operators. -**When to ignore this guideline:** -- The operator works on a single source observable sequence (then §6.8 applies). -- The operator does not introduce concurrency. -- Other constraints guarantee no concurrency is in play. +**When to ignore:** +- Single-source operator (§6.8 applies) +- No concurrency introduced +- Other constraints guarantee no concurrency -> NOTE: If a source observable sequence breaks the contract, a developer can fix it before passing it to an operator by calling `Synchronize()` (per §5.8). +> NOTE: if a source breaks the contract, the consumer can fix it with `Synchronize()` (§5.8) before passing it in. ### §6.8. Avoid serializing operators -Per §6.7 every operator already serializes; downstream operators can **assume** serialized input. Adding `Synchronize` "just in case" clutters the code, harms performance, and signals misunderstanding of the contract. - -If a source isn't following the contract, fix it at the consumer boundary with `Synchronize()` (per §5.8). The scope of additional synchronization should be limited to where it is needed. - -```csharp -// Select assumes its source follows §6.7 and requires no additional locking -public static IObservable Select( - this IObservable source, Func selector) => - Observable.Create(observer => source.Subscribe( - x => - { - TResult result; - try { result = selector(x); } - catch (Exception e) { observer.OnError(e); return; } - observer.OnNext(result); - }, - observer.OnError, - observer.OnCompleted)); -``` - -**When to ignore this guideline:** none known. +Per §6.7 every operator already serializes; downstream operators can **assume** serialized input. Adding `Synchronize` "just in case" clutters code, harms performance, and signals misunderstanding of the contract. Fix non-conforming sources at the consumer boundary (§5.8), not inside operators. ### §6.9. Parameterize concurrency by providing a scheduler argument -There are many different notions of concurrency, and no scenario fits all, so it is best to **parameterize the concurrency an operator introduces** via the `IScheduler` interface. - +Concurrency-introducing operators take an `IScheduler`: ```csharp -public static IObservable Return(TValue value, IScheduler scheduler) => - Observable.Create(observer => - scheduler.Schedule(() => - { - observer.OnNext(value); - observer.OnCompleted(); - })); +public static IObservable Return(TValue value, IScheduler scheduler) + => Observable.Create(observer => + scheduler.Schedule(() => { observer.OnNext(value); observer.OnCompleted(); })); ``` -**When to ignore this guideline:** -- The operator is not in control of creating the concurrency (e.g. an operator converting an event to an observable: the event source is in control, not the operator). -- The operator is in control but needs to use a specific scheduler for introducing concurrency. +**When to ignore:** the operator doesn't control concurrency creation (e.g. event-to-observable wrappers), or must use a specific scheduler internally. ### §6.10. Provide a default scheduler -In most cases there is a good default scheduler. Providing it as an overload makes calling code more succinct. - -```csharp -public static IObservable Return(TValue value) => - Return(value, Scheduler.Immediate); -``` - -> NOTE: when choosing the default, follow §6.12 — use `Scheduler.Immediate` where possible, only choose a scheduler with more concurrency when needed. +In most cases there is a good default. Provide it as an overload so callers can stay succinct. Per §6.12, prefer `Scheduler.Immediate`. -**When to ignore this guideline:** when no good default can be chosen. +**When to ignore:** when no good default exists. ### §6.11. The scheduler should be the last argument to the operator -Putting the scheduler last makes the operator fluent in IntelliSense. Combined with §6.10's default-providing overload, adding or omitting a scheduler becomes natural without changing argument order. +Makes the operator fluent in IntelliSense; combined with §6.10's overload, callers can add or omit the scheduler without changing argument order. -```csharp -public static IObservable Return(TValue value) => Return(value, Scheduler.Immediate); -public static IObservable Return(TValue value, IScheduler scheduler) => /* ... */; -``` - -**When to ignore this guideline:** C# and VB `params` syntax requires the `params` argument to be last. For `params T[]` operators, make the scheduler the **second-to-last** argument instead. +**When to ignore:** `params T[]` operators require `params` last. Make the scheduler the **second-to-last** argument instead. ### §6.12. Avoid introducing concurrency -Adding concurrency changes the timeliness of a sequence: messages are scheduled to arrive later, and **delivery time is itself observable data**. Adding concurrency skews that data. +Adding concurrency changes timeliness, and **delivery time is itself observable data** — concurrency skews that data. Defaults should be `Scheduler.Immediate` where possible; only introduce concurrency when essential to the operator's semantics. -This guideline includes not transferring control to a different context such as the UI context. - -```csharp -// Select does not use a scheduler — it stays on the source's OnNext call, -// staying in the same time window. -public static IObservable Select( - this IObservable source, Func selector) => - Observable.Create(observer => source.Subscribe( - x => - { - try { observer.OnNext(selector(x)); } - catch (Exception e) { observer.OnError(e); } - }, - observer.OnError, - observer.OnCompleted)); - -// Return defaults to Scheduler.Immediate — no concurrency introduced. -public static IObservable Return(TValue value) => Return(value, Scheduler.Immediate); -``` - -**When to ignore this guideline:** when introducing concurrency is an essential part of what the operator does. - -> NOTE: When using `Scheduler.Immediate` or calling the observer directly from within `Subscribe`, the `Subscribe` call becomes blocking. Any expensive computation in that situation is a candidate for introducing concurrency. +> NOTE: with `Scheduler.Immediate`, `Subscribe` becomes blocking. Expensive computation in that situation is a candidate for introducing concurrency. ### §6.13. Hand out all disposable instances created inside the operator to consumers -Disposable instances control subscription lifetime and cancellation of scheduled actions. Rx gives users an opportunity to unsubscribe via disposable instances. After a subscription has ended, no more messages are allowed through, and any leftover state inside the sequence is inefficient and can lead to unexpected semantics. - -To compose multiple disposable instances, Rx provides the `System.Reactive.Disposables` namespace: +Every `IDisposable` created inside an operator (subscriptions, scheduled actions, resources) MUST be reachable through the disposable returned to the subscriber. Compose via the `System.Reactive.Disposables` family: -| Name | Description | +| Type | Purpose | |---|---| -| `CompositeDisposable` | Composes and disposes a group of disposable instances together. | -| `SerialDisposable` *(modern; PDF says `MutableDisposable`)* | A holder for a replaceable disposable; assigning a new disposable disposes the previous. | -| `BooleanDisposable` | Maintains state on whether disposing has occurred. | -| `CancellationDisposable` | Wraps `CancellationToken` into the disposable pattern. | -| `ContextDisposable` | Disposes an underlying disposable in a specified `SynchronizationContext`. | -| `ScheduledDisposable` | Uses a scheduler to dispose an underlying disposable. | -| `SingleAssignmentDisposable` *(modern, not in PDF)* | Assignable once; throws on second assignment. Useful when the disposable is wired up asynchronously after subscription returns. | -| `RefCountDisposable` *(modern, not in PDF)* | Reference-counted disposable; underlying resource disposed only when all dependents released. | +| `CompositeDisposable` | Groups multiple disposables; dispose together | +| `SerialDisposable` | Replaceable holder; assignment disposes the previous | +| `SingleAssignmentDisposable` | Assignable once; throws on second assignment | +| `RefCountDisposable` | Underlying disposed only when all dependents released | +| `BooleanDisposable` | Exposes `IsDisposed` state | +| `CancellationDisposable` | Bridges `IDisposable` and `CancellationToken` | +| `ContextDisposable` | Disposes on a specified `SynchronizationContext` | +| `ScheduledDisposable` | Disposes via a scheduler | -```csharp -// Hand the group of internal subscriptions back to the subscriber via a CompositeDisposable -return Observable.Create(observer => -{ - var group = new CompositeDisposable(); - group.Add(left.Subscribe(/* ... */)); - group.Add(right.Subscribe(/* ... */)); - return group; -}); -``` - -**When to ignore this guideline:** none known. +Hidden disposables leak at unsubscribe time and break cleanup. ### §6.14. Operators should not block -Rx is a library for composing asynchronous and event-based programs over observable collections. A blocking operator loses those characteristics, and potentially loses composability (e.g. by returning `T` instead of `IObservable`). - -```csharp -public static IObservable Sum(this IObservable source) => - source.Aggregate(0, (prev, curr) => checked(prev + curr)); -``` -`Sum` returns `IObservable` rather than `int`. It does not block, and the result remains composable. If the developer wants to escape the observable world, they can use `First*`, `Last*`, or `Single*`. - -**When to ignore this guideline:** none known. +Return `IObservable`, never `T`. Aggregation operators like `Sum` return `IObservable`; callers escape via `First*`/`Last*`/`Single*` when they need a value. ### §6.15. Avoid deep stacks caused by recursion in operators -Code inside Rx operators can be called from different execution contexts in many different scenarios. It is nearly impossible to establish how deep the stack already is. A recursive operator can trigger stack overflow much sooner than expected. - -Two recommended ways to avoid this: -- Use the **recursive `Schedule` extension method** on `IScheduler`. -- Implement an **infinite-looping `IEnumerable>`** using `yield` and convert it via `Concat`. - -```csharp -// Sample 1: scheduler-based recursion -public static IObservable Repeat(TSource value, IScheduler scheduler) => - Observable.Create(observer => - scheduler.Schedule(self => - { - observer.OnNext(value); - self(); - })); - -// Sample 2: yield iterator + Concat -public static IObservable Repeat(TSource value) => - RepeatHelper(value).Concat(); - -private static IEnumerable> RepeatHelper(TSource value) -{ - while (true) - yield return Observable.Return(value); -} -``` -Schedulers such as the current-thread scheduler do not rely on stack semantics. The yield-iterator pattern ensures stack depth does not grow per iteration. - -**When to ignore this guideline:** none known. +Stack depth at operator invocation is unknown; recursive operators blow the stack faster than expected. Two solutions: +- The recursive `IScheduler.Schedule(self => ...)` overload +- `yield`-based `IEnumerable>` + `Concat` ### §6.16. Argument validation should occur outside Observable.Create -§6.5 requires that `Observable.Create`'s subscribe lambda not throw. Therefore **argument validation that may throw belongs outside the `Observable.Create(...)` call**: - +Per §6.5 the subscribe lambda must not throw. Therefore null checks and other validation belong **before** the `Observable.Create(...)` call: ```csharp -public static IObservable Select( - this IObservable source, Func selector) -{ - if (source == null) throw new ArgumentNullException(nameof(source)); - if (selector == null) throw new ArgumentNullException(nameof(selector)); - - return Observable.Create(observer => source.Subscribe(/* ... */)); -} +if (source == null) throw new ArgumentNullException(nameof(source)); +if (selector == null) throw new ArgumentNullException(nameof(selector)); +return Observable.Create(observer => /* ... */); ``` -**When to ignore this guideline:** when some aspect of the argument cannot be checked until the subscription is active (rare). +**When to ignore:** when validation genuinely requires the subscription to be live (rare). ### §6.17. Unsubscription should be idempotent -The `IDisposable` returned from `Subscribe` doesn't expose subscription state. Consumers don't know whether they've already disposed, and may dispose defensively. Multiple `Dispose` calls **must be allowed**: the first runs the cleanup, subsequent calls are no-ops. - -```csharp -var subscription = xs.Subscribe(Console.WriteLine); -subscription.Dispose(); -subscription.Dispose(); // no-op, must not throw -``` -Use a `_disposedValue` flag or equivalent guard. - -**When to ignore this guideline:** none known. +The `IDisposable` returned from `Subscribe` doesn't expose state. Consumers may dispose defensively. First `Dispose` runs cleanup; subsequent calls are no-ops. Use a `_disposedValue` flag or equivalent guard. ### §6.18. Unsubscription should not throw -Rx composition chains subscriptions, which means it also chains unsubscriptions. Any operator can trigger an unsubscription at any time. A throw will crash the application, and because the observer is already unsubscribed, `OnError` cannot route the exception. - -- If cleanup can fail, swallow + log; never propagate. -- When disposing multiple children, wrap each in try/catch so one failure doesn't skip the rest. - -**When to ignore this guideline:** none known. +Disposal cascades through compositions. A throw crashes the app, and the observer is already unsubscribed so `OnError` can't route the exception. +- If cleanup can fail: swallow + log; never propagate. +- When disposing multiple children: wrap each in try/catch so one failure doesn't skip the rest. ### §6.19. Custom IObservable implementations should follow the Rx contract -When you cannot follow §6.2 (use `Observable.Create`), the custom `IObservable` must still follow the Rx contract (§4) to behave correctly with Rx operators. +When you can't use `Observable.Create` (§6.2), your custom `IObservable` must still satisfy all of §4. You take on the burden the library would otherwise carry. -**When to ignore this guideline:** only when writing observable sequences that need to break the contract on purpose (e.g. for testing how code behaves when the contract is broken). +**When to ignore:** only for sequences that intentionally break the contract (e.g. testing how downstream code behaves under broken contracts). ### §6.20. Operator implementations should follow guidelines for Rx usage -Operators internally compose other operators (per §6.1). The §5 ("Using Rx") guidelines apply **recursively to operator internals**. - -**When to ignore this guideline:** as described in §2, only follow a guideline when it makes sense in that specific situation. - ---- - -## Common violation patterns (quick scan during code review) - -| Violation shape | Rule | Detection cue | -|---|---|---| -| Buffering operator flushes buffer on OnError | §6.6 | `OnError` handler calls OnNext before propagating | -| Multi-source operator forgets to serialize OnCompleted (only serializes OnNext) | §6.7 | `Synchronize` applied to OnNext but raw OnCompleted/OnError | -| Single-source operator wraps OnNext in unnecessary `lock(gate)` | §6.8 | `lock` inside a Transform/Filter/Select implementation | -| `null` check inside `Observable.Create(obs => { if (x == null) throw ... })` | §6.16 | Validation inside the subscribe lambda | -| Subscribe lambda throws on error condition rather than routing to OnError | §6.5 | `throw` inside `Observable.Create(observer => { ... throw ... })` | -| Dispose method throws (or contains code that can throw without catch) | §6.18 | `IDisposable.Dispose` without try/finally; multi-child dispose without per-child try/catch | -| Re-entrant Dispose runs cleanup twice | §6.17 | Missing `_disposedValue` flag or equivalent idempotency guard | -| Scheduler-queued work fires OnNext after Dispose | §4.4 | `IScheduler.Schedule(() => observer.OnNext(x))` without checking subscription state | -| User callback (selector, comparer, predicate, action) not protected by try/catch | §6.4 | Direct invocation of user delegate inside operator without wrapping | -| Recursive operator without `Schedule(self)` or `Concat`+yield | §6.15 | Direct self-recursion in OnNext handler | -| Hidden disposable not exposed to caller | §6.13 | `_ = scheduler.Schedule(...)` discarding the return value | -| Sum/Aggregate returns `T` instead of `IObservable` | §6.14 | Blocking return type on what should be an operator | -| Argument validation missing entirely on a public extension method | §6.16 | No null check on extension method parameters | - ---- - -## How to use this reference - -### When writing or modifying an operator -1. After implementing the operator, walk every rule in §4 and §6. -2. For each rule, confirm the operator does not violate it. -3. Document the audit in the PR description (e.g., "Audited against §4.1-§4.4, §6.1, §6.4-§6.8, §6.13, §6.16-§6.18; §6.7 N/A (single-source operator)"). - -### When reviewing a PR -1. Use the "Common violation patterns" table as a scanning checklist. -2. Cite rule IDs in review comments (e.g., "§6.6: buffer should be discarded on error, not flushed"). -3. Block merge on any unaddressed rule violation. - -### When debugging an Rx-related bug -1. Identify the rule(s) most likely to explain the symptom. -2. Add tests that exercise the contract boundary (use `Tests/Utilities/ValidateSynchronization` for §4.2, `TestSourceCache.SetError` for §4.4 / §6.6, etc.). -3. Reference the rule ID in the fix commit message. - -### Citing rules - -Always use the literal `§` character when citing rule IDs (in commit messages, PR descriptions, code review comments, and code comments). Do NOT use ASCII substitutes (`S`, `SS`, `sec`, `§§`, etc.). Example commit subject: - -``` -Fix §6.6 violation in BatchIf - -BatchIf was flushing its buffered changeset on source OnError, which -violates §6.6's abort semantic. Discard the buffer instead so error -deliveries are not preceded by stale OnNext. -``` +Operators internally compose other operators (§6.1). The §5 rules apply **recursively** inside operator implementations. --- ## Maintaining this document -This file is a derivative of the [Microsoft Rx Design Guidelines v1.0 (October 2010)](https://go.microsoft.com/fwlink/?LinkID=205219). The Microsoft document has not been republished since v1.0 and the underlying Rx contract is stable. **Consult the PDF directly when revising rule text, adding new rules, or resolving disputes about intent.** - -**Do NOT modify rule IDs.** External code reviews, PR descriptions, and commit messages cite the `§X.Y` IDs; renumbering breaks those references. The IDs come from the original PDF and changing them severs traceability. +Derivative of the [Microsoft Rx Design Guidelines v1.0 (October 2010)](https://go.microsoft.com/fwlink/?LinkID=205219). The Microsoft document has not been republished since v1.0 and the underlying Rx contract is stable. **Consult the PDF when revising rule text or resolving questions about original intent.** -**DO add new rules** in a new section (e.g., a DynamicData-specific section that goes beyond standard Rx) if the codebase discovers patterns the Microsoft document doesn't cover. Use a non-numeric prefix (e.g., `DD-1`, `DD-2`) to make clear they are not from the original spec. +**Do NOT modify rule IDs.** External code reviews, PRs, and commit messages cite `§X.Y` IDs; renumbering breaks references. -**Keep the Modernization notes table current.** When a 2010 API name appears in the PDF that has since been renamed, removed, or supplemented in Rx.NET, add a row. +**DO add new rules** in a new section if the codebase discovers patterns the Microsoft document doesn't cover. Use a non-numeric prefix (e.g., `DD-1`) to make clear they are not from the original spec. -**Cross-references:** see `rx.instructions.md` for DynamicData-specific patterns and practical Rx.NET reference material that complements this design guide (Defer pattern for per-subscription state, disposable family practical examples, the modern Rx.NET operator catalog, common pitfalls). That file links back here for any rule it touches on. +**See also:** `rx.instructions.md` for DynamicData-specific patterns and practical Rx.NET reference material (Defer pattern, disposable code samples, modern operator catalog, common pitfalls). From 1d77314acabf771a02adb98f8157073913bf030e Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Sun, 14 Jun 2026 12:50:52 -0700 Subject: [PATCH 20/24] Drop CacheParentSubscription mention from rx-design-guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CacheParentSubscription is being removed from DynamicData, so the §6.7 example pointing to it as the canonical cache-operator multi-source serialization site is becoming outdated. The gate-based Synchronize pattern shown above already conveys the rule without needing a class-specific anchor. --- .github/instructions/rx-design-guide.instructions.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/instructions/rx-design-guide.instructions.md b/.github/instructions/rx-design-guide.instructions.md index a43e6d8e..b08fd704 100644 --- a/.github/instructions/rx-design-guide.instructions.md +++ b/.github/instructions/rx-design-guide.instructions.md @@ -188,7 +188,6 @@ var gate = new object(); source1.Synchronize(gate).Subscribe(observer); source2.Synchronize(gate).Subscribe(observer); ``` -DynamicData's `CacheParentSubscription._synchronize` is the canonical example for cache operators. **When to ignore:** - Single-source operator (§6.8 applies) From faf6b0531261f50b66b4a519345d2d39006485fa Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Sun, 14 Jun 2026 12:51:29 -0700 Subject: [PATCH 21/24] Remove DynamicData references from rx-design-guide The design guide is a reference document for the Rx contract that predates this codebase and isn't specific to it. Codebase-specific commentary belongs in rx.instructions.md. Changes: - Intro: dropped "in DynamicData" from the scope sentence. - 5.8: "Rx and DynamicData operators already satisfy 4.1/4.2" becomes "Operators created by Rx already satisfy". - Maintaining section: new-rule prefix example changed from DD-1 to X-1; cross-reference paragraph no longer describes rx.instructions.md as DynamicData-specific. --- .github/instructions/rx-design-guide.instructions.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/instructions/rx-design-guide.instructions.md b/.github/instructions/rx-design-guide.instructions.md index b08fd704..0f08c785 100644 --- a/.github/instructions/rx-design-guide.instructions.md +++ b/.github/instructions/rx-design-guide.instructions.md @@ -3,7 +3,7 @@ applyTo: "**/*.cs" --- # Rx Design Guide -Complete distillation of the **[Microsoft Rx Design Guidelines v1.0 (October 2010)](https://go.microsoft.com/fwlink/?LinkID=205219)**. This is the authoritative reference for the Rx contract and the rules for using Rx and implementing Rx operators in DynamicData. +Complete distillation of the **[Microsoft Rx Design Guidelines v1.0 (October 2010)](https://go.microsoft.com/fwlink/?LinkID=205219)**. This is the authoritative reference for the Rx contract and the rules for using Rx and implementing Rx operators. **Code samples and API names have been updated to current Rx.NET (modernized from the 2010 spec).** Treat the conventions here as current. @@ -101,7 +101,7 @@ xs.Where(x => x.Failed).Do(x => Log(x)).Subscribe(...); ### §5.8. Use the Synchronize operator only to "fix" custom IObservable implementations -Rx and DynamicData operators already satisfy §4.1 / §4.2. Calling `Synchronize()` on one of them is redundant and counterproductive. Only use it on external sources that don't follow the contract. +Operators created by Rx already satisfy §4.1 / §4.2. Calling `Synchronize()` on one of them is redundant and counterproductive. Only use it on external sources that don't follow the contract. > NOTE: this refers to the **single-argument `Synchronize()`** for non-conforming sources. The **gate-based `Synchronize(gate)`** in multi-source operators (§6.7) is a different pattern and is valid. @@ -295,6 +295,6 @@ Derivative of the [Microsoft Rx Design Guidelines v1.0 (October 2010)](https://g **Do NOT modify rule IDs.** External code reviews, PRs, and commit messages cite `§X.Y` IDs; renumbering breaks references. -**DO add new rules** in a new section if the codebase discovers patterns the Microsoft document doesn't cover. Use a non-numeric prefix (e.g., `DD-1`) to make clear they are not from the original spec. +**DO add new rules** in a new section if the codebase discovers patterns the Microsoft document doesn't cover. Use a non-numeric prefix (e.g., `X-1`) to make clear they are not from the original spec. -**See also:** `rx.instructions.md` for DynamicData-specific patterns and practical Rx.NET reference material (Defer pattern, disposable code samples, modern operator catalog, common pitfalls). +**See also:** `rx.instructions.md` for practical Rx.NET reference material and codebase-specific patterns (Defer pattern, disposable code samples, modern operator catalog, common pitfalls). From 31bdda938623faac7fd626796d88fb406ac8b4ad Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Sun, 14 Jun 2026 13:04:29 -0700 Subject: [PATCH 22/24] Virtualise: collapse per-branch null filters into single post-merge filter Both request and dataChange branches were doing .Select(...).Where(x => x is not null).Select(x => x!) and then a third redundant .Where(updates => updates is not null) was applied after the UnsynchronizedMerge. The post-merge Where filtered nothing because both inputs were already non-null by then. Drop the per-branch null handling and do it once after the merge. Result matches the existing shape of the sibling operator Page.cs, which already used this cleaner form. No behavioral change. Verified with the Virtualise unit tests and the DeadlockTorture suite (30/30 pass). --- src/DynamicData/Cache/Internal/Virtualise.cs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/DynamicData/Cache/Internal/Virtualise.cs b/src/DynamicData/Cache/Internal/Virtualise.cs index 9e858f5d..8eb827a2 100644 --- a/src/DynamicData/Cache/Internal/Virtualise.cs +++ b/src/DynamicData/Cache/Internal/Virtualise.cs @@ -21,9 +21,13 @@ public IObservable> Run() => Observable.Create< var virtualiser = new Virtualiser(); var queue = new SharedDeliveryQueue(); - var request = _virtualRequests.SynchronizeSafe(queue).Select(virtualiser.Virtualise).Where(x => x is not null).Select(x => x!); - var dataChange = _source.SynchronizeSafe(queue).Select(virtualiser.Update).Where(x => x is not null).Select(x => x!); - return new CompositeDisposable(request.UnsynchronizedMerge(dataChange).Where(updates => updates is not null).SubscribeSafe(observer), queue); + var request = _virtualRequests.SynchronizeSafe(queue).Select(virtualiser.Virtualise); + var dataChange = _source.SynchronizeSafe(queue).Select(virtualiser.Update); + + return new CompositeDisposable(request.UnsynchronizedMerge(dataChange) + .Where(updates => updates is not null) + .Select(x => x!) + .SubscribeSafe(observer), queue); }); private sealed class Virtualiser(VirtualRequest? request = null) From 78fc364fdd165190aa8a4af7729cabaf267b6e16 Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Sun, 14 Jun 2026 13:07:45 -0700 Subject: [PATCH 23/24] GroupOn/GroupOnImmutable: collapse per-branch Where into post-merge Both files had the same redundant pattern Virtualise had: per-branch .Where(changes => changes.Count != 0) on each input of the UnsynchronizedMerge. Move the filter to a single post-merge call. No behavioral change. The set of changesets that reach the observer is identical; only the order of operations is changed. Verified with the GroupOn, GroupOnImmutable (GroupWithImmutableState), and DeadlockTorture suites (87/87 pass). --- src/DynamicData/Cache/Internal/GroupOn.cs | 6 +++--- src/DynamicData/Cache/Internal/GroupOnImmutable.cs | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/DynamicData/Cache/Internal/GroupOn.cs b/src/DynamicData/Cache/Internal/GroupOn.cs index 546e2e8f..9cf8ca0e 100644 --- a/src/DynamicData/Cache/Internal/GroupOn.cs +++ b/src/DynamicData/Cache/Internal/GroupOn.cs @@ -27,11 +27,11 @@ public IObservable> Run() => Observabl var queue = new SharedDeliveryQueue(); var grouper = new Grouper(_groupSelectorKey); - var groups = _source.SynchronizeSafe(queue).Finally(observer.OnCompleted).Select(grouper.Update).Where(changes => changes.Count != 0); + var groups = _source.SynchronizeSafe(queue).Finally(observer.OnCompleted).Select(grouper.Update); - var regroup = _regrouper.SynchronizeSafe(queue).Select(_ => grouper.Regroup()).Where(changes => changes.Count != 0); + var regroup = _regrouper.SynchronizeSafe(queue).Select(_ => grouper.Regroup()); - var published = groups.UnsynchronizedMerge(regroup).Publish(); + var published = groups.UnsynchronizedMerge(regroup).Where(changes => changes.Count != 0).Publish(); var subscriber = published.SubscribeSafe(observer); var disposer = published.DisposeMany().Subscribe(); diff --git a/src/DynamicData/Cache/Internal/GroupOnImmutable.cs b/src/DynamicData/Cache/Internal/GroupOnImmutable.cs index fc6f3b16..1f0923af 100644 --- a/src/DynamicData/Cache/Internal/GroupOnImmutable.cs +++ b/src/DynamicData/Cache/Internal/GroupOnImmutable.cs @@ -25,11 +25,11 @@ public IObservable> Run() => var queue = new SharedDeliveryQueue(); var grouper = new Grouper(_groupSelectorKey); - var groups = _source.SynchronizeSafe(queue).Select(grouper.Update).Where(changes => changes.Count != 0); + var groups = _source.SynchronizeSafe(queue).Select(grouper.Update); - var regroup = _regrouper.SynchronizeSafe(queue).Select(_ => grouper.Regroup()).Where(changes => changes.Count != 0); + var regroup = _regrouper.SynchronizeSafe(queue).Select(_ => grouper.Regroup()); - return new CompositeDisposable(groups.UnsynchronizedMerge(regroup).SubscribeSafe(observer), queue); + return new CompositeDisposable(groups.UnsynchronizedMerge(regroup).Where(changes => changes.Count != 0).SubscribeSafe(observer), queue); }); private sealed class Grouper(Func groupSelectorKey) From 146c6e9f33af2f79c37c78fede3c1f28a55d365f Mon Sep 17 00:00:00 2001 From: dwcullop Date: Mon, 15 Jun 2026 07:31:36 -0700 Subject: [PATCH 24/24] Trim PR-narration from gate-elimination comments Cleaned three patterns that fail the long-term maintenance test: - DeadlockTortureTest.TransformToTree: dropped the 'TreeBuilder.cs:200' line reference (file:line couplings rot the moment anyone touches the referenced file) and the inline expression mirror that duplicated it. - DeadlockTortureTest.Switch: dropped 'the refactored Switch.cs' and the implementation-detail enumeration. 'Refactored' won't mean anything once the PR is merged. - SynchronizeSafeExtensions: reframed 'Removing that gate matters' / 'the queue-drain design eliminated' from past-tense PR-narration to present-tense design rationale. Kept everything that describes the operator's contract or non-obvious WHY (ABBA-gate avoidance, disposal ordering, the ObserverBase one-shot flag rationale, etc.). --- src/DynamicData.Tests/Cache/DeadlockTortureTest.cs | 10 ++++------ src/DynamicData/Internal/SynchronizeSafeExtensions.cs | 5 +++-- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs b/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs index 4969eaed..e986b685 100644 --- a/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs +++ b/src/DynamicData.Tests/Cache/DeadlockTortureTest.cs @@ -275,9 +275,8 @@ [Fact] public async Task ThreeWayCircular_DoesNotDeadlock() [Fact] public async Task TransformToTree_DoesNotDeadlock() { - // Exercises TreeBuilder.cs:200 (_predicateChanged.SynchronizeSafe(queue).UnsynchronizedCombineLatest - // (reFilterObservable.SynchronizeSafe(queue), ...)). Cross-cache cycle is closed via a side-channel - // Subscribe that writes a marker into the other cache for every tree changeset. + // Cross-cache cycle is closed via a side-channel Subscribe that writes a marker + // into the other cache for every tree changeset. for (var iter = 0; iter < Iterations; iter++) { using var sourceA = new SourceCache(p => p.UniqueKey); @@ -302,8 +301,7 @@ [Fact] public async Task TransformToTree_DoesNotDeadlock() } [Fact] public async Task Switch_DoesNotDeadlock() => - // Exercises the refactored Switch.cs (SerialDisposable + UnsynchronizedMerge of destination.Connect() - // and the errors subject). Observable.Return(s).Switch() drives exactly one outer notification, which - // is enough to wire up the destination cache and exercise the gate-free merge on every inner change. + // Observable.Return(s).Switch() drives exactly one outer notification, which is enough + // to wire up the destination cache and exercise its inner-change delivery path. (await RunBidirectionalDeadlockTest(s => System.Reactive.Linq.Observable.Return(s).Switch())).Should().BeTrue(); } diff --git a/src/DynamicData/Internal/SynchronizeSafeExtensions.cs b/src/DynamicData/Internal/SynchronizeSafeExtensions.cs index 603f545c..063e9270 100644 --- a/src/DynamicData/Internal/SynchronizeSafeExtensions.cs +++ b/src/DynamicData/Internal/SynchronizeSafeExtensions.cs @@ -77,7 +77,7 @@ public static IObservable SynchronizeSafe(this IObservable source) => // that at most one notification is in flight to the downstream observer at a time, so the // additional gate that Observable.Merge would install is redundant. // - // Removing that gate matters in cross-cache pipelines: Observable.Merge holds its private + // The gate omission matters in cross-cache pipelines: Observable.Merge holds its private // _gate for the entire duration of downstream delivery, and when downstream delivery walks // into another cache's writer lock, two such gates on two operators form an ABBA cycle that // the queue-drain design is meant to prevent. @@ -145,7 +145,8 @@ void OnCompletedSafe() // // The Rx gate matters here for the same reason as Merge: Observable.CombineLatest holds // its private _gate for the entire downstream delivery, and any operator-level lock held - // across a cross-cache write reconstructs the ABBA cycle the queue-drain design eliminated. + // across a cross-cache write reconstructs the ABBA cycle the queue-drain design is meant + // to prevent. // // Without the external serialization precondition, concurrent OnNext calls would race the // latest-value state and could produce torn reads. Do not use as a general-purpose