Merge pull request #67142 from sharwell/no-rooting

Prevent LegacyGlobalOptionService from GC rooting PreviewWorkspace

src/Compilers/Test/Core/ObjectReference.cs

@@ -12,6 +12,16 @@ namespace Roslyn.Test.Utilities
 {
     public static class ObjectReference
     {
+        // We want to ensure this isn't inlined, because we need to ensure that any temporaries
+        // on the stack in this method or targetFactory get cleaned up. Otherwise, they might still
+        // be alive when we want to make later assertions.
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        public static ObjectReference<T> CreateFromFactory<T, TArg>(Func<TArg, T> targetFactory, TArg arg)
+            where T : class
+        {
+            return new ObjectReference<T>(targetFactory(arg));
+        }
+
         // We want to ensure this isn't inlined, because we need to ensure that any temporaries
         // on the stack in this method or targetFactory get cleaned up. Otherwise, they might still
         // be alive when we want to make later assertions.

src/EditorFeatures/Test/Preview/PreviewWorkspaceTests.cs

@@ -220,16 +220,21 @@ public async Task TestPreviewDiagnosticTaggerInPreviewPane()
         }
 
         [WorkItem(28639, "https://github.com/dotnet/roslyn/issues/28639")]
-        [ConditionalFact(typeof(Bitness32))]
+        [Fact]
         public void TestPreviewWorkspaceDoesNotLeakSolution()
         {
             // Verify that analyzer execution doesn't leak solution instances from the preview workspace.
 
             var previewWorkspace = new PreviewWorkspace();
             Assert.NotNull(previewWorkspace.CurrentSolution);
-            var project = previewWorkspace.CurrentSolution.AddProject("project", "project.dll", LanguageNames.CSharp);
-            Assert.True(previewWorkspace.TryApplyChanges(project.Solution));
-            var solutionObjectReference = ObjectReference.Create(previewWorkspace.CurrentSolution);
+            var solutionObjectReference = ObjectReference.CreateFromFactory(
+                static previewWorkspace =>
+                {
+                    var project = previewWorkspace.CurrentSolution.AddProject("project", "project.dll", LanguageNames.CSharp);
+                    Assert.True(previewWorkspace.TryApplyChanges(project.Solution));
+                    return previewWorkspace.CurrentSolution;
+                },
+                previewWorkspace);
 
             var analyzers = ImmutableArray.Create<DiagnosticAnalyzer>(new CommonDiagnosticAnalyzers.NotConfigurableDiagnosticAnalyzer());
             ExecuteAnalyzers(previewWorkspace, analyzers);
@@ -238,6 +243,24 @@ public void TestPreviewWorkspaceDoesNotLeakSolution()
             solutionObjectReference.AssertReleased();
         }
 
+        [Fact]
+        [WorkItem(67142, "https://github.com/dotnet/roslyn/pull/67142")]
+        public void TestPreviewWorkspaceDoesNotLeakItself()
+        {
+            var composition = EditorTestCompositions.EditorFeatures;
+            var exportProvider = composition.ExportProviderFactory.CreateExportProvider();
+            var previewWorkspaceReference = ObjectReference.CreateFromFactory(
+                static composition => new PreviewWorkspace(composition.GetHostServices()),
+                composition);
+
+            // Verify the GC can reclaim member for a workspace which has not been disposed.
+            previewWorkspaceReference.AssertReleased();
+
+            // Keep the export provider alive longer than the workspace to further ensure that the workspace is not GC
+            // rooted within the export provider instance.
+            GC.KeepAlive(exportProvider);
+        }
+
         private static void ExecuteAnalyzers(PreviewWorkspace previewWorkspace, ImmutableArray<DiagnosticAnalyzer> analyzers)
         {
             var analyzerOptions = new AnalyzerOptions(additionalFiles: ImmutableArray<AdditionalText>.Empty);

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

@@ -30,7 +30,7 @@ public WorkspaceService(ILegacyGlobalOptionService legacyGlobalOptions)
     public IGlobalOptionService GlobalOptions { get; }
 
     // access is interlocked
-    private ImmutableArray<Workspace> _registeredWorkspaces;
+    private ImmutableArray<WeakReference<Workspace>> _registeredWorkspaces;
 
     /// <summary>
     /// Stores options that are not defined by Roslyn and do not implement <see cref="IOption2"/>.
@@ -42,7 +42,7 @@ public WorkspaceService(ILegacyGlobalOptionService legacyGlobalOptions)
     public LegacyGlobalOptionService(IGlobalOptionService globalOptionService)
     {
         GlobalOptions = globalOptionService;
-        _registeredWorkspaces = ImmutableArray<Workspace>.Empty;
+        _registeredWorkspaces = ImmutableArray<WeakReference<Workspace>>.Empty;
         _currentExternallyDefinedOptionValues = ImmutableDictionary.Create<OptionKey, object?>();
     }
 
@@ -102,16 +102,38 @@ public void UpdateRegisteredWorkspaces()
     {
         // Ensure that the Workspace's CurrentSolution snapshot is updated with new options for all registered workspaces
         // prior to raising option changed event handlers.
-        foreach (var workspace in _registeredWorkspaces)
+        foreach (var weakWorkspace in _registeredWorkspaces)
         {
+            if (!weakWorkspace.TryGetTarget(out var workspace))
+                continue;
+
             workspace.UpdateCurrentSolutionOnOptionsChanged();
         }
     }
 
     public void RegisterWorkspace(Workspace workspace)
-        => ImmutableInterlocked.Update(ref _registeredWorkspaces, (workspaces, workspace) => workspaces.Add(workspace), workspace);
+    {
+        ImmutableInterlocked.Update(
+            ref _registeredWorkspaces,
+            static (workspaces, workspace) =>
+            {
+                return workspaces
+                    .RemoveAll(static weakWorkspace => !weakWorkspace.TryGetTarget(out _))
+                    .Add(new WeakReference<Workspace>(workspace));
+            },
+            workspace);
+    }
 
     public void UnregisterWorkspace(Workspace workspace)
-        => ImmutableInterlocked.Update(ref _registeredWorkspaces, (workspaces, workspace) => workspaces.Remove(workspace), workspace);
-
+    {
+        ImmutableInterlocked.Update(
+            ref _registeredWorkspaces,
+            static (workspaces, workspace) =>
+            {
+                return workspaces.WhereAsArray(
+                    static (weakWorkspace, workspaceToRemove) => weakWorkspace.TryGetTarget(out var workspace) && workspace != workspaceToRemove,
+                    workspace);
+            },
+            workspace);
+    }
 }

src/Workspaces/CoreTest/WorkspaceTests/AdhocWorkspaceTests.cs

@@ -599,5 +599,23 @@ public void TestDefaultDocumentTextDifferencingService()
             Assert.NotNull(service);
             Assert.Equal(typeof(DefaultDocumentTextDifferencingService), service.GetType());
         }
