fix(binding): stop memoizing a failed ICreatesObservableForProperty lookup - #58
Merged
Merged
Conversation
…ookup - Resolve the factory through a helper that reads the MRU cache with TryGet and only writes back a resolution that found an implementation. - A lookup made while the locator is still empty previously stuck for the life of the process, so that sender and property kept throwing "Could not find a ICreatesObservableForProperty" long after registration had happened. - Supply the entry through the cache's context argument so the write costs no second locator scan; the read path drops from 45.0ns to 18.9ns. - Cover both the by-name and the expression-chain path with a lookup that runs before registration and succeeds after it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #58 +/- ##
=======================================
Coverage 97.61% 97.61%
=======================================
Files 227 227
Lines 7744 7744
Branches 1062 1062
=======================================
Hits 7559 7559
Misses 143 143
Partials 42 42 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
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.



What kind of change does this PR introduce?
Bug fix (runtime property observation lookup).
What is the new behavior?
ICreatesObservableForPropertylookup is no longer remembered. If nothing is registered when a property is first observed, a later lookup for the same property re-scans the locator and succeeds once registration has happened.TryGetinstead ofGetmeasures 18.94 ns against 45.02 ns, a ratio of 0.42, because supplying the resolved value through the cache context removes a second locator scan on the write path.What is the current behavior?
Closes #31
The resolution cache is keyed by sender type, property name and before-change flag, and its factory returns
nullwhen nothing in the locator bids a positive affinity. Thatnullis memoized permanently and nothing invalidates it. A single lookup that happens before registrations land therefore disables that key for the lifetime of the process, and every later call throwsCould not find a ICreatesObservableForPropertyeven though registration has since completed correctly. A different property on the same type continues to work, which is why the failure looks intermittent and is most visible inside a test run where initialization order varies.What might this PR break?
TryGetandGetare each internally gated, and the only race left is a redundant re-resolve of the same key, never an incorrect result.Checklist
mainbranchAdditional information
Evidence for the diagnosis. A throwaway app against the runtime project, doing lookup then registration then lookup, produced:
Two regression tests cover the by-name and expression paths. Both were confirmed failing against the unmodified runtime on net8, net9, net10 and net11 using the committed test bodies, and passing with the change.
Ruled out while investigating:
AppLocator.CurrentandLocator.Currentare the same instance, so there is no split between the store this library reads and the one others register into; and the generator does not intercept ReactiveUI's ownWhenAnyValue, whose containing type is not one of the recognised extension classes.Two related problems were found and are deliberately not addressed here, since they are separate from the caching defect:
ReactiveUI.Bindingcan force a Splat assembly-version downgrade that stops ReactiveUI binding to Splat at all, which produces a different failure with the same "add these two packages, remove them again" shape.Build is clean with 0 warnings and 0 errors. The full suite passes 10764/10764, run twice, with the runtime test project run three further times on its own to check for order dependence.