From 79f641418724873e893e2b5d474a104a06ca65b6 Mon Sep 17 00:00:00 2001 From: Michael Adelson Date: Wed, 14 Dec 2022 07:24:06 -0500 Subject: [PATCH 1/2] Ensure that OptionsCache only permits creating a single options instance per name. This incurs an extra delegate allocation, but only on instance creation. fix #79529 --- .../src/OptionsCache.cs | 2 +- .../OptionsMonitorTest.cs | 51 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs index 9be845ca4e8b71..51de2bbe4fc27d 100644 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs @@ -65,7 +65,7 @@ internal TOptions GetOrAdd(string? name, Func crea #if NET || NETSTANDARD2_1 return _cache.GetOrAdd( name ?? Options.DefaultName, - static (name, arg) => new Lazy(arg.createOptions(name, arg.factoryArgument)), (createOptions, factoryArgument)).Value; + static (name, arg) => new Lazy(() => arg.createOptions(name, arg.factoryArgument)), (createOptions, factoryArgument)).Value; #endif } diff --git a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs index 06fb1d9476350b..98fc61e441b4b8 100644 --- a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs +++ b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Linq; using System.Threading; +using System.Threading.Tasks; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Primitives; @@ -485,5 +486,55 @@ public void TestCurrentValueDoesNotAllocateOnceValueIsCached() Assert.Equal(0, GC.GetAllocatedBytesForCurrentThread() - initialBytes); } #endif + + /// + /// Replicates https://github.com/dotnet/runtime/issues/79529 + /// + [Fact] + public void InstantiatesOnlyOneOptionsInstance() + { + using AutoResetEvent @event = new(initialState: false); + + OptionsMonitor monitor = new( + // WaitHandleConfigureOptions makes instance configuration slow enough to force a race condition + new OptionsFactory(new[] { new WaitHandleConfigureOptions(@event) }, Enumerable.Empty>()), + Enumerable.Empty>(), + new OptionsCache()); + + using Barrier barrier = new(participantCount: 2); + Task[] instanceTasks = Enumerable.Range(0, 2) + .Select(_ => Task.Factory.StartNew( + () => + { + barrier.SignalAndWait(); + return monitor.Get("someName"); + }, + CancellationToken.None, + TaskCreationOptions.LongRunning, + TaskScheduler.Default) + ) + .ToArray(); + + // No tasks can finish yet; but give them a chance to run and get blocked on the WaitHandle + Assert.Equal(-1, Task.WaitAny(instanceTasks, TimeSpan.FromSeconds(0.01))); + + // 1 release should be sufficient to complete both tasks + @event.Set(); + Assert.True(Task.WaitAll(instanceTasks, TimeSpan.FromSeconds(30))); + Assert.Equal(1, instanceTasks.Select(t => t.Result).Distinct().Count()); + } + + private class WaitHandleConfigureOptions : IConfigureNamedOptions + { + private readonly WaitHandle _waitHandle; + + public WaitHandleConfigureOptions(WaitHandle waitHandle) + { + _waitHandle = waitHandle; + } + + void IConfigureNamedOptions.Configure(string? name, FakeOptions options) => _waitHandle.WaitOne(); + void IConfigureOptions.Configure(FakeOptions options) => _waitHandle.WaitOne(); + } } } From d779fb373465b608558b1edd742aaeae25ed2615 Mon Sep 17 00:00:00 2001 From: Michael Adelson Date: Fri, 16 Dec 2022 07:23:55 -0500 Subject: [PATCH 2/2] Ignore test on browser --- .../Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs index 98fc61e441b4b8..c04f48af0e8f9d 100644 --- a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs +++ b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs @@ -491,6 +491,7 @@ public void TestCurrentValueDoesNotAllocateOnceValueIsCached() /// Replicates https://github.com/dotnet/runtime/issues/79529 /// [Fact] + [SkipOnPlatform(TestPlatforms.Browser, "Synchronous wait is not supported on browser")] public void InstantiatesOnlyOneOptionsInstance() { using AutoResetEvent @event = new(initialState: false);