+
+        [Fact]
+        [WorkItem(67142, "https://github.com/dotnet/roslyn/pull/67142")]
+        public void TestNotGCRootedOnConstruction()
+        {
+            var composition = FeaturesTestCompositions.Features;
+            var exportProvider = composition.ExportProviderFactory.CreateExportProvider();
+            var adhocWorkspaceReference = ObjectReference.CreateFromFactory(
+                static composition => new AdhocWorkspace(composition.GetHostServices()),
+                composition);
+
+            // Verify the GC can reclaim member for a workspace which has not been disposed.
+            adhocWorkspaceReference.AssertReleased();
+
+            // Keep the export provider alive longer than the workspace to further ensure that the workspace is not GC
+            // rooted within the export provider instance.
+            GC.KeepAlive(exportProvider);
+        }
     }
 }

src/Workspaces/MSBuildTest/VisualStudioMSBuildWorkspaceTests.cs

@@ -2801,7 +2801,6 @@ public async Task MSBuildProjectShouldHandleDefaultCodePageProperty()
 
         [ConditionalFact(typeof(VisualStudioMSBuildInstalled))]
         [WorkItem(981208, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/981208")]
-        [WorkItem(28639, "https://github.com/dotnet/roslyn/issues/28639")]
         public void DisposeMSBuildWorkspaceAndServicesCollected()
         {
             CreateFiles(GetSimpleCSharpSolutionFiles());