Defer the visible content of the view together with the notification (#115) - #125
Open
aetos382 wants to merge 13 commits into
Open
Defer the visible content of the view together with the notification (#115)#125aetos382 wants to merge 13 commits into
aetos382 wants to merge 13 commits into
Conversation
When notifications are deferred by ICollectionEventDispatcher, the internal list was updated immediately on the mutating thread, so a subscriber on the dispatcher thread observed a state ahead of the notification it received. - add DeferredViewList<TView>, which holds the list as subscribers see it and applies each change at the same time as the notification is raised - translate visible indexes into writer-side indexes on positional writes (setter, Insert, RemoveAt) by replaying the pending changes - fix SynchronizationContextCollectionEventDispatcher.Post to compare against the bound SynchronizationContext instead of testing Current for null - fix duplicated notification on Move in the filtered view - document the limitation of ToNotifyCollectionChangedSlim, which shares the actual data and therefore can not defer its content Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Only the head is ever taken, so a queue expresses the intent and drops the O(n) List.RemoveAt(0). Draining n pending changes in order becomes O(n) instead of O(n squared), which matters because the queue grows whenever the mutating thread runs ahead of the dispatcher thread. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The setter updated the visible list without a notification, so its content diverged from the notification stream and never recovered. Route the change through Publish, and notify only when the converter rejects the source write because otherwise the Replace of the source carries the same content. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The drain loop of InvokeChangedEvent raises an older pending event before the event it was called for. A subscriber exception escaped the loop and left the newer events pending, so their content and notification were never delivered until the next change happened, if any. Collect the exceptions, finish the drain, then rethrow. A single exception is rethrown with ExceptionDispatchInfo so the behavior stays as it was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
When the source does not provide the index, the view looks the element up by value. A filter that is not pure can make the lookup miss, and the resulting notification carried -1 as its index. Such an event can not be applied to the deferred list, so it threw inside the gate after being dequeued and broke the consistency between the visible content and the notifications for good. The multiple items branch also never updated the index and raised the whole range once per element. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ToListViewIndex is the only entry point of the indexer setter, Insert and RemoveAt, so validate the index there before translating it. Filtable fell back to appending when the translated index was past the end of listView, which was meant for the insertion point at the tail but also swallowed an out of range index. Insert(Count + 1, x) appended silently instead of throwing, with or without a dispatcher. UntrackableIndex is -1 as well, so an index of -1 passed by the caller was mistaken for an untrackable one. Insert(-1, x), RemoveAt(-1) and this[-1] = x threw InvalidOperationException with a message about another thread, only when a dispatcher was configured. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The message blamed another thread for changing the element, but a dispatcher defers a change of the same thread as well, and the change is never a Replace, which does not move any element. It is either a remove of the element itself or a reset of the whole content. Return the reason from ToWriterIndex and tell which one it is, and what to do about it. A pending reset makes every index unresolvable, so do not report an index for it as if another one could work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adding the tests did not add any project to the solution, so its only real change was the version of Visual Studio that happened to open it. The rest of the diff was the line endings being rewritten from LF to CRLF. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bumping it to net10.0 has nothing to do with issue Cysharp#115. Restore net6.0 and stop using a collection expression, which the resulting C# 10 does not have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Defers synchronized-view state alongside dispatched notifications to keep UI-visible content and event indices consistent.
Changes:
- Adds deferred view state and writable-index translation.
- Fixes dispatcher thread detection and notification inconsistencies.
- Adds documentation and extensive regression tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
README.md |
Documents slim-view threading limitations. |
src/ObservableCollections/ICollectionEventDispatcher.cs |
Corrects synchronization-context dispatching. |
src/ObservableCollections/Internal/DeferredViewList.cs |
Implements deferred visible state. |
src/ObservableCollections/ObservableList.OptimizeView.cs |
Documents slim-view behavior. |
src/ObservableCollections/SynchronizedViewList.cs |
Integrates deferral and index translation. |
tests/ObservableCollections.Tests/DeferredNotificationConsistencyTest.cs |
Tests notification-state consistency. |
tests/ObservableCollections.Tests/DeferredNotificationTest.cs |
Covers deferred behavior and writes. |
tests/ObservableCollections.Tests/MultipleUiThreadTest.cs |
Tests multiple UI contexts. |
tests/ObservableCollections.Tests/NotifyCollectionChangedContractTracker.cs |
Adds notification contract validation. |
tests/ObservableCollections.Tests/QueuedCollectionEventDispatcher.cs |
Adds queued dispatcher fixture. |
tests/ObservableCollections.Tests/QueuedSynchronizationContext.cs |
Adds synchronization-context fixture. |
tests/ObservableCollections.Tests/TestUiThread.cs |
Adds simulated UI thread. |
tests/ObservableCollections.Tests/ToNotifyCollectionChangedTest.cs |
Cleans unused imports. |
tests/ObservableCollections.Tests/WritableViewIndexTranslationTest.cs |
Tests writable index translation. |
Suppressed comments (3)
src/ObservableCollections/SynchronizedViewList.cs:1167
- When the converter rejects the source write (
setValue == false) and no dispatcher was supplied, this condition skipsPublish, so the notify collection changes its visible item without raisingCollectionChanged. Emit the syntheticReplacefor the inline path as well;Publishalready supports it.
if (deferred != null)
src/ObservableCollections/SynchronizedViewList.cs:1076
- Breaking as soon as the callback's own event is reached leaves any newer pending changes untouched. If this callback's subscriber throws, the exception is rethrown and can abort the dispatcher pump before those callbacks run, so remaining pending changes are not applied and raised before rethrow. Drain all changes already pending at callback entry before propagating collected exceptions.
if (ReferenceEquals(applied, e)) break;
src/ObservableCollections/SynchronizedViewList.cs:1174
- Suppressing this synthetic notification assumes
SetToSourceCollectionwill always deliver the sourceReplace. AnObservableList.CollectionChangedsubscriber registered before the view can throw before the view handler runs; the setter then throws afterlistViewwas changed, and this queued change is applied silently with no source notification to replace it. Ensure the synthetic update is notified or rolled back when the source update fails.
IsInvokeCollectionChanged = !setValue, // the Replace of the source carries the same content
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+487
to
+497
| if (deferred != null) | ||
| { | ||
| // never touch the visible list without a notification, the Replace of the source updates it | ||
| Publish(new CollectionEventDispatcherEventArgs(NotifyCollectionChangedAction.Replace, value, oldView, listViewIndex) | ||
| { | ||
| Collection = this, | ||
| Invoker = raiseChangedEventInvoke, | ||
| IsInvokeCollectionChanged = !setValue, // the Replace of the source carries the same content | ||
| IsInvokePropertyChanged = false | ||
| }); | ||
| } |
| (exceptions ??= new()).Add(ex); | ||
| } | ||
|
|
||
| if (ReferenceEquals(applied, e)) break; |
Comment on lines
+85
to
+93
| bool IsPending(CollectionEventDispatcherEventArgs ev) | ||
| { | ||
| foreach (var change in pending) | ||
| { | ||
| if (ReferenceEquals(change.Args, ev)) | ||
| { | ||
| return true; | ||
| } | ||
| } |
| { | ||
| Collection = this, | ||
| Invoker = raiseChangedEventInvoke, | ||
| IsInvokeCollectionChanged = !setValue, // the Replace of the source carries the same content |
The previous commit routed the set of a writable view through Publish, but it still updated listView first and then suppressed the notification when the source write was going to carry the same content. Both halves were wrong. When the write goes through, the parent view recomputes the view from the selector, so the value put into listView is overwritten by selector(newOriginal) a moment later. The deferred list applied the change twice, the first time without a notification. When the source write did not come back - either SetToSourceCollection throws, or a CollectionChanged subscriber registered before the view throws so the view never sees the Replace - that first application was all that was left, and the visible content changed with nothing to say so. Let the Replace of the source be the only change. When the converter rejects the write there is no source notification at all, so the set has to notify by itself. It was guarded by a dispatcher being configured, so without one the visible content changed silently. That was the behavior before this branch as well. Both were pointed out by Copilot on the pull request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
IsPending walked the queue on every TryApplyNext. In the normal case the target sits at the head and the walk ends on the first comparison, but when a synchronous notification overtakes the queued ones - the case the drain loop exists for - the target is at the tail, so draining N changes compared N + (N-1) + ... + 1 times while holding the gate. The queue already applies the changes strictly in the order they were enqueued and no event is enqueued twice, so an increasing sequence per change answers the same question by itself: an event is pending exactly while its sequence is above the one applied last. An event that was never enqueued keeps the default 0 and is never reported as pending, so the ApplyWithoutNotification path is unaffected. Hoisting the check out of the drain loop instead would not work, because the loop releases the gate to raise the notification and another callback may dequeue the target in the meantime. Pointed out by Copilot on the pull request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
When
ToNotifyCollectionChanged()is given anICollectionEventDispatcher, only theCollectionChangedinvocation is deferred to the UI thread; the internal list (listView) isupdated immediately, on the mutating thread.
As a result, when the collection is mutated from a thread other than the UI thread, the content has already moved on by the time the UI thread receives the notification, so the indices carried by the
notification no longer match the content. A consumer that reads the collection right after the notification — such as the WinUI 3
ListView, which callsIBindableVector.GetAt(index)— crashesthere with an out-of-range access (#115).
Changes
1. Defer the visible content together with the notification (the core of #115)
Added
Internal/DeferredViewList<TView>. When a dispatcher is specified, the view now holds both the list as the subscribers see it (published) and a queue of not-yet-raised changes (pending):pendingbeforeICollectionEventDispatcher.Postpublishedat the moment the notification is raisedThis keeps the content of the notification and the visible content consistent at all times.
this[int]/Count/IndexOf/Contains/GetEnumeratorall readpublished.The following are also guaranteed:
and raised first, in order).
Resetdoes not carry items, so the writer-side content is captured as a snapshot atPosttime.later does not receive past notifications.
AggregateExceptionwhen there is more than one).CollectionChanged/PropertyChangedbecame explicit events whose add / remove are guarded bygate, so that "there is no subscriber" can be decided atomically with applying a change.2. Index translation for position-based writes on writable views
The indexer setter,
InsertandRemoveAtofToWritableNotifyCollectionChanged()passed the view index straight through to the source collection. This is now fixed:AlternateIndexList.GetAlternateIndex(previously it was off by the number of filtered-out elements).Insertat the tail of the view has no determined source index, so it is treated asAddToSourceCollection.ArgumentOutOfRangeException, whether or not a dispatcher is used.Resetmakes any index unresolvable, anInvalidOperationExceptionis thrown, with the two reasons (element removed vs. collection reset) worded distinctly.Replaceso that the visible content is never mutated without a notification.3. Thread check in
SynchronizationContextCollectionEventDispatcherThe condition for raising synchronously was "
SynchronizationContext.Currentis not null", so in a process with more than one thread that has aSynchronizationContext, a mutation from a UI threadother than the bound one was raised directly on that thread. The check is now "is
Currentthe same context this dispatcher is bound to".public static readonly ... Current = current.Value;was also changed to=> current.Value.Previously the
Lazywas evaluated during static initialization of the type, so if a thread without aSynchronizationContexttouched the type first, the result was aTypeInitializationException.4. Notification inconsistencies (found while investigating #115)
Moveon a filtered view raised the notification with the translated indices and then fell through and raised the original one as well — notifying the same move twice, the second time with untranslated indices.listViewis unchanged, no notification must be raised. (Enqueuing a notification with an unknown index (-1) into the deferral queue makes it inapplicable at raise time, permanently breaking consistency.)OldStartingIndex == -1raised, for each element, the arguments of the whole batch, and with an uninitializedindexof -1. It now raises a single-item notification carrying the position actually removed.5. Documented the limitation of
ToNotifyCollectionChangedSlim()By design
ToNotifyCollectionChangedSlim()shares the data with the source, so its visible content cannot be deferred and the same inconsistency remains. This is accepted as the trade-off for its performance, and is documented instead: the XML docremarksand the README now state that when it is combined with a dispatcher, the collection must be mutated on the dispatcher thread.Behavioral changes
Countor the indexer until the notification is raised — even as seen from the mutating thread. This is intentional; consistency with the notification is the point.InvalidOperationExceptionfor an index that cannot be resolved while notifications are pending.SynchronizationContextCollectionEventDispatcher.Currentchanged from astatic readonlyfield to a property, so binary compatibility is not preserved.Known bugs not fixed in this PR
typeof(T) == typeof(TView), soAdd/Insert/Removeignore the converter even for cases that need conversion between identical types (split out into a separate branch; out of scope here).SynchronizationContextCollectionEventDispatcher.Currentis pinned to theSynchronizationContextof whichever thread reads it first, andLazy<T>caches exceptions too, so if a thread without a context reads it first, it throwsInvalidOperationExceptionforever for the rest of the process.(Making it a property avoids the static-initialization failure, but the pinning and the cached exception remain.)
ToNotifyCollectionChangedSlim()still carries the same inconsistency as described above; only documentation was added.