From f0cf935c4be54ce98409f1fb1bfdd6ab301d06b9 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Tue, 2 May 2023 08:41:43 -0500 Subject: [PATCH 1/3] Use test accessor for GlobalOptionService --- .../Options/GlobalOptionsTest.cs | 2 +- .../Portable/Options/GlobalOptionService.cs | 22 +++++++++++++++---- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/VisualStudio/IntegrationTest/New.IntegrationTests/Options/GlobalOptionsTest.cs b/src/VisualStudio/IntegrationTest/New.IntegrationTests/Options/GlobalOptionsTest.cs index bd81640b6d62f..8308a65b77010 100644 --- a/src/VisualStudio/IntegrationTest/New.IntegrationTests/Options/GlobalOptionsTest.cs +++ b/src/VisualStudio/IntegrationTest/New.IntegrationTests/Options/GlobalOptionsTest.cs @@ -91,7 +91,7 @@ storage is VisualStudioOptionStorage.RoamingProfileStorage && await vsSettingsPersister.PersistAsync(storage, key, differentValue); // make sure we fetch the value from the storage: - globalOptions.ClearCachedValues(); + globalOptions.GetTestAccessor().ClearCachedValues(); object? updatedValue; try diff --git a/src/Workspaces/Core/Portable/Options/GlobalOptionService.cs b/src/Workspaces/Core/Portable/Options/GlobalOptionService.cs index 4da63c3e59b5c..2f23daec22c72 100644 --- a/src/Workspaces/Core/Portable/Options/GlobalOptionService.cs +++ b/src/Workspaces/Core/Portable/Options/GlobalOptionService.cs @@ -251,12 +251,26 @@ private void RaiseOptionChangedEvent(List changedOptions } } - // for testing - public void ClearCachedValues() + internal TestAccessor GetTestAccessor() { - lock (_gate) + return new TestAccessor(this); + } + + internal readonly struct TestAccessor + { + private readonly GlobalOptionService _instance; + + internal TestAccessor(GlobalOptionService instance) { - _currentValues = ImmutableDictionary.Create(); + _instance = instance; + } + + public void ClearCachedValues() + { + lock (_instance._gate) + { + _instance._currentValues = ImmutableDictionary.Create(); + } } } } From a014df3a04df6d40e8a87151e8e3822d3a5935fd Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Tue, 2 May 2023 08:55:19 -0500 Subject: [PATCH 2/3] Use a weak event pattern for GlobalOptionService --- .../WorkspaceTests_EditorFeatures.cs | 4 +- ...erEventSources.OptionChangedEventSource.cs | 4 +- .../Test/Options/GlobalOptionsTests.cs | 10 ++- .../SolutionCrawler/WorkCoordinatorTests.cs | 4 +- .../EngineV2/DiagnosticIncrementalAnalyzer.cs | 4 +- .../Razor/RazorGlobalOptions.cs | 6 +- .../InheritanceMarginViewMargin.cs | 4 +- ...ctLanguageService`2.VsCodeWindowManager.cs | 4 +- .../StackTraceExplorerCommandHandler.cs | 4 +- .../AbstractDelayStartedService.cs | 4 +- .../Core/Def/Telemetry/FileLogger.cs | 2 +- .../OptionPages/ForceLowMemoryMode.cs | 2 +- .../Portable/Options/GlobalOptionService.cs | 77 ++++++++++++++++++- .../Portable/Options/IGlobalOptionService.cs | 10 ++- .../GlobalOptionServiceTests.cs | 4 +- 15 files changed, 107 insertions(+), 36 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/Workspaces/WorkspaceTests_EditorFeatures.cs b/src/EditorFeatures/CSharpTest/Workspaces/WorkspaceTests_EditorFeatures.cs index 85a4a1237543d..68cc8f692a267 100644 --- a/src/EditorFeatures/CSharpTest/Workspaces/WorkspaceTests_EditorFeatures.cs +++ b/src/EditorFeatures/CSharpTest/Workspaces/WorkspaceTests_EditorFeatures.cs @@ -1453,7 +1453,7 @@ public void TestOptionChangedHandlerInvokedAfterCurrentSolutionChanged() Assert.Equal(FormattingOptions2.IndentStyle.Smart, secondaryWorkspace.Options.GetOption(optionKey)); // Hook up the option changed event handler. - primaryWorkspace.GlobalOptions.OptionChanged += OptionService_OptionChanged; + primaryWorkspace.GlobalOptions.AddOptionChangedHandler(this, OptionService_OptionChanged); // Change workspace options through primary workspace primaryWorkspace.Options = primaryWorkspace.Options.WithChangedOption(optionKey, FormattingOptions2.IndentStyle.Block); @@ -1465,7 +1465,7 @@ public void TestOptionChangedHandlerInvokedAfterCurrentSolutionChanged() Assert.Equal(FormattingOptions2.IndentStyle.Block, primaryWorkspace.Options.GetOption(optionKey)); Assert.Equal(FormattingOptions2.IndentStyle.Block, secondaryWorkspace.Options.GetOption(optionKey)); - primaryWorkspace.GlobalOptions.OptionChanged -= OptionService_OptionChanged; + primaryWorkspace.GlobalOptions.RemoveOptionChangedHandler(this, OptionService_OptionChanged); return; void OptionService_OptionChanged(object sender, OptionChangedEventArgs e) diff --git a/src/EditorFeatures/Core/Shared/Tagging/EventSources/TaggerEventSources.OptionChangedEventSource.cs b/src/EditorFeatures/Core/Shared/Tagging/EventSources/TaggerEventSources.OptionChangedEventSource.cs index e0c63de3690d2..ece360640cae9 100644 --- a/src/EditorFeatures/Core/Shared/Tagging/EventSources/TaggerEventSources.OptionChangedEventSource.cs +++ b/src/EditorFeatures/Core/Shared/Tagging/EventSources/TaggerEventSources.OptionChangedEventSource.cs @@ -22,12 +22,12 @@ public GlobalOptionChangedEventSource(IGlobalOptionService globalOptions, IOptio public override void Connect() { - _globalOptions.OptionChanged += OnGlobalOptionChanged; + _globalOptions.AddOptionChangedHandler(this, OnGlobalOptionChanged); } public override void Disconnect() { - _globalOptions.OptionChanged -= OnGlobalOptionChanged; + _globalOptions.RemoveOptionChangedHandler(this, OnGlobalOptionChanged); } private void OnGlobalOptionChanged(object? sender, OptionChangedEventArgs e) diff --git a/src/EditorFeatures/Test/Options/GlobalOptionsTests.cs b/src/EditorFeatures/Test/Options/GlobalOptionsTests.cs index 57d9529184545..aab8ae3d6ac5e 100644 --- a/src/EditorFeatures/Test/Options/GlobalOptionsTests.cs +++ b/src/EditorFeatures/Test/Options/GlobalOptionsTests.cs @@ -77,10 +77,6 @@ public T GetOption(OptionKey2 optionKey) #region Unused -#pragma warning disable CS0067 - public event EventHandler? OptionChanged; -#pragma warning restore - public ImmutableArray GetOptions(ImmutableArray optionKeys) => throw new NotImplementedException(); @@ -99,6 +95,12 @@ public void SetGlobalOption(OptionKey2 optionKey, object? value) public bool SetGlobalOptions(ImmutableArray> options) => throw new NotImplementedException(); + public void AddOptionChangedHandler(object target, EventHandler handler) + => throw new NotImplementedException(); + + public void RemoveOptionChangedHandler(object target, EventHandler handler) + => throw new NotImplementedException(); + #endregion } diff --git a/src/EditorFeatures/Test/SolutionCrawler/WorkCoordinatorTests.cs b/src/EditorFeatures/Test/SolutionCrawler/WorkCoordinatorTests.cs index bbabd440a63f6..0c63fa89cdea4 100644 --- a/src/EditorFeatures/Test/SolutionCrawler/WorkCoordinatorTests.cs +++ b/src/EditorFeatures/Test/SolutionCrawler/WorkCoordinatorTests.cs @@ -1827,12 +1827,12 @@ public Analyzer(IGlobalOptionService globalOptions, bool waitForCancellation = f this.BlockEvent = new ManualResetEventSlim(initialState: false); this.RunningEvent = new ManualResetEventSlim(initialState: false); - _globalOptions.OptionChanged += GlobalOptionChanged; + _globalOptions.AddOptionChangedHandler(this, GlobalOptionChanged); } public void Shutdown() { - _globalOptions.OptionChanged -= GlobalOptionChanged; + _globalOptions.RemoveOptionChangedHandler(this, GlobalOptionChanged); } private void GlobalOptionChanged(object sender, OptionChangedEventArgs e) diff --git a/src/Features/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.cs b/src/Features/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.cs index 6a10fee2ec89a..a161c98f4e887 100644 --- a/src/Features/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.cs +++ b/src/Features/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.cs @@ -65,7 +65,7 @@ public DiagnosticIncrementalAnalyzer( _diagnosticAnalyzerRunner = new InProcOrRemoteHostAnalyzerRunner(analyzerInfoCache, analyzerService.Listener); - GlobalOptions.OptionChanged += OnGlobalOptionChanged; + GlobalOptions.AddOptionChangedHandler(this, OnGlobalOptionChanged); } private void OnGlobalOptionChanged(object? sender, OptionChangedEventArgs e) @@ -110,7 +110,7 @@ private void OnProjectAnalyzerReferenceChanged(object? sender, ProjectAnalyzerRe public void Shutdown() { - GlobalOptions.OptionChanged -= OnGlobalOptionChanged; + GlobalOptions.RemoveOptionChangedHandler(this, OnGlobalOptionChanged); var stateSets = _stateManager.GetAllStateSets(); diff --git a/src/Tools/ExternalAccess/Razor/RazorGlobalOptions.cs b/src/Tools/ExternalAccess/Razor/RazorGlobalOptions.cs index a0a19faf32edb..de2cb08311a7d 100644 --- a/src/Tools/ExternalAccess/Razor/RazorGlobalOptions.cs +++ b/src/Tools/ExternalAccess/Razor/RazorGlobalOptions.cs @@ -51,10 +51,6 @@ public static RazorGlobalOptions GetGlobalOptions(Workspace workspace) private sealed class TestGlobalOptionService : IGlobalOptionService { -#pragma warning disable CS0067 // Remove unused event - public event EventHandler? OptionChanged; -#pragma warning restore - public T GetOption(PerLanguageOption2 option, string languageName) => default!; @@ -67,6 +63,8 @@ public T GetOption(PerLanguageOption2 option, string languageName) public void SetGlobalOption(PerLanguageOption2 option, string language, T value) => throw new NotImplementedException(); public void SetGlobalOption(OptionKey2 optionKey, object? value) => throw new NotImplementedException(); public bool SetGlobalOptions(ImmutableArray> options) => throw new NotImplementedException(); + public void AddOptionChangedHandler(object target, EventHandler handler) => throw new NotImplementedException(); + public void RemoveOptionChangedHandler(object target, EventHandler handler) => throw new NotImplementedException(); bool IOptionsReader.TryGetOption(OptionKey2 optionKey, out T value) { diff --git a/src/VisualStudio/Core/Def/InheritanceMargin/InheritanceMarginViewMargin.cs b/src/VisualStudio/Core/Def/InheritanceMargin/InheritanceMarginViewMargin.cs index b3bf09f849749..ce8b3b7e9cff9 100644 --- a/src/VisualStudio/Core/Def/InheritanceMargin/InheritanceMarginViewMargin.cs +++ b/src/VisualStudio/Core/Def/InheritanceMargin/InheritanceMarginViewMargin.cs @@ -79,7 +79,7 @@ public InheritanceMarginViewMargin( _tagAggregator.BatchedTagsChanged += OnTagsChanged; _textView.LayoutChanged += OnLayoutChanged; _textView.ZoomLevelChanged += OnZoomLevelChanged; - _globalOptions.OptionChanged += OnGlobalOptionChanged; + _globalOptions.AddOptionChangedHandler(this, OnGlobalOptionChanged); UpdateMarginVisibility(); } @@ -93,7 +93,7 @@ void IDisposable.Dispose() _tagAggregator.BatchedTagsChanged -= OnTagsChanged; _textView.LayoutChanged -= OnLayoutChanged; _textView.ZoomLevelChanged -= OnZoomLevelChanged; - _globalOptions.OptionChanged -= OnGlobalOptionChanged; + _globalOptions.RemoveOptionChangedHandler(this, OnGlobalOptionChanged); _tagAggregator.Dispose(); ((IDisposable)_glyphManager).Dispose(); } diff --git a/src/VisualStudio/Core/Def/LanguageService/AbstractLanguageService`2.VsCodeWindowManager.cs b/src/VisualStudio/Core/Def/LanguageService/AbstractLanguageService`2.VsCodeWindowManager.cs index 09efac6659130..eaba2b1c93f22 100644 --- a/src/VisualStudio/Core/Def/LanguageService/AbstractLanguageService`2.VsCodeWindowManager.cs +++ b/src/VisualStudio/Core/Def/LanguageService/AbstractLanguageService`2.VsCodeWindowManager.cs @@ -54,7 +54,7 @@ public VsCodeWindowManager(TLanguageService languageService, IVsCodeWindow codeW _globalOptions = languageService.Package.ComponentModel.GetService(); _sink = ComEventSink.Advise(codeWindow, this); - _globalOptions.OptionChanged += GlobalOptionChanged; + _globalOptions.AddOptionChangedHandler(this, GlobalOptionChanged); } private void SetupView(IVsTextView view) @@ -227,7 +227,7 @@ public int OnNewView(IVsTextView view) public int RemoveAdornments() { _sink.Unadvise(); - _globalOptions.OptionChanged -= GlobalOptionChanged; + _globalOptions.RemoveOptionChangedHandler(this, GlobalOptionChanged); if (_codeWindow is IVsDropdownBarManager dropdownManager) { diff --git a/src/VisualStudio/Core/Def/StackTraceExplorer/StackTraceExplorerCommandHandler.cs b/src/VisualStudio/Core/Def/StackTraceExplorer/StackTraceExplorerCommandHandler.cs index a82a8a194a93c..e8446589678b4 100644 --- a/src/VisualStudio/Core/Def/StackTraceExplorer/StackTraceExplorerCommandHandler.cs +++ b/src/VisualStudio/Core/Def/StackTraceExplorer/StackTraceExplorerCommandHandler.cs @@ -30,7 +30,7 @@ private StackTraceExplorerCommandHandler(RoslynPackage package) _threadingContext = package.ComponentModel.GetService(); _globalOptions = package.ComponentModel.GetService(); - _globalOptions.OptionChanged += GlobalOptionChanged; + _globalOptions.AddOptionChangedHandler(this, GlobalOptionChanged); var enabled = _globalOptions.GetOption(StackTraceExplorerOptionsStorage.OpenOnFocus); if (enabled) @@ -76,7 +76,7 @@ public void Dispose() { UnadviseBroadcastMessages(); - _globalOptions.OptionChanged -= GlobalOptionChanged; + _globalOptions.RemoveOptionChangedHandler(this, GlobalOptionChanged); } private void AdviseBroadcastMessages() diff --git a/src/VisualStudio/Core/Def/SymbolSearch/AbstractDelayStartedService.cs b/src/VisualStudio/Core/Def/SymbolSearch/AbstractDelayStartedService.cs index 402ea6b1f09f6..d7efe102f8247 100644 --- a/src/VisualStudio/Core/Def/SymbolSearch/AbstractDelayStartedService.cs +++ b/src/VisualStudio/Core/Def/SymbolSearch/AbstractDelayStartedService.cs @@ -68,7 +68,7 @@ protected AbstractDelayStartedService( ProcessOptionChangesAsync, listenerProvider.GetListener(FeatureAttribute.Workspace), this.DisposalToken); - _globalOptions.OptionChanged += OnOptionChanged; + _globalOptions.AddOptionChangedHandler(this, OnOptionChanged); } protected abstract Task EnableServiceAsync(CancellationToken cancellationToken); @@ -100,7 +100,7 @@ private async ValueTask ProcessOptionChangesAsync(CancellationToken arg) // We were enabled for some language. Kick off the work for this service now. Since we're now enabled, we // no longer need to listen for option changes. _enabled = true; - _globalOptions.OptionChanged -= OnOptionChanged; + _globalOptions.RemoveOptionChangedHandler(this, OnOptionChanged); // Don't both kicking off delay-started services prior to the actual workspace being fully loaded. We don't // want them using CPU/memory in the BG while we're loading things for the user. diff --git a/src/VisualStudio/Core/Def/Telemetry/FileLogger.cs b/src/VisualStudio/Core/Def/Telemetry/FileLogger.cs index 3e2252d890c80..3c25dcb64e9f6 100644 --- a/src/VisualStudio/Core/Def/Telemetry/FileLogger.cs +++ b/src/VisualStudio/Core/Def/Telemetry/FileLogger.cs @@ -39,7 +39,7 @@ public FileLogger(IGlobalOptionService globalOptions, string logFilePath) _buffer = new(); _taskQueue = new(AsynchronousOperationListenerProvider.NullListener, TaskScheduler.Default); _enabled = globalOptions.GetOption(VisualStudioLoggingOptionsStorage.EnableFileLoggingForDiagnostics); - globalOptions.OptionChanged += OptionService_OptionChanged; + globalOptions.AddOptionChangedHandler(this, OptionService_OptionChanged); } public FileLogger(IGlobalOptionService optionService) diff --git a/src/VisualStudio/VisualStudioDiagnosticsToolWindow/OptionPages/ForceLowMemoryMode.cs b/src/VisualStudio/VisualStudioDiagnosticsToolWindow/OptionPages/ForceLowMemoryMode.cs index f132925996f66..f03278257e88c 100644 --- a/src/VisualStudio/VisualStudioDiagnosticsToolWindow/OptionPages/ForceLowMemoryMode.cs +++ b/src/VisualStudio/VisualStudioDiagnosticsToolWindow/OptionPages/ForceLowMemoryMode.cs @@ -22,7 +22,7 @@ public ForceLowMemoryMode(IGlobalOptionService globalOptions) { _globalOptions = globalOptions; - globalOptions.OptionChanged += Options_OptionChanged; + globalOptions.AddOptionChangedHandler(this, Options_OptionChanged); RefreshFromSettings(); } diff --git a/src/Workspaces/Core/Portable/Options/GlobalOptionService.cs b/src/Workspaces/Core/Portable/Options/GlobalOptionService.cs index 2f23daec22c72..c3c42fb49039e 100644 --- a/src/Workspaces/Core/Portable/Options/GlobalOptionService.cs +++ b/src/Workspaces/Core/Portable/Options/GlobalOptionService.cs @@ -7,6 +7,7 @@ using System.Collections.Immutable; using System.Composition; using System.Diagnostics; +using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Host.Mef; @@ -29,9 +30,15 @@ internal sealed class GlobalOptionService : IGlobalOptionService private ImmutableArray _lazyOptionPersisters; private ImmutableDictionary _currentValues; + /// + /// Each registered event handler has the lifetime of an associated owning object. This table ensures the weak + /// references to the event handlers are not cleaned up while the owning object is still alive. + /// + private readonly ConditionalWeakTable> _keepAliveTable = new(); + #endregion - public event EventHandler? OptionChanged; + private ImmutableList>> _weakHandlers = ImmutableList>>.Empty; [ImportingConstructor] [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] @@ -236,17 +243,79 @@ public bool RefreshOption(OptionKey2 optionKey, object? newValue) return true; } + public void AddOptionChangedHandler(object target, EventHandler handler) + { + lock (_gate) + { + if (_keepAliveTable.TryGetValue(target, out var existingHandler)) + { + var newHandler = existingHandler + handler; +#if NET6_0_OR_GREATER + _keepAliveTable.AddOrUpdate(target, newHandler); +#else + _keepAliveTable.Remove(target); + _keepAliveTable.Add(target, newHandler); +#endif + } + } + + ImmutableInterlocked.Update( + ref _weakHandlers, + static (weakHandlers, handler) => + { + return weakHandlers + .RemoveAll(WeakReferenceExtensions.IsNull) + .Add(new WeakReference>(handler)); + }, + handler); + } + + public void RemoveOptionChangedHandler(object target, EventHandler handler) + { + lock (_gate) + { + if (_keepAliveTable.TryGetValue(target, out var existingHandler)) + { + var newHandler = existingHandler - handler; + if (newHandler is null) + { + _keepAliveTable.Remove(target); + } + else + { +#if NET6_0_OR_GREATER + _keepAliveTable.AddOrUpdate(target, newHandler); +#else + _keepAliveTable.Remove(target); + _keepAliveTable.Add(target, newHandler); +#endif + } + } + } + + ImmutableInterlocked.Update( + ref _weakHandlers, + static (weakHandlers, handler) => + { + return weakHandlers + .RemoveAll(weakHandler => !weakHandler.TryGetTarget(out var target) || (target - handler) is null); + }, + handler); + } + private void RaiseOptionChangedEvent(List changedOptions) { Debug.Assert(changedOptions.Count > 0); // Raise option changed events. - var optionChanged = OptionChanged; - if (optionChanged != null) + foreach (var weakHandler in _weakHandlers) { + if (!weakHandler.TryGetTarget(out var handler)) + continue; + foreach (var changedOption in changedOptions) { - optionChanged(this, changedOption); + handler(this, changedOption); } } } diff --git a/src/Workspaces/Core/Portable/Options/IGlobalOptionService.cs b/src/Workspaces/Core/Portable/Options/IGlobalOptionService.cs index ab71f0ec22878..5a55e1de076dd 100644 --- a/src/Workspaces/Core/Portable/Options/IGlobalOptionService.cs +++ b/src/Workspaces/Core/Portable/Options/IGlobalOptionService.cs @@ -43,21 +43,19 @@ internal interface IGlobalOptionService : IOptionsReader /// Sets and persists the value of a global option. /// Sets the value of a global option. /// Invokes registered option persisters. - /// Triggers . + /// Triggers option changed event for handlers registered with . /// void SetGlobalOption(OptionKey2 optionKey, object? value); /// /// Atomically sets the values of specified global options. The option values are persisted. - /// Triggers . + /// Triggers option changed event for handlers registered with . /// /// /// Returns true if any option changed its value stored in the global options. /// bool SetGlobalOptions(ImmutableArray> options); - event EventHandler? OptionChanged; - /// /// Refreshes the stored value of an option. This should only be called from persisters. /// Does not persist the new option value. @@ -66,5 +64,9 @@ internal interface IGlobalOptionService : IOptionsReader /// Returns true if the option changed its value stored in the global options. /// bool RefreshOption(OptionKey2 optionKey, object? newValue); + + void AddOptionChangedHandler(object target, EventHandler handler); + + void RemoveOptionChangedHandler(object target, EventHandler handler); } } diff --git a/src/Workspaces/CoreTest/WorkspaceServiceTests/GlobalOptionServiceTests.cs b/src/Workspaces/CoreTest/WorkspaceServiceTests/GlobalOptionServiceTests.cs index 7ba8ea0566bdc..44a71e9ad489c 100644 --- a/src/Workspaces/CoreTest/WorkspaceServiceTests/GlobalOptionServiceTests.cs +++ b/src/Workspaces/CoreTest/WorkspaceServiceTests/GlobalOptionServiceTests.cs @@ -175,7 +175,7 @@ public void GlobalOptions() var changedOptions = new List(); var handler = new EventHandler((_, e) => changedOptions.Add(e)); - globalOptions.OptionChanged += handler; + globalOptions.AddOptionChangedHandler(this, handler); var values = globalOptions.GetOptions(ImmutableArray.Create(new OptionKey2(option1), new OptionKey2(option2))); Assert.Equal(1, values[0]); @@ -201,7 +201,7 @@ public void GlobalOptions() Assert.Equal(6, globalOptions.GetOption(option2)); Assert.Equal(3, globalOptions.GetOption(option3)); - globalOptions.OptionChanged -= handler; + globalOptions.RemoveOptionChangedHandler(this, handler); } [Fact] From 1142fb72ef3cc2eea5c13bb458966f3583f425df Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Tue, 2 May 2023 20:18:19 -0500 Subject: [PATCH 3/3] Extract and use WeakEvent --- .../Portable/Options/GlobalOptionService.cs | 74 +--------- .../Core/CompilerExtensions.projitems | 1 + .../Compiler/Core/Utilities/WeakEvent`1.cs | 131 ++++++++++++++++++ 3 files changed, 137 insertions(+), 69 deletions(-) create mode 100644 src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/WeakEvent`1.cs diff --git a/src/Workspaces/Core/Portable/Options/GlobalOptionService.cs b/src/Workspaces/Core/Portable/Options/GlobalOptionService.cs index c3c42fb49039e..6d8d8a0e1c353 100644 --- a/src/Workspaces/Core/Portable/Options/GlobalOptionService.cs +++ b/src/Workspaces/Core/Portable/Options/GlobalOptionService.cs @@ -7,7 +7,6 @@ using System.Collections.Immutable; using System.Composition; using System.Diagnostics; -using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Host.Mef; @@ -30,15 +29,9 @@ internal sealed class GlobalOptionService : IGlobalOptionService private ImmutableArray _lazyOptionPersisters; private ImmutableDictionary _currentValues; - /// - /// Each registered event handler has the lifetime of an associated owning object. This table ensures the weak - /// references to the event handlers are not cleaned up while the owning object is still alive. - /// - private readonly ConditionalWeakTable> _keepAliveTable = new(); - #endregion - private ImmutableList>> _weakHandlers = ImmutableList>>.Empty; + private readonly WeakEvent _optionChanged = new(); [ImportingConstructor] [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] @@ -245,78 +238,21 @@ public bool RefreshOption(OptionKey2 optionKey, object? newValue) public void AddOptionChangedHandler(object target, EventHandler handler) { - lock (_gate) - { - if (_keepAliveTable.TryGetValue(target, out var existingHandler)) - { - var newHandler = existingHandler + handler; -#if NET6_0_OR_GREATER - _keepAliveTable.AddOrUpdate(target, newHandler); -#else - _keepAliveTable.Remove(target); - _keepAliveTable.Add(target, newHandler); -#endif - } - } - - ImmutableInterlocked.Update( - ref _weakHandlers, - static (weakHandlers, handler) => - { - return weakHandlers - .RemoveAll(WeakReferenceExtensions.IsNull) - .Add(new WeakReference>(handler)); - }, - handler); + _optionChanged.AddHandler(target, handler); } public void RemoveOptionChangedHandler(object target, EventHandler handler) { - lock (_gate) - { - if (_keepAliveTable.TryGetValue(target, out var existingHandler)) - { - var newHandler = existingHandler - handler; - if (newHandler is null) - { - _keepAliveTable.Remove(target); - } - else - { -#if NET6_0_OR_GREATER - _keepAliveTable.AddOrUpdate(target, newHandler); -#else - _keepAliveTable.Remove(target); - _keepAliveTable.Add(target, newHandler); -#endif - } - } - } - - ImmutableInterlocked.Update( - ref _weakHandlers, - static (weakHandlers, handler) => - { - return weakHandlers - .RemoveAll(weakHandler => !weakHandler.TryGetTarget(out var target) || (target - handler) is null); - }, - handler); + _optionChanged.RemoveHandler(target, handler); } private void RaiseOptionChangedEvent(List changedOptions) { Debug.Assert(changedOptions.Count > 0); - // Raise option changed events. - foreach (var weakHandler in _weakHandlers) + foreach (var changedOption in changedOptions) { - if (!weakHandler.TryGetTarget(out var handler)) - continue; - - foreach (var changedOption in changedOptions) - { - handler(this, changedOption); - } + _optionChanged.RaiseEvent(this, changedOption); } } diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CompilerExtensions.projitems b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CompilerExtensions.projitems index 1c053010c9dca..70a91d98dcd3c 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CompilerExtensions.projitems +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CompilerExtensions.projitems @@ -538,6 +538,7 @@ + diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/WeakEvent`1.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/WeakEvent`1.cs new file mode 100644 index 0000000000000..07e68114d095a --- /dev/null +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/WeakEvent`1.cs @@ -0,0 +1,131 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Immutable; +using System.Runtime.CompilerServices; + +namespace Roslyn.Utilities; + +internal sealed class WeakEvent +{ + /// + /// Each registered event handler has the lifetime of an associated owning object. This table ensures the weak + /// references to the event handlers are not cleaned up while the owning object is still alive. + /// + /// + /// Access to this table is guarded by itself. For example: + /// + /// () + /// { + /// // Read or write to . + /// } + /// + /// + private readonly ConditionalWeakTable> _keepAliveTable = new(); + + private ImmutableList>> _weakHandlers = ImmutableList>>.Empty; + + public void AddHandler(object target, EventHandler handler) + { + lock (_keepAliveTable) + { + if (_keepAliveTable.TryGetValue(target, out var existingHandler)) + { + // Combine the delegates and store the combination in the keep-alive table + var newHandler = existingHandler + handler; +#if NET6_0_OR_GREATER + _keepAliveTable.AddOrUpdate(target, newHandler); +#else + _keepAliveTable.Remove(target); + _keepAliveTable.Add(target, newHandler); +#endif + } + else + { + // This is the first handler for this target, so just store it in the table directly + _keepAliveTable.Add(target, handler); + } + } + + ImmutableInterlocked.Update( + ref _weakHandlers, + static (weakHandlers, handler) => + { + // Add a weak reference to the handler to the list of delegates to invoke for this event. During the + // mutation, also remove any handlers that were collected without being removed (and are now null). + return weakHandlers + .RemoveAll(WeakReferenceExtensions.IsNull) + .Add(new WeakReference>(handler)); + }, + handler); + } + + public void RemoveHandler(object target, EventHandler handler) + { + lock (_keepAliveTable) + { + if (_keepAliveTable.TryGetValue(target, out var existingHandler)) + { + var newHandler = existingHandler - handler; + if (newHandler is null) + { + _keepAliveTable.Remove(target); + } + else + { +#if NET6_0_OR_GREATER + _keepAliveTable.AddOrUpdate(target, newHandler); +#else + _keepAliveTable.Remove(target); + _keepAliveTable.Add(target, newHandler); +#endif + } + } + } + + ImmutableInterlocked.Update( + ref _weakHandlers, + static (weakHandlers, handler) => + { + // Remove any references to the handler from the list of delegates to invoke for this event. During the + // mutation, also remove any handlers that were collected without being removed (and are now null). + var builder = weakHandlers.ToBuilder(); + for (var i = weakHandlers.Count - 1; i >= 0; i--) + { + var weakHandler = weakHandlers[i]; + if (!weakHandler.TryGetTarget(out var target)) + { + // This handler was collected + builder.RemoveAt(i); + continue; + } + + var updatedHandler = target - handler; + if (updatedHandler is null) + { + builder.RemoveAt(i); + } + else if (updatedHandler != target) + { + builder[i].SetTarget(updatedHandler); + } + } + + return builder.ToImmutable(); + }, + handler); + } + + public void RaiseEvent(object? sender, TEventArgs e) + { + foreach (var weakHandler in _weakHandlers) + { + if (!weakHandler.TryGetTarget(out var handler)) + continue; + + handler(sender, e); + } + } +}