Async hook up change in options (#9809)

Unified settings is an async package, which means any service it provides has to be loaded asynchronously or incurr a JTF.Run. Since MEF constructors are free threaded, this can cause a block. Use async hook up of change events to mitigate any threading concerns

src/Razor/src/Microsoft.VisualStudio.Editor.Razor/ClientSettingsManager.cs

@@ -6,6 +6,7 @@ using System.Collections.Generic;
 using System.ComponentModel.Composition;
 using Microsoft.CodeAnalysis.ExternalAccess.Razor;
 using Microsoft.CodeAnalysis.Razor.Editor;
+using Microsoft.VisualStudio.Threading;
 
 namespace Microsoft.VisualStudio.Editor.Razor;
 
@@ -50,7 +51,7 @@ internal class ClientSettingsManager : EditorSettingsManager, IClientSettingsMan
         if (_advancedSettingsStorage is not null)
         {
             Update(_advancedSettingsStorage.GetAdvancedSettings());
-            _advancedSettingsStorage.Changed += AdvancedSettingsChanged;
+            _advancedSettingsStorage.OnChangedAsync(Update).Forget();
         }
     }
 
@@ -102,8 +103,6 @@ internal class ClientSettingsManager : EditorSettingsManager, IClientSettingsMan
         }
     }
 
