From 7ee2e1fa8cee997f7ac7398fb8e826ac00b089df Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Wed, 1 Mar 2023 17:10:02 -0600 Subject: [PATCH 1/2] Prevent LegacyGlobalOptionService from GC rooting PreviewWorkspace Fixes https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1756080 --- .../Options/LegacyWorkspaceOptionService.cs | 34 +++++++++++++++---- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/src/Workspaces/Core/Portable/Options/LegacyWorkspaceOptionService.cs b/src/Workspaces/Core/Portable/Options/LegacyWorkspaceOptionService.cs index 23aa69abff501..c09829e856e3a 100644 --- a/src/Workspaces/Core/Portable/Options/LegacyWorkspaceOptionService.cs +++ b/src/Workspaces/Core/Portable/Options/LegacyWorkspaceOptionService.cs @@ -30,7 +30,7 @@ public WorkspaceService(ILegacyGlobalOptionService legacyGlobalOptions) public IGlobalOptionService GlobalOptions { get; } // access is interlocked - private ImmutableArray _registeredWorkspaces; + private ImmutableArray> _registeredWorkspaces; /// /// Stores options that are not defined by Roslyn and do not implement . @@ -42,7 +42,7 @@ public WorkspaceService(ILegacyGlobalOptionService legacyGlobalOptions) public LegacyGlobalOptionService(IGlobalOptionService globalOptionService) { GlobalOptions = globalOptionService; - _registeredWorkspaces = ImmutableArray.Empty; + _registeredWorkspaces = ImmutableArray>.Empty; _currentExternallyDefinedOptionValues = ImmutableDictionary.Create(); } @@ -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); + } 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); + } } From 37125a45e96542037daf8983ad8a8ed84e8d40fe Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 2 Mar 2023 13:41:24 -0600 Subject: [PATCH 2/2] Verify workspaces are not GC rooted on construction --- src/Compilers/Test/Core/ObjectReference.cs | 10 ++++++ .../Test/Preview/PreviewWorkspaceTests.cs | 31 ++++++++++++++++--- .../WorkspaceTests/AdhocWorkspaceTests.cs | 18 +++++++++++ .../VisualStudioMSBuildWorkspaceTests.cs | 1 - 4 files changed, 55 insertions(+), 5 deletions(-) diff --git a/src/Compilers/Test/Core/ObjectReference.cs b/src/Compilers/Test/Core/ObjectReference.cs index a93ad8393b518..a9719662c29e7 100644 --- a/src/Compilers/Test/Core/ObjectReference.cs +++ b/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 CreateFromFactory(Func targetFactory, TArg arg) + where T : class + { + return new ObjectReference(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. diff --git a/src/EditorFeatures/Test/Preview/PreviewWorkspaceTests.cs b/src/EditorFeatures/Test/Preview/PreviewWorkspaceTests.cs index 0024cd12e7041..2ef4fc2d031af 100644 --- a/src/EditorFeatures/Test/Preview/PreviewWorkspaceTests.cs +++ b/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(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 analyzers) { var analyzerOptions = new AnalyzerOptions(additionalFiles: ImmutableArray.Empty); diff --git a/src/Workspaces/CoreTest/WorkspaceTests/AdhocWorkspaceTests.cs b/src/Workspaces/CoreTest/WorkspaceTests/AdhocWorkspaceTests.cs index dbccdfe0d9a53..2278b02a56d25 100644 --- a/src/Workspaces/CoreTest/WorkspaceTests/AdhocWorkspaceTests.cs +++ b/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); + } } } diff --git a/src/Workspaces/MSBuildTest/VisualStudioMSBuildWorkspaceTests.cs b/src/Workspaces/MSBuildTest/VisualStudioMSBuildWorkspaceTests.cs index bb6bb331171ca..101b9a6aa2c5d 100644 --- a/src/Workspaces/MSBuildTest/VisualStudioMSBuildWorkspaceTests.cs +++ b/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());