From a1c4e145cb4424ecf81f8cccce475c4760dfd0ff Mon Sep 17 00:00:00 2001 From: Glenn Watson <5834289+glennawatson@users.noreply.github.com> Date: Sun, 2 Aug 2026 12:02:36 +1000 Subject: [PATCH] fix(binding): stop memoizing a failed ICreatesObservableForProperty lookup - 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. --- .../ReactiveNotifyPropertyChangedMixins.cs | 60 ++++++++++++------ ...ReactiveNotifyPropertyChangedMixinTests.cs | 59 ++++++++++++++++++ .../TestModels/LateRegistrationFixture.cs | 61 +++++++++++++++++++ 3 files changed, 161 insertions(+), 19 deletions(-) create mode 100644 src/tests/ReactiveUI.Binding.Tests/TestModels/LateRegistrationFixture.cs diff --git a/src/ReactiveUI.Binding.Shared/ObservableForProperty/ReactiveNotifyPropertyChangedMixins.cs b/src/ReactiveUI.Binding.Shared/ObservableForProperty/ReactiveNotifyPropertyChangedMixins.cs index 78ea5bd..f754d8c 100644 --- a/src/ReactiveUI.Binding.Shared/ObservableForProperty/ReactiveNotifyPropertyChangedMixins.cs +++ b/src/ReactiveUI.Binding.Shared/ObservableForProperty/ReactiveNotifyPropertyChangedMixins.cs @@ -29,27 +29,20 @@ public static class ReactiveNotifyPropertyChangedMixins "This should never happen, your service locator is probably broken. Please make sure you have registered ICreatesObservableForProperty implementations."; /// MRU cache that maps (sender type, property name, before-change flag) to the best implementation for that combination. + /// + /// The entry is supplied through the context argument rather than looked up here, so only a + /// resolution that actually found an implementation is ever stored. A failure is deliberately not + /// stored: the locator can still be empty the first time a property is observed — an observation + /// running ahead of application startup, or a test suite whose registration belongs to a later + /// test — and a stored failure would outlive the registration that fixes it, leaving that sender + /// and property permanently unobservable. + /// private static readonly MemoizingMRUCache< (Type senderType, string propertyName, bool beforeChange), - ICreatesObservableForProperty?> + ICreatesObservableForProperty> NotifyFactoryCache = new( - static (t, _) => - { - var bestScore = 0; - ICreatesObservableForProperty? best = null; - foreach (var candidate in AppLocator.Current.GetServices()) - { - var score = candidate.GetAffinityForObject(t.senderType, t.propertyName, t.beforeChange); - if (score > bestScore) - { - bestScore = score; - best = candidate; - } - } - - return best; - }, + static (_, resolved) => (ICreatesObservableForProperty)resolved!, NotifyFactoryCacheSize); /// Provides ObservableForProperty extension members for . @@ -115,7 +108,7 @@ public IObservable> ObservableForProperty> SubscribeToExpressionChain< "The expression does not have valid member info", nameof(expression)); var propertyName = memberInfo.Name; - var result = NotifyFactoryCache.Get((sender.GetType(), propertyName, beforeChange)); + var result = ResolveNotifyFactory((sender.GetType(), propertyName, beforeChange)); return result switch { @@ -299,4 +292,33 @@ public IObservable> SubscribeToExpressionChain< _ => result.GetNotificationForProperty(sender, expression, propertyName, beforeChange) }; } + + /// + /// Gets the highest-affinity for a sender type and + /// property, consulting the service locator only when the combination has not already resolved. + /// + /// The sender type, property name and before-change flag being resolved. + /// The best implementation, or when nothing bids a positive affinity. + private static ICreatesObservableForProperty? ResolveNotifyFactory( + (Type senderType, string propertyName, bool beforeChange) key) + { + if (NotifyFactoryCache.TryGet(key, out var memoized)) + { + return memoized; + } + + var bestScore = 0; + ICreatesObservableForProperty? best = null; + foreach (var candidate in AppLocator.Current.GetServices()) + { + var score = candidate.GetAffinityForObject(key.senderType, key.propertyName, key.beforeChange); + if (score > bestScore) + { + bestScore = score; + best = candidate; + } + } + + return best is null ? null : NotifyFactoryCache.Get(key, best); + } } diff --git a/src/tests/ReactiveUI.Binding.Tests/ObservableForProperty/ReactiveNotifyPropertyChangedMixinTests.cs b/src/tests/ReactiveUI.Binding.Tests/ObservableForProperty/ReactiveNotifyPropertyChangedMixinTests.cs index 36e9738..f31ca3a 100644 --- a/src/tests/ReactiveUI.Binding.Tests/ObservableForProperty/ReactiveNotifyPropertyChangedMixinTests.cs +++ b/src/tests/ReactiveUI.Binding.Tests/ObservableForProperty/ReactiveNotifyPropertyChangedMixinTests.cs @@ -318,6 +318,65 @@ public async Task ObservableForProperty_ByName_GetCurrentValue_ValIsTValue_Succe await Assert.That(receivedValue).IsEqualTo("Test"); } + /// + /// Verifies that a lookup made while no is registered + /// does not stop the same sender and property from resolving once registration has happened. + /// + /// A task representing the asynchronous test operation. + [Test] + public async Task ObservableForProperty_ByName_LookupBeforeRegistration_ResolvesAfterRegistration() + { + RxBindingBuilder.ResetForTesting(); + + var vm = new LateRegistrationFixture { Name = InitialValue }; + + await Assert.That(() => vm.ObservableForProperty( + nameof(LateRegistrationFixture.Name), + skipInitial: false)) + .ThrowsExactly(); + + EnsureInitialized(); + + var values = new List>(); + using var sub = vm.ObservableForProperty( + nameof(LateRegistrationFixture.Name), + skipInitial: false) + .Subscribe(values.Add); + + vm.Name = ChangedValue; + + await Assert.That(values.Count).IsGreaterThanOrEqualTo(ExpectedTwoEmissions); + } + + /// + /// Verifies that a nested-chain lookup made while no + /// is registered does not stop the same sender and property from resolving once registration + /// has happened. + /// + /// A task representing the asynchronous test operation. + [Test] + public async Task NotifyForProperty_LookupBeforeRegistration_ResolvesAfterRegistration() + { + RxBindingBuilder.ResetForTesting(); + + var vm = new LateRegistrationFixture { Title = InitialValue }; + Expression> expr = x => x.Title; + var body = Reflection.Rewrite(expr.Body); + + await Assert.That(() => ReactiveNotifyPropertyChangedMixins.NotifyForProperty(vm, body, false)) + .ThrowsExactly(); + + EnsureInitialized(); + + var results = new List>(); + using var sub = ReactiveNotifyPropertyChangedMixins.NotifyForProperty(vm, body, false) + .Subscribe(results.Add); + + vm.Title = ChangedValue; + + await Assert.That(results.Count).IsGreaterThanOrEqualTo(1); + } + /// Resets and initializes the ReactiveUI binding infrastructure for testing. private static void EnsureInitialized() { diff --git a/src/tests/ReactiveUI.Binding.Tests/TestModels/LateRegistrationFixture.cs b/src/tests/ReactiveUI.Binding.Tests/TestModels/LateRegistrationFixture.cs new file mode 100644 index 0000000..58231f8 --- /dev/null +++ b/src/tests/ReactiveUI.Binding.Tests/TestModels/LateRegistrationFixture.cs @@ -0,0 +1,61 @@ +// Copyright (c) 2019-2026 ReactiveUI Association Incorporated. All rights reserved. +// ReactiveUI Association Incorporated licenses this file to you under the MIT license. +// See the LICENSE file in the project root for full license information. + +using System.ComponentModel; +using System.Runtime.CompilerServices; + +namespace ReactiveUI.Binding.Tests.TestModels; + +/// +/// A fixture observed once while the locator is still empty and again after registration. +/// It is used by no other test, so the factory-resolution cache entry for its properties is +/// created solely by the test that exercises that sequence. +/// +public class LateRegistrationFixture : INotifyPropertyChanged +{ + /// + public event PropertyChangedEventHandler? PropertyChanged; + + /// Gets or sets the value observed through the by-name overload. + public string Name + { + get => field; + set + { + if (field == value) + { + return; + } + + field = value; + OnPropertyChanged(); + } + } = string.Empty; + + /// Gets or sets the value observed through the expression-chain path. + /// + /// A resolution that succeeds is memoized for the lifetime of the process, so each test that + /// observes this fixture before registration needs a property of its own to start from an + /// unresolved cache entry. + /// + public string Title + { + get => field; + set + { + if (field == value) + { + return; + } + + field = value; + OnPropertyChanged(); + } + } = string.Empty; + + /// Raises the PropertyChanged event. + /// The property name. + protected void OnPropertyChanged([CallerMemberName] string? propertyName = null) => + PropertyChanged?.Invoke(this, new(propertyName)); +}