Repository navigation
fix: ObserveCountChanged drops a count change that happens while subscribing - #140
Open
armadavalor wants to merge 1 commit into
Open
armadavalor wants to merge 1 commit into
armadavalor wants to merge 1 commit into
Conversation
armadavalor
force-pushed
the
fix/132-count-changed-subscribe-race
branch
from
November 2, 2026 19:55
9895b59 to
beb6461
Compare
armadavalor
force-pushed
the
fix/132-count-changed-subscribe-race
branch
from
November 2, 2026 19:56
beb6461 to
db0ecbc
Compare
There was a problem hiding this comment.
🟡 Changes recommended
With notifyCurrentCount enabled, a reentrant attachment-time reset produces duplicate initial count notifications ([0, 0]) that the outer lock does not prevent.
1 open issue
What changed in this PR
This PR addresses lost count-change notifications during R3 ObserveCountChanged subscriptions, as reported in #132.
Changes:
- Initializes the previous count through a new pre-attachment hook.
- Locks subscription setup to prevent concurrent mutations between handler attachment and the initial notification.
- Adds a regression test that clears the collection during handler attachment.
| File | Description |
|---|---|
| tests/ObservableCollections.R3.Tests/ObservableCollectionExtensionsTest.cs | Adds an attach-time mutation helper and regression test. |
| src/ObservableCollections.R3/ObservableCollectionR3Extensions.cs | Initializes count tracking before attachment and locks subscription setup. |
Review effort: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Fixes the lost count change in
ObserveCountChangeddescribed in #132.Cause: the base observer constructor attached the
CollectionChangedhandler before the derived constructor initializedcountPrev. AResetraised in that window was compared against the default0and dropped, and the derived constructor then overwrotecountPrevwith the new count, so nothing could detect it afterwards.Fix:
ObservableCollectionObserverBasegets aprotected virtual OnBeforeAttach()hook that runs before the handler is attached.countPrevis initialized there.lock (collection.SyncRoot), so a change from another thread cannot land between attaching the handler and thenotifyCurrentCount: truenotification. This also covers the duplicated or out-of-order first value mentioned in the issue.Verification: added
ObserveCountChanged_ChangeWhileSubscribing, which uses the decorator from the issue to clear the collection right after the handler is attached. It fails without the change (nothing emitted) and passes with it. I only had a .NET 10 SDK with the runtime rolled forward (DOTNET_ROLL_FORWARD=Major), so the net8.0 test project ran on that:Passed! - Failed: 0, Passed: 11, Skipped: 0, Total: 11 - ObservableCollections.R3.Tests.dll (net8.0)Closes #132