Make GlobalOptionService initialization synchronous. (#77823)

As Tomas pointed out in an earlier PR, all the hoops that are jumped through in GlobalOptionService to make it's initialization asynchronous aren't really necessary if the services obtained in VisualStudioOptionPersisterProvider were obtained synchronously. Doing just that allows GlobalOptionService to be quite a bit simpler and to no longer require a potential JTF.Run call when getting the first option.

The concern one could have with obtaining these services synchronously would be whether they were expensive to get and whether the first call would come through on the main thread. The first call to get an option comes through on a background thread (I've seen it come from either the background processing in after package load, or from the workspace initialization call roslyn gets from project system). Additionally, the measurements that I've taken of obtaining those services haven't shown those as expensive to obtain, so this does seem like a better approach than earlier attempts.

Author: ToddGrun

src/VisualStudio/Core/Def/Options/VisualStudioOptionPersisterProvider.cs

@@ -6,10 +6,7 @@
 using System.Collections.Generic;
 using System.Collections.Immutable;
 using System.ComponentModel.Composition;
-using System.Threading;
-using System.Threading.Tasks;
 using Microsoft.CodeAnalysis;
-using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
 using Microsoft.CodeAnalysis.ErrorReporting;
 using Microsoft.CodeAnalysis.Host.Mef;
 using Microsoft.CodeAnalysis.Options;
@@ -24,46 +21,41 @@ namespace Microsoft.VisualStudio.LanguageServices.Options;
 [Export(typeof(VisualStudioOptionPersisterProvider))]
 internal sealed class VisualStudioOptionPersisterProvider : IOptionPersisterProvider
 {
-    private readonly IAsyncServiceProvider _serviceProvider;
-    private readonly ILegacyGlobalOptionService _legacyGlobalOptions;
+    private readonly IServiceProvider _serviceProvider;
+    private readonly Lazy<ILegacyGlobalOptionService> _legacyGlobalOptions;
 
     // maps config name to a read fallback:
     private readonly ImmutableDictionary<string, Lazy<IVisualStudioStorageReadFallback, OptionNameMetadata>> _readFallbacks;
 
-    // Use vs-threading's JTF-aware AsyncLazy<T>. Ensure only one persister instance is created (even in the face of
-    // parallel requests for the value) because the constructor registers global event handler callbacks.
-    private readonly Threading.AsyncLazy<IOptionPersister> _lazyPersister;
+    // Ensure only one persister instance is created (even in the face of parallel requests for the value)
+    // because the constructor registers global event handler callbacks.
+    private readonly Lazy<IOptionPersister> _lazyPersister;
 
     [ImportingConstructor]
     [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
     public VisualStudioOptionPersisterProvider(
-        [Import(typeof(SAsyncServiceProvider))] IAsyncServiceProvider serviceProvider,
+        [Import(typeof(SVsServiceProvider))] IServiceProvider serviceProvider,
         [ImportMany] IEnumerable<Lazy<IVisualStudioStorageReadFallback, OptionNameMetadata>> readFallbacks,
-        IThreadingContext threadingContext,
-        ILegacyGlobalOptionService legacyGlobalOptions)
+        Lazy<ILegacyGlobalOptionService> legacyGlobalOptions)
     {
         _serviceProvider = serviceProvider;
         _legacyGlobalOptions = legacyGlobalOptions;
         _readFallbacks = readFallbacks.ToImmutableDictionary(item => item.Metadata.ConfigName, item => item);
-        _lazyPersister = new Threading.AsyncLazy<IOptionPersister>(() => CreatePersisterAsync(threadingContext.DisposalToken), threadingContext.JoinableTaskFactory);
+        _lazyPersister = new Lazy<IOptionPersister>(() => CreatePersister());
     }
 
-    public ValueTask<IOptionPersister> GetOrCreatePersisterAsync(CancellationToken cancellationToken)
-        => new(_lazyPersister.GetValueAsync(cancellationToken));
+    public IOptionPersister GetOrCreatePersister()
+        => _lazyPersister.Value;
 
-    private async Task<IOptionPersister> CreatePersisterAsync(CancellationToken cancellationToken)
+    private IOptionPersister CreatePersister()
     {
-        // Obtain services before creating instances. This avoids state corruption in the event cancellation is
-        // requested (some of the constructors register event handlers that could leak if cancellation occurred
-        // in the middle of construction).
-        var settingsManager = await GetFreeThreadedServiceAsync<SVsSettingsPersistenceManager, ISettingsManager>().ConfigureAwait(false);
+        var settingsManager = GetFreeThreadedService<SVsSettingsPersistenceManager, ISettingsManager>();
         Assumes.Present(settingsManager);
-        var localRegistry = await GetFreeThreadedServiceAsync<SLocalRegistry, ILocalRegistry4>().ConfigureAwait(false);
+
+        var localRegistry = GetFreeThreadedService<SLocalRegistry, ILocalRegistry4>();
         Assumes.Present(localRegistry);
-        var featureFlags = await GetFreeThreadedServiceAsync<SVsFeatureFlags, IVsFeatureFlags>().ConfigureAwait(false);
 
-        // Cancellation is not allowed after this point
-        cancellationToken = CancellationToken.None;
+        var featureFlags = GetFreeThreadedService<SVsFeatureFlags, IVsFeatureFlags>();
 
         return new VisualStudioOptionPersister(
             new VisualStudioSettingsOptionPersister(RefreshOption, _readFallbacks, settingsManager),
@@ -73,23 +65,23 @@ private async Task<IOptionPersister> CreatePersisterAsync(CancellationToken canc
 
     private void RefreshOption(OptionKey2 optionKey, object? newValue)
     {
-        if (_legacyGlobalOptions.GlobalOptions.RefreshOption(optionKey, newValue))
+        if (_legacyGlobalOptions.Value.GlobalOptions.RefreshOption(optionKey, newValue))
         {
             // We may be updating the values of internally defined public options.
             // Update solution snapshots of all workspaces to reflect the new values.
-            _legacyGlobalOptions.UpdateRegisteredWorkspaces();
+            _legacyGlobalOptions.Value.UpdateRegisteredWorkspaces();
         }
     }
 
     /// <summary>
     /// Returns a service without doing a transition to the UI thread to cast the service to the interface type. This should only be called for services that are
     /// well-understood to be castable off the UI thread, either because they are managed or free-threaded COM.
     /// </summary>
-    private async ValueTask<I?> GetFreeThreadedServiceAsync<T, I>() where I : class
+    private I? GetFreeThreadedService<T, I>() where I : class
     {
         try
         {
-            return (I?)await _serviceProvider.GetServiceAsync(typeof(T)).ConfigureAwait(false);
+            return (I?)_serviceProvider.GetService(typeof(T));
         }
         catch (Exception e) when (FatalError.ReportAndPropagate(e))
         {

src/VisualStudio/Core/Def/RoslynPackage.cs

@@ -3,7 +3,6 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
-using System.Collections.Immutable;
 using System.ComponentModel.Design;
 using System.Runtime.InteropServices;
 using System.Threading;
@@ -18,12 +17,9 @@
 using Microsoft.CodeAnalysis.Notification;
 using Microsoft.CodeAnalysis.Options;
 using Microsoft.CodeAnalysis.Remote.ProjectSystem;
-using Microsoft.CodeAnalysis.Shared.TestHooks;
-using Microsoft.VisualStudio.ComponentModelHost;
 using Microsoft.VisualStudio.LanguageServices.EditorConfigSettings;
 using Microsoft.VisualStudio.LanguageServices.Implementation.Diagnostics;
 using Microsoft.VisualStudio.LanguageServices.Implementation.LanguageService;
-using Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem;
 using Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.RuleSets;
 using Microsoft.VisualStudio.LanguageServices.Implementation.Suppression;
 using Microsoft.VisualStudio.LanguageServices.Implementation.SyncNamespaces;
@@ -106,6 +102,9 @@ protected override void RegisterOnAfterPackageLoadedAsyncWork(PackageLoadTasks a
 
         Task OnAfterPackageLoadedBackgroundThreadAsync(PackageLoadTasks afterPackageLoadedTasks, CancellationToken cancellationToken)
         {
+            // Ensure the options persisters are loaded since we have to fetch options from the shell
+            _ = ComponentModel.GetService<IGlobalOptionService>();
+
             var colorSchemeApplier = ComponentModel.GetService<ColorSchemeApplier>();
             colorSchemeApplier.RegisterInitializationWork(afterPackageLoadedTasks);
 
@@ -115,9 +114,6 @@ Task OnAfterPackageLoadedBackgroundThreadAsync(PackageLoadTasks afterPackageLoad
             _solutionEventMonitor = new SolutionEventMonitor(globalNotificationService);
             TrackBulkFileOperations(globalNotificationService);
 
-            // Ensure the options persisters are loaded since we have to fetch options from the shell
-            LoadOptionPersistersAsync(this.ComponentModel, cancellationToken).Forget();
-
             return Task.CompletedTask;
         }
 
@@ -145,21 +141,6 @@ private async Task ProfferServiceBrokerServicesAsync(CancellationToken cancellat
             (_, _, _, _) => ValueTaskFactory.FromResult<object?>(new ManagedEditAndContinueLanguageServiceBridge(this.ComponentModel.GetService<EditAndContinueLanguageService>())));
     }
 
-    private async Task LoadOptionPersistersAsync(IComponentModel componentModel, CancellationToken cancellationToken)
-    {
-        // Ensure on a background thread to ensure assembly loads don't show up as UI delays attributed to
-        // InitializeAsync.
-        Contract.ThrowIfTrue(JoinableTaskFactory.Context.IsOnMainThread);
-
-        var listenerProvider = componentModel.GetService<IAsynchronousOperationListenerProvider>();
-        using var token = listenerProvider.GetListener(FeatureAttribute.Workspace).BeginAsyncOperation(nameof(LoadOptionPersistersAsync));
-
-        var persisterProviders = componentModel.GetExtensions<IOptionPersisterProvider>().ToImmutableArray();
-
-        foreach (var provider in persisterProviders)
-            await provider.GetOrCreatePersisterAsync(cancellationToken).ConfigureAwait(true);
-    }
-
     protected override async Task LoadComponentsAsync(CancellationToken cancellationToken)
     {
         await TaskScheduler.Default;

src/VisualStudio/IntegrationTest/New.IntegrationTests/Options/GlobalOptionsTest.cs

@@ -29,7 +29,7 @@ public async Task ValidateAllOptions()
     {
         var globalOptions = (GlobalOptionService)await TestServices.Shell.GetComponentModelServiceAsync<IGlobalOptionService>(HangMitigatingCancellationToken);
         var provider = await TestServices.Shell.GetComponentModelServiceAsync<VisualStudioOptionPersisterProvider>(HangMitigatingCancellationToken);
-        var vsSettingsPersister = (VisualStudioOptionPersister)await provider.GetOrCreatePersisterAsync(HangMitigatingCancellationToken);
+        var vsSettingsPersister = (VisualStudioOptionPersister)provider.GetOrCreatePersister();
 
         var optionsInfo = OptionsTestInfo.CollectOptions(Path.GetDirectoryName(typeof(GlobalOptionsTest).Assembly.Location!));
         var allLanguages = new[] { LanguageNames.CSharp, LanguageNames.VisualBasic };

src/Workspaces/Core/Portable/Options/GlobalOptionService.cs

@@ -7,12 +7,9 @@
 using System.Collections.Immutable;
 using System.Composition;
 using System.Diagnostics;
-using System.Threading;
-using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.Host.Mef;
 using Microsoft.CodeAnalysis.PooledObjects;
 using Microsoft.CodeAnalysis.Shared.Collections;
-using Microsoft.CodeAnalysis.Shared.Utilities;
 using Roslyn.Utilities;
 
 namespace Microsoft.CodeAnalysis.Options;
@@ -21,62 +18,23 @@ namespace Microsoft.CodeAnalysis.Options;
 [method: ImportingConstructor]
 [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
 internal sealed class GlobalOptionService(
-    [Import(AllowDefault = true)] IWorkspaceThreadingService? workspaceThreadingService,
-    [ImportMany] IEnumerable<Lazy<IOptionPersisterProvider>> optionPersisters) : IGlobalOptionService
+    [ImportMany] IEnumerable<Lazy<IOptionPersisterProvider>> optionPersisterProviders) : IGlobalOptionService
 {
-    private readonly ImmutableArray<Lazy<IOptionPersisterProvider>> _optionPersisterProviders = [.. optionPersisters];
-
     private readonly object _gate = new();
 
     #region Guarded by _gate
 
-    private ImmutableArray<IOptionPersister> _lazyOptionPersisters;
+    private readonly Lazy<ImmutableArray<IOptionPersister>> _optionPersisters = new(() => GetOptionPersisters(optionPersisterProviders));
     private ImmutableDictionary<OptionKey2, object?> _currentValues = ImmutableDictionary.Create<OptionKey2, object?>();
 
     #endregion
 
     private readonly WeakEvent<OptionChangedEventArgs> _optionChanged = new();
 
-    private ImmutableArray<IOptionPersister> GetOptionPersisters()
+    private static ImmutableArray<IOptionPersister> GetOptionPersisters(IEnumerable<Lazy<IOptionPersisterProvider>> optionPersisterProviders)
     {
-        if (_lazyOptionPersisters.IsDefault)
-        {
-            // Option persisters cannot be initialized while holding the global options lock
-            // https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1353715
-            Debug.Assert(!Monitor.IsEntered(_gate));
-
-            ImmutableInterlocked.InterlockedInitialize(
-                ref _lazyOptionPersisters,
-                GetOptionPersistersSlow(workspaceThreadingService, _optionPersisterProviders, CancellationToken.None));
-        }
-
-        return _lazyOptionPersisters;
-
-        // Local functions
-        static ImmutableArray<IOptionPersister> GetOptionPersistersSlow(
-            IWorkspaceThreadingService? workspaceThreadingService,
-            ImmutableArray<Lazy<IOptionPersisterProvider>> persisterProviders,
-            CancellationToken cancellationToken)
-        {
-            if (workspaceThreadingService is not null && workspaceThreadingService.IsOnMainThread)
-            {
-                // speedometer tests report jtf.run calls from background threads, so we try to avoid those.
-                return workspaceThreadingService.Run(() => GetOptionPersistersAsync(persisterProviders, cancellationToken));
-            }
-            else
-            {
-                return GetOptionPersistersAsync(persisterProviders, cancellationToken).WaitAndGetResult_CanCallOnBackground(cancellationToken);
-            }
-        }
-
-        static async Task<ImmutableArray<IOptionPersister>> GetOptionPersistersAsync(
-            ImmutableArray<Lazy<IOptionPersisterProvider>> persisterProviders,
-            CancellationToken cancellationToken)
-        {
-            return await persisterProviders.SelectAsArrayAsync(
-                static (lazyProvider, cancellationToken) => lazyProvider.Value.GetOrCreatePersisterAsync(cancellationToken),
-                cancellationToken).ConfigureAwait(false);
-        }
+        return optionPersisterProviders.SelectAsArray(
+            static provider => provider.Value.GetOrCreatePersister());
     }
 
     private static object? LoadOptionFromPersisterOrGetDefault(OptionKey2 optionKey, ImmutableArray<IOptionPersister> persisters)
@@ -108,16 +66,15 @@ public T GetOption<T>(PerLanguageOption2<T> option, string language)
 
     public T GetOption<T>(OptionKey2 optionKey)
     {
-        // Ensure the option persisters are available before taking the global lock
-        var persisters = GetOptionPersisters();
-
         // Performance: This is called very frequently, with the vast majority (> 99%) of calls requesting a previously
         //  added key. In those cases, we can avoid taking the lock as _currentValues is an immutable structure.
         if (_currentValues.TryGetValue(optionKey, out var value))
         {
             return (T)value!;
         }
 
+        // Ensure the option persisters are available before taking the global lock
+        var persisters = _optionPersisters.Value;
         lock (_gate)
         {
             return (T)GetOption_NoLock(ref _currentValues, optionKey, persisters)!;
@@ -126,8 +83,6 @@ public T GetOption<T>(OptionKey2 optionKey)
 
     public ImmutableArray<object?> GetOptions(ImmutableArray<OptionKey2> optionKeys)
     {
-        // Ensure the option persisters are available before taking the global lock
-        var persisters = GetOptionPersisters();
         using var values = TemporaryArray<object?>.Empty;
 
         // Performance: The vast majority of calls are for previously added keys. In those cases, we can avoid taking the lock
@@ -146,6 +101,8 @@ public T GetOption<T>(OptionKey2 optionKey)
 
         if (values.Count != optionKeys.Length)
         {
+            // Ensure the option persisters are available before taking the global lock
+            var persisters = _optionPersisters.Value;
             lock (_gate)
             {
                 foreach (var optionKey in optionKeys)
@@ -190,7 +147,7 @@ public bool SetGlobalOptions(ImmutableArray<KeyValuePair<OptionKey2, object?>> o
     private bool SetGlobalOptions(OneOrMany<KeyValuePair<OptionKey2, object?>> options)
     {
         using var _ = ArrayBuilder<(OptionKey2, object?)>.GetInstance(options.Count, out var changedOptions);
-        var persisters = GetOptionPersisters();
+        var persisters = _optionPersisters.Value;
 
         lock (_gate)
         {

src/Workspaces/Core/Portable/Options/IOptionPersisterProvider.cs

@@ -2,9 +2,6 @@
 // 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.Threading;
-using System.Threading.Tasks;
-
 namespace Microsoft.CodeAnalysis.Options;
 
 internal interface IOptionPersisterProvider
@@ -16,7 +13,6 @@ internal interface IOptionPersisterProvider
     /// This method is safe for concurrent use from any thread. No guarantees are made regarding the use of the UI
     /// thread.
     /// </remarks>
-    /// <param name="cancellationToken">A cancellation token the operation may observe.</param>
     /// <returns>The option persister.</returns>
-    ValueTask<IOptionPersister> GetOrCreatePersisterAsync(CancellationToken cancellationToken);
+    IOptionPersister GetOrCreatePersister();
 }