-    private void AdvancedSettingsChanged(object sender, ClientAdvancedSettingsChangedEventArgs e) => Update(e.Settings);
-
     private void UpdateSettings_NoLock(ClientSettings settings)
     {
         if (!_settings.Equals(settings))

src/Razor/src/Microsoft.VisualStudio.Editor.Razor/IClientSettingsManager.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT license. See License.txt in the project root for license information.
 
 using System;
+using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.Razor.Editor;
 
 namespace Microsoft.VisualStudio.Editor.Razor;
@@ -23,7 +24,7 @@ internal interface IAdvancedSettingsStorage : IDisposable
 {
     ClientAdvancedSettings GetAdvancedSettings();
 
-    event EventHandler<ClientAdvancedSettingsChangedEventArgs>? Changed;
+    Task OnChangedAsync(Action<ClientAdvancedSettings> changed);
 }
 
 internal class ClientAdvancedSettingsChangedEventArgs(ClientAdvancedSettings advancedSettings) : EventArgs

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/Options/OptionsStorage.cs

@@ -2,29 +2,32 @@
 // Licensed under the MIT license. See License.txt in the project root for license information.
 
 using System;
-using System.Composition;
+using System.ComponentModel.Composition;
+using System.Linq;
+using System.Threading.Tasks;
 using Microsoft.AspNetCore.Razor.Telemetry;
 using Microsoft.CodeAnalysis.Razor.Editor;
 using Microsoft.Extensions.Logging;
+using Microsoft.Internal.VisualStudio.Shell.Interop;
 using Microsoft.VisualStudio.Editor.Razor;
 using Microsoft.VisualStudio.Settings;
 using Microsoft.VisualStudio.Shell;
+using Microsoft.VisualStudio.Shell.Interop;
 using Microsoft.VisualStudio.Shell.Settings;
-using Microsoft.Internal.VisualStudio.Shell.Interop;
+using Microsoft.VisualStudio.Threading;
 using Microsoft.VisualStudio.Utilities.UnifiedSettings;
-using System.Linq;
 
 namespace Microsoft.VisualStudio.LanguageServerClient.Razor.Options;
 
-[Shared]
 [Export(typeof(OptionsStorage))]
 [Export(typeof(IAdvancedSettingsStorage))]
 internal class OptionsStorage : IAdvancedSettingsStorage
 {
     private readonly WritableSettingsStore _writableSettingsStore;
     private readonly Lazy<ITelemetryReporter> _telemetryReporter;
-    private readonly ISettingsReader _unifiedSettingsReader;
-    private readonly IDisposable _unifiedSettingsSubscription;
+    private readonly JoinableTask _initializeTask;
+    private ISettingsReader? _unifiedSettingsReader;
+    private IDisposable? _unifiedSettingsSubscription;
 
     public bool FormatOnType
     {
@@ -69,20 +72,34 @@ internal class OptionsStorage : IAdvancedSettingsStorage
     }
 
     [ImportingConstructor]
-    public OptionsStorage(SVsServiceProvider vsServiceProvider, Lazy<ITelemetryReporter> telemetryReporter)
+    public OptionsStorage(
+        SVsServiceProvider synchronousServiceProvider,
+        [Import(typeof(SAsyncServiceProvider))] IAsyncServiceProvider serviceProvider,
+        Lazy<ITelemetryReporter> telemetryReporter,
+        JoinableTaskContext joinableTaskContext)
     {
-        var shellSettingsManager = new ShellSettingsManager(vsServiceProvider);
+        var shellSettingsManager = new ShellSettingsManager(synchronousServiceProvider);
         _writableSettingsStore = shellSettingsManager.GetWritableSettingsStore(SettingsScope.UserSettings);
 
         _writableSettingsStore.CreateCollection(SettingsNames.LegacyCollection);
         _telemetryReporter = telemetryReporter;
 
-        var settingsManager = vsServiceProvider.GetService<SVsUnifiedSettingsManager, Utilities.UnifiedSettings.ISettingsManager>();
-        _unifiedSettingsReader = settingsManager.GetReader();
+        _initializeTask = joinableTaskContext.Factory.RunAsync(async () =>
+        {
+            var unifiedSettingsManager = await serviceProvider.GetServiceAsync<SVsUnifiedSettingsManager, Utilities.UnifiedSettings.ISettingsManager>();
+            _unifiedSettingsReader = unifiedSettingsManager.GetReader();
             _unifiedSettingsSubscription = _unifiedSettingsReader.SubscribeToChanges(OnUnifiedSettingsChanged, SettingsNames.AllSettings.Select(s => s.UnifiedName).ToArray());
+        });
     }
 
-    public event EventHandler<ClientAdvancedSettingsChangedEventArgs>? Changed;
+    public async Task OnChangedAsync(Action<ClientAdvancedSettings> changed)
+    {
+        await _initializeTask.JoinAsync();
+
+        _changed += (_, args) => changed(args.Settings);
+    }
+
+    private EventHandler<ClientAdvancedSettingsChangedEventArgs>? _changed;
 
     public ClientAdvancedSettings GetAdvancedSettings() => new(FormatOnType, AutoClosingTags, AutoInsertAttributeQuotes, ColorBackground, CommitElementsWithSpace, Snippets, LogLevel);
 
@@ -124,7 +141,8 @@ internal class OptionsStorage : IAdvancedSettingsStorage
 
     private void NotifyChange()
     {
-        Changed?.Invoke(this, new ClientAdvancedSettingsChangedEventArgs(GetAdvancedSettings()));
+        _initializeTask.Join();
+        _changed?.Invoke(this, new ClientAdvancedSettingsChangedEventArgs(GetAdvancedSettings()));
     }
 
     private void OnUnifiedSettingsChanged(SettingsUpdate update)
@@ -134,6 +152,6 @@ internal class OptionsStorage : IAdvancedSettingsStorage
 
     public void Dispose()
     {
-        _unifiedSettingsSubscription.Dispose();
+        _unifiedSettingsSubscription?.Dispose();
     }
 }

src/Razor/src/Microsoft.VisualStudio.RazorExtension/Snippets/SnippetService.cs

@@ -61,16 +61,16 @@ internal class SnippetService
         _serviceProvider = serviceProvider;
         _snippetCache = snippetCache;
         _advancedSettingsStorage = advancedSettingsStorage;
-        _advancedSettingsStorage.Changed += (s, e) =>
-        {
-            _joinableTaskFactory.RunAsync(PopulateAsync).FileAndForget("SnippetService_Populate");
-        };
-
         _joinableTaskFactory.RunAsync(InitializeAsync).FileAndForget("SnippetService_Initialize");
     }
 
     private async Task InitializeAsync()
     {
+        await _advancedSettingsStorage.OnChangedAsync(_ =>
+        {
+            PopulateAsync().FileAndForget("SnippetService_Populate");
+        }).ConfigureAwait(false);
+
         await _joinableTaskFactory.SwitchToMainThreadAsync();
         var textManager = (IVsTextManager2?)await _serviceProvider.GetServiceAsync(typeof(SVsTextManager)).ConfigureAwait(true);
         if (textManager is null)

src/Razor/test/Microsoft.VisualStudio.Editor.Razor.Test/ClientSettingsManagerTest.cs

@@ -5,6 +5,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Threading.Tasks;
 using Microsoft.AspNetCore.Razor.Test.Common.Editor;
 using Microsoft.CodeAnalysis.Razor.Editor;
 using Xunit;
@@ -142,10 +143,6 @@ public class ClientSettingsManagerTest(ITestOutputHelper testOutput) : ProjectSn
             _settings = settings;
         }
 
-#pragma warning disable CS0067 // The event 'ClientSettingsManagerTest.AdvancedSettingsStorage.Changed' is never used
-        public event EventHandler<ClientAdvancedSettingsChangedEventArgs> Changed;
-#pragma warning restore CS0067 // The event 'ClientSettingsManagerTest.AdvancedSettingsStorage.Changed' is never used
-
         public ClientAdvancedSettings GetAdvancedSettings()
         {
             return _settings;
@@ -154,5 +151,10 @@ public class ClientSettingsManagerTest(ITestOutputHelper testOutput) : ProjectSn
         public void Dispose()
         {
         }
+
+        public Task OnChangedAsync(Action<ClientAdvancedSettings> changed)
+        {
+            return Task.CompletedTask;
+        }
     }
 }