diff --git a/src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs b/src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs index 2fab59685e7b55592cb35b9a08b1d16af277137c..9b026888f6eb03ac4db13bb1aa598d5eb4da3934 100644 --- a/src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs +++ b/src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs @@ -3807,16 +3807,12 @@ class C var compilation = CreateCompilationWithMscorlib(text); var tree = compilation.SyntaxTrees[0]; var node = (IdentifierNameSyntax)tree.GetCompilationUnitRoot().DescendantTokens().Where(t => t.ToString() == "Alias").Last().Parent; - var model1 = compilation.GetSemanticModel(tree); - var alias1 = model1.GetAliasInfo(node); + var modelWeakReference = ObjectReference.CreateFromFactory(() => compilation.GetSemanticModel(tree)); + var alias1 = modelWeakReference.UseReference(sm => sm.GetAliasInfo(node)); // We want the Compilation's WeakReference to be collected // so that the next semantic model will get a new one. - model1 = null; // otherwise it will keep the BinderFactory alive - GC.Collect(GC.MaxGeneration); - GC.WaitForPendingFinalizers(); - GC.Collect(GC.MaxGeneration); - GC.WaitForPendingFinalizers(); + modelWeakReference.AssertReleased(); var model2 = compilation.GetSemanticModel(tree); var alias2 = model2.GetAliasInfo(node); diff --git a/src/Compilers/Core/CodeAnalysisTest/InternalUtilities/WeakListTests.cs b/src/Compilers/Core/CodeAnalysisTest/InternalUtilities/WeakListTests.cs index 6aea7be420949627ae7695b63d4530131b6b5ca2..70d45bbe75e775febccf80da610c9a478222b069 100644 --- a/src/Compilers/Core/CodeAnalysisTest/InternalUtilities/WeakListTests.cs +++ b/src/Compilers/Core/CodeAnalysisTest/InternalUtilities/WeakListTests.cs @@ -31,15 +31,15 @@ public override string ToString() } [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)] - private ObjectReference Create(string value) + private ObjectReference Create(string value) { - return new ObjectReference(new C(value)); + return new ObjectReference(new C(value)); } [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)] - private void Add(WeakList list, ObjectReference value) + private void Add(WeakList list, ObjectReference value) { - list.Add(value.Strong); + value.UseReference(r => list.Add(r)); } [Fact] @@ -71,19 +71,14 @@ public void EnumeratorCompacts() Assert.Equal(5, list.WeakCount); - a.Strong = null; - c.Strong = null; - d.Strong = null; - e.Strong = null; - - while (a.Weak.IsAlive || c.Weak.IsAlive || d.Weak.IsAlive || e.Weak.IsAlive) - { - GC.Collect(2, GCCollectionMode.Forced); - } + a.AssertReleased(); + c.AssertReleased(); + d.AssertReleased(); + e.AssertReleased(); Assert.Equal(5, list.WeakCount); Assert.Same(null, list.GetWeakReference(0).GetTarget()); - Assert.Same(b.Strong, list.GetWeakReference(1).GetTarget()); + Assert.Same(b.GetReference(), list.GetWeakReference(1).GetTarget()); Assert.Same(null, list.GetWeakReference(2).GetTarget()); Assert.Same(null, list.GetWeakReference(3).GetTarget()); Assert.Same(null, list.GetWeakReference(4).GetTarget()); @@ -91,14 +86,14 @@ public void EnumeratorCompacts() var array = list.ToArray(); Assert.Equal(1, array.Length); - Assert.Same(b.Strong, array[0]); + Assert.Same(b.GetReference(), array[0]); // list was compacted: Assert.Equal(1, list.WeakCount); - Assert.Same(b.Strong, list.GetWeakReference(0).GetTarget()); + Assert.Same(b.GetReference(), list.GetWeakReference(0).GetTarget()); Assert.Equal(4, list.TestOnly_UnderlyingArray.Length); - GC.KeepAlive(b.Strong); + GC.KeepAlive(b.GetReference()); } [Fact] @@ -114,16 +109,16 @@ public void ResizeCompactsAllDead() Assert.Equal(list.WeakCount, list.TestOnly_UnderlyingArray.Length); // full - a.Strong = null; - while (a.Weak.IsAlive) - { - GC.Collect(2, GCCollectionMode.Forced); - } + a.AssertReleased(); - Add(list, a); // shrinks, #alive < length/4 + var b = Create("B"); + + Add(list, b); // shrinks, #alive < length/4 Assert.Equal(4, list.TestOnly_UnderlyingArray.Length); Assert.Equal(1, list.WeakCount); + b.AssertReleased(); + list.ToArray(); // shrinks, #alive == 0 Assert.Equal(0, list.TestOnly_UnderlyingArray.Length); Assert.Equal(0, list.WeakCount); @@ -144,21 +139,13 @@ public void ResizeCompactsFirstFourth() Add(list, b); Assert.Equal(list.WeakCount, list.TestOnly_UnderlyingArray.Length); // full - a.Strong = null; - while (a.Weak.IsAlive) - { - GC.Collect(2, GCCollectionMode.Forced); - } + a.AssertReleased(); Add(list, b); // shrinks, #alive < length/4 Assert.Equal(4, list.TestOnly_UnderlyingArray.Length); Assert.Equal(2, list.WeakCount); - b.Strong = null; - while (b.Weak.IsAlive) - { - GC.Collect(2, GCCollectionMode.Forced); - } + b.AssertReleased(); list.ToArray(); // shrinks, #alive == 0 Assert.Equal(0, list.TestOnly_UnderlyingArray.Length); @@ -184,11 +171,7 @@ public void ResizeCompactsSecondFourth() Assert.Equal(list.WeakCount, list.TestOnly_UnderlyingArray.Length); // full - a.Strong = null; - while (a.Weak.IsAlive) - { - GC.Collect(2, GCCollectionMode.Forced); - } + a.AssertReleased(); Add(list, b); // just compacts, length/4 < #alive < 3/4 length Assert.Equal(9, list.TestOnly_UnderlyingArray.Length); @@ -198,7 +181,7 @@ public void ResizeCompactsSecondFourth() { if (i < 4) { - Assert.Same(b.Strong, list.TestOnly_UnderlyingArray[i].GetTarget()); + Assert.Same(b.GetReference(), list.TestOnly_UnderlyingArray[i].GetTarget()); } else { @@ -206,7 +189,7 @@ public void ResizeCompactsSecondFourth() } } - GC.KeepAlive(b.Strong); + GC.KeepAlive(b); } [Fact] @@ -228,11 +211,7 @@ public void ResizeCompactsThirdFourth() Assert.Equal(list.WeakCount, list.TestOnly_UnderlyingArray.Length); // full - a.Strong = null; - while (a.Weak.IsAlive) - { - GC.Collect(2, GCCollectionMode.Forced); - } + a.AssertReleased(); Add(list, b); // compacts #alive < 3/4 length Assert.Equal(9, list.TestOnly_UnderlyingArray.Length); @@ -242,7 +221,7 @@ public void ResizeCompactsThirdFourth() { if (i < 6) { - Assert.Same(b.Strong, list.TestOnly_UnderlyingArray[i].GetTarget()); + Assert.Same(b.GetReference(), list.TestOnly_UnderlyingArray[i].GetTarget()); } else { @@ -250,7 +229,7 @@ public void ResizeCompactsThirdFourth() } } - GC.KeepAlive(b.Strong); + GC.KeepAlive(b); } [Fact] @@ -272,11 +251,7 @@ public void ResizeCompactsLastFourth() Assert.Equal(list.WeakCount, list.TestOnly_UnderlyingArray.Length); // full - a.Strong = null; - while (a.Weak.IsAlive) - { - GC.Collect(2, GCCollectionMode.Forced); - } + a.AssertReleased(); Add(list, b); // expands #alive > 3/4 length Assert.Equal(9 * 2 + 1, list.TestOnly_UnderlyingArray.Length); @@ -286,7 +261,7 @@ public void ResizeCompactsLastFourth() { if (i < 8) { - Assert.Same(b.Strong, list.TestOnly_UnderlyingArray[i].GetTarget()); + Assert.Same(b.GetReference(), list.TestOnly_UnderlyingArray[i].GetTarget()); } else { @@ -294,7 +269,7 @@ public void ResizeCompactsLastFourth() } } - GC.KeepAlive(b.Strong); + GC.KeepAlive(b); } [Fact] @@ -318,7 +293,7 @@ public void ResizeCompactsAllAlive() { if (i < 10) { - Assert.Same(b.Strong, list.TestOnly_UnderlyingArray[i].GetTarget()); + Assert.Same(b.GetReference(), list.TestOnly_UnderlyingArray[i].GetTarget()); } else { @@ -326,7 +301,7 @@ public void ResizeCompactsAllAlive() } } - GC.KeepAlive(b.Strong); + GC.KeepAlive(b); } [Fact] diff --git a/src/EditorFeatures/Core/Implementation/Workspaces/ProjectCacheService.cs b/src/EditorFeatures/Core/Implementation/Workspaces/ProjectCacheService.cs index 41f1155a165256ad2b4c757a2c039dffd70106a9..9a2f2ef87f33a024cd73cf6cbb5b150425c1da48 100644 --- a/src/EditorFeatures/Core/Implementation/Workspaces/ProjectCacheService.cs +++ b/src/EditorFeatures/Core/Implementation/Workspaces/ProjectCacheService.cs @@ -159,7 +159,7 @@ private class Cache : IDisposable internal int Count; private readonly ProjectCacheService _cacheService; private readonly ProjectId _key; - private readonly ConditionalWeakTable _cache = new ConditionalWeakTable(); + private ConditionalWeakTable _cache = new ConditionalWeakTable(); private readonly List> _ownerObjects = new List>(); public Cache(ProjectCacheService cacheService, ProjectId key) @@ -197,6 +197,14 @@ internal void FreeOwnerEntries() owner.CachedObject = null; } } + + // Explicitly free our ConditionalWeakTable to make sure it's released. We have a number of places in the codebase + // (in both tests and product code) that do using (service.EnableCaching), which implicitly returns a disposable instance + // this type. The runtime in many cases disposes, but does not unroot, the underlying object after the the using block is exited. + // This means the cache could still be rooting objects we don't expect it to be rooting by that point. By explicitly clearing + // these out, we get the expected behavior. + _cache = null; + _ownerObjects.Clear(); } } } diff --git a/src/EditorFeatures/Test/Workspaces/ProjectCacheHostServiceFactoryTests.cs b/src/EditorFeatures/Test/Workspaces/ProjectCacheHostServiceFactoryTests.cs index 9be8e0d32e3d6915604af50dedbf47cba48eea54..79b8423133fdd47aa2719d9317916b6ca088a591 100644 --- a/src/EditorFeatures/Test/Workspaces/ProjectCacheHostServiceFactoryTests.cs +++ b/src/EditorFeatures/Test/Workspaces/ProjectCacheHostServiceFactoryTests.cs @@ -15,7 +15,7 @@ namespace Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces { public class ProjectCacheHostServiceFactoryTests { - private void Test(Action action) + private void Test(Action> action) { // Putting cacheService.CreateStrongReference in a using statement // creates a temporary local that isn't collected in Debug builds @@ -23,7 +23,7 @@ private void Test(Action new object()); action(cacheService, projectId, owner, instance); } @@ -33,19 +33,15 @@ public void TestCacheKeepsObjectAlive1() { Test((cacheService, projectId, owner, instance) => { - ((Action)(() => + using (cacheService.EnableCaching(projectId)) { - using (cacheService.EnableCaching(projectId)) - { - cacheService.CacheObjectIfCachingEnabledForKey(projectId, (object)owner, instance.Strong); - instance.Strong = null; - CollectGarbage(); - Assert.True(instance.Weak.IsAlive); - } - })).Invoke(); - - CollectGarbage(); - Assert.False(instance.Weak.IsAlive); + instance.UseReference(i => cacheService.CacheObjectIfCachingEnabledForKey(projectId, (object)owner, i)); + + instance.AssertHeld(); + } + + instance.AssertReleased(); + GC.KeepAlive(owner); }); } @@ -55,19 +51,15 @@ public void TestCacheKeepsObjectAlive2() { Test((cacheService, projectId, owner, instance) => { - ((Action)(() => + using (cacheService.EnableCaching(projectId)) { - using (cacheService.EnableCaching(projectId)) - { - cacheService.CacheObjectIfCachingEnabledForKey(projectId, owner, instance.Strong); - instance.Strong = null; - CollectGarbage(); - Assert.True(instance.Weak.IsAlive); - } - })).Invoke(); - - CollectGarbage(); - Assert.False(instance.Weak.IsAlive); + instance.UseReference(i => cacheService.CacheObjectIfCachingEnabledForKey(projectId, owner, i)); + + instance.AssertHeld(); + } + + instance.AssertReleased(); + GC.KeepAlive(owner); }); } @@ -81,9 +73,8 @@ public void TestCacheDoesNotKeepObjectsAliveAfterOwnerIsCollected1() { cacheService.CacheObjectIfCachingEnabledForKey(projectId, (object)owner, instance); owner = null; - instance.Strong = null; - CollectGarbage(); - Assert.False(instance.Weak.IsAlive); + + instance.AssertReleased(); } }); } @@ -97,9 +88,8 @@ public void TestCacheDoesNotKeepObjectsAliveAfterOwnerIsCollected2() { cacheService.CacheObjectIfCachingEnabledForKey(projectId, owner, instance); owner = null; - instance.Strong = null; - CollectGarbage(); - Assert.False(instance.Weak.IsAlive); + + instance.AssertReleased(); } }); } @@ -109,12 +99,10 @@ public void TestImplicitCacheKeepsObjectAlive1() { var workspace = new AdhocWorkspace(MockHostServices.Instance, workspaceKind: WorkspaceKind.Host); var cacheService = new ProjectCacheService(workspace, int.MaxValue); - var instance = new object(); - var weak = new WeakReference(instance); - cacheService.CacheObjectIfCachingEnabledForKey(ProjectId.CreateNewId(), (object)null, instance); - instance = null; - CollectGarbage(); - Assert.True(weak.IsAlive); + var reference = ObjectReference.CreateFromFactory(() => new object()); + reference.UseReference(r => cacheService.CacheObjectIfCachingEnabledForKey(ProjectId.CreateNewId(), (object)null, r)); + reference.AssertHeld(); + GC.KeepAlive(cacheService); } @@ -125,39 +113,18 @@ public void TestImplicitCacheMonitoring() var cacheService = new ProjectCacheService(workspace, 10); var weak = PutObjectInImplicitCache(cacheService); - var timeout = TimeSpan.FromSeconds(10); - var current = DateTime.UtcNow; - do - { - Thread.Sleep(100); - CollectGarbage(); - - if (DateTime.UtcNow - current > timeout) - { - break; - } - } - while (weak.IsAlive); - - // FailFast (and thereby capture a dump) to investigate what's - // rooting the object. - if (weak.IsAlive) - { - CodeAnalysis.Test.Utilities.ExceptionUtilities.FailFast(new Exception("Please investigate why the object wasn't collected!")); - } + weak.AssertReleased(); GC.KeepAlive(cacheService); } - private static WeakReference PutObjectInImplicitCache(ProjectCacheService cacheService) + private static ObjectReference PutObjectInImplicitCache(ProjectCacheService cacheService) { - var instance = new object(); - var weak = new WeakReference(instance); + var reference = ObjectReference.CreateFromFactory(() => new object()); - cacheService.CacheObjectIfCachingEnabledForKey(ProjectId.CreateNewId(), (object)null, instance); - instance = null; + reference.UseReference(r => cacheService.CacheObjectIfCachingEnabledForKey(ProjectId.CreateNewId(), (object)null, r)); - return weak; + return reference; } [Fact] @@ -171,14 +138,12 @@ public void TestP2PReference() var solution = workspace.AddSolution(solutionInfo); - var instance = new object(); - var weak = new WeakReference(instance); + var instanceTracker = ObjectReference.CreateFromFactory(() => new object()); var cacheService = new ProjectCacheService(workspace, int.MaxValue); using (var cache = cacheService.EnableCaching(project2.Id)) { - cacheService.CacheObjectIfCachingEnabledForKey(project1.Id, (object)null, instance); - instance = null; + instanceTracker.UseReference(r => cacheService.CacheObjectIfCachingEnabledForKey(project1.Id, (object)null, r)); solution = null; workspace.OnProjectRemoved(project1.Id); @@ -186,8 +151,7 @@ public void TestP2PReference() } // make sure p2p reference doesn't go to implicit cache - CollectGarbage(); - Assert.False(weak.IsAlive); + instanceTracker.AssertReleased(); } [Fact] @@ -199,8 +163,8 @@ public void TestEjectFromImplicitCache() compilations.Add(CSharpCompilation.Create(i.ToString())); } - var weakFirst = new WeakReference(compilations[0]); - var weakLast = new WeakReference(compilations[compilations.Count - 1]); + var weakFirst = ObjectReference.Create(compilations[0]); + var weakLast = ObjectReference.Create(compilations[compilations.Count - 1]); var workspace = new AdhocWorkspace(MockHostServices.Instance, workspaceKind: WorkspaceKind.Host); var cache = new ProjectCacheService(workspace, int.MaxValue); @@ -210,10 +174,10 @@ public void TestEjectFromImplicitCache() } compilations = null; - CollectGarbage(); - Assert.False(weakFirst.IsAlive); - Assert.True(weakLast.IsAlive); + weakFirst.AssertReleased(); + weakLast.AssertHeld(); + GC.KeepAlive(cache); } @@ -224,8 +188,8 @@ public void TestCacheCompilationTwice() var comp2 = CSharpCompilation.Create("2"); var comp3 = CSharpCompilation.Create("3"); - var weak3 = new WeakReference(comp3); - var weak1 = new WeakReference(comp1); + var weak3 = ObjectReference.Create(comp3); + var weak1 = ObjectReference.Create(comp1); var workspace = new AdhocWorkspace(MockHostServices.Instance, workspaceKind: WorkspaceKind.Host); var cache = new ProjectCacheService(workspace, int.MaxValue); @@ -241,10 +205,9 @@ public void TestCacheCompilationTwice() comp2 = null; comp3 = null; - CollectGarbage(); + weak3.AssertHeld(); + weak1.AssertHeld(); - Assert.True(weak1.IsAlive); - Assert.True(weak3.IsAlive); GC.KeepAlive(cache); } diff --git a/src/Test/Utilities/Desktop/ObjectReference.cs b/src/Test/Utilities/Desktop/ObjectReference.cs new file mode 100644 index 0000000000000000000000000000000000000000..842be37894efc8740e2af0e356a9419fff2ffd96 --- /dev/null +++ b/src/Test/Utilities/Desktop/ObjectReference.cs @@ -0,0 +1,142 @@ +// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Runtime.CompilerServices; +using Xunit; + +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) where T : class + { + return new ObjectReference(targetFactory()); + } + + public static ObjectReference Create(T target) where T : class + { + return new ObjectReference(target); + } + } + + /// + /// A wrapper to hold onto an object that you wish to make assertions about the lifetime of. This type has specific protections + /// to ensure the best possible patterns to avoid "gotchas" with these sorts of tests. + /// + public sealed class ObjectReference where T : class + { + private T _strongReference; + + /// + /// Tracks if was called, which means it's no longer safe to do lifetime assertions. + /// + private bool _strongReferenceRetrievedOutsideScopedCall; + + private readonly WeakReference _weakReference; + + public ObjectReference(T target) + { + if (target == null) + { + throw new ArgumentNullException(nameof(target)); + } + + _strongReference = target; + _weakReference = new WeakReference(target); + } + + /// + /// Asserts that the underlying object has been released. + /// + [MethodImpl(MethodImplOptions.NoInlining)] + public void AssertReleased() + { + ReleaseAndGarbageCollect(); + + Assert.False(_weakReference.IsAlive, "Reference should have been released but was not."); + } + + /// + /// Asserts that the underlying object is still being held. + /// + [MethodImpl(MethodImplOptions.NoInlining)] + public void AssertHeld() + { + ReleaseAndGarbageCollect(); + + // Since we are asserting it's still held, if it is held we can just recover our strong reference again + _strongReference = (T)_weakReference.Target; + Assert.True(_strongReference != null, "Reference should still be held."); + } + + // Ensure the mention of the field doesn't result in any local temporaries being created in the parent + [MethodImpl(MethodImplOptions.NoInlining)] + private void ReleaseAndGarbageCollect() + { + if (_strongReferenceRetrievedOutsideScopedCall) + { + throw new InvalidOperationException($"The strong reference being held by the {nameof(ObjectReference)} was retrieved via a call to {nameof(GetReference)}. Since the CLR might have cached a temporary somewhere in your stack, assertions can no longer be made about the correctness of lifetime."); + } + + _strongReference = null; + + // We'll loop 1000 times, or until the weak reference disappears. When we're trying to assert that the + // object is released, once the weak reference goes away, we know we're good. But if we're trying to assert + // that the object is held, our only real option is to know to do it "enough" times; but if it goes away then + // we are definitely done. + for (var i = 0; i < 1000 && _weakReference.IsAlive; i++) + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + } + } + + /// + /// Provides the underlying strong refernce to the given action. This method is marked not be inlined, to ensure that no temporaries are left + /// on the stack that might still root the strong reference. The caller must not "leak" the object out of the given action for any lifetime + /// assertions to be safe. + /// + [MethodImpl(MethodImplOptions.NoInlining)] + public void UseReference(Action action) + { + action(GetReferenceWithChecks()); + } + + /// + /// Provides the underlying strong refernce to the given function. This method is marked not be inlined, to ensure that no temporaries are left + /// on the stack that might still root the strong reference. The caller must not "leak" the object out of the given action for any lifetime + /// assertions to be safe. + /// + [MethodImpl(MethodImplOptions.NoInlining)] + public U UseReference(Func function) + { + return function(GetReferenceWithChecks()); + } + + /// + /// Fetches the object strongly being held from this. Because the value returned might be cached in a local temporary from + /// the caller of this function, no further calls to or may be called + /// on this object as the test is not valid either way. If you need to operate with the object without invalidating + /// the ability to reference the object, see . + /// + public T GetReference() + { + _strongReferenceRetrievedOutsideScopedCall = true; + return GetReferenceWithChecks(); + } + + private T GetReferenceWithChecks() + { + if (_strongReference == null) + { + throw new InvalidOperationException($"The type has already been released due to a call to {nameof(AssertReleased)}."); + } + + return _strongReference; + } + } +} diff --git a/src/Test/Utilities/Desktop/TestUtilities.Desktop.csproj b/src/Test/Utilities/Desktop/TestUtilities.Desktop.csproj index 8835cf58a4dc40c0ea4dbd5eafa457052bf83d10..cbf412283b900ce4bf98f693e36697afb6a17164 100644 --- a/src/Test/Utilities/Desktop/TestUtilities.Desktop.csproj +++ b/src/Test/Utilities/Desktop/TestUtilities.Desktop.csproj @@ -88,6 +88,7 @@ + diff --git a/src/Test/Utilities/Shared/FX/ObjectReference.cs b/src/Test/Utilities/Shared/FX/ObjectReference.cs deleted file mode 100644 index abde4d91457776e319b416b87086d99b45fc1199..0000000000000000000000000000000000000000 --- a/src/Test/Utilities/Shared/FX/ObjectReference.cs +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -using System; - -namespace Roslyn.Test.Utilities -{ - public class ObjectReference - { - public object Strong; - public readonly WeakReference Weak; - - public ObjectReference(object target) - { - this.Strong = target; - this.Weak = new WeakReference(target); - } - - public ObjectReference() : this(new object()) - { - } - } -} diff --git a/src/Test/Utilities/Shared/TestUtilities.projitems b/src/Test/Utilities/Shared/TestUtilities.projitems index 6203345b1c54a1fee616e3d1f72a3c1ec1aab903..fd1beb61f23d8a493a429c64d5538a635a04cd39 100644 --- a/src/Test/Utilities/Shared/TestUtilities.projitems +++ b/src/Test/Utilities/Shared/TestUtilities.projitems @@ -49,7 +49,6 @@ - diff --git a/src/VisualStudio/CSharp/Test/CSharpVisualStudioTest.csproj b/src/VisualStudio/CSharp/Test/CSharpVisualStudioTest.csproj index a0954517587db44f22f4a186120c7fa82fa80208..a2f8108c4a625f65c3a55671a34ad582e4ad2798 100644 --- a/src/VisualStudio/CSharp/Test/CSharpVisualStudioTest.csproj +++ b/src/VisualStudio/CSharp/Test/CSharpVisualStudioTest.csproj @@ -174,6 +174,7 @@ + diff --git a/src/VisualStudio/CSharp/Test/ProjectSystemShim/LifetimeTests.cs b/src/VisualStudio/CSharp/Test/ProjectSystemShim/LifetimeTests.cs new file mode 100644 index 0000000000000000000000000000000000000000..fa6248921b815137a63d05edfecea97c59478245 --- /dev/null +++ b/src/VisualStudio/CSharp/Test/ProjectSystemShim/LifetimeTests.cs @@ -0,0 +1,29 @@ +// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Linq; +using Microsoft.VisualStudio.LanguageServices.UnitTests.ProjectSystemShim.Framework; +using Roslyn.Test.Utilities; +using Xunit; + +namespace Roslyn.VisualStudio.CSharp.UnitTests.ProjectSystemShim +{ + public class LifetimeTests + { + [WpfFact] + [Trait(Traits.Feature, Traits.Features.ProjectSystemShims)] + [WorkItem(10358, "https://github.com/dotnet/roslyn/issues/10358")] + public void DisconnectingAProjectDoesNotLeak() + { + using (var environment = new TestEnvironment()) + { + var project = ObjectReference.CreateFromFactory(() => CSharpHelpers.CreateCSharpProject(environment, "Test")); + + Assert.Single(environment.Workspace.CurrentSolution.Projects); + + project.UseReference(p => p.Disconnect()); + project.AssertReleased(); + } + } + } +} diff --git a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioProjectTracker.WorkspaceHostState.cs b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioProjectTracker.WorkspaceHostState.cs index 47be212871557f7d2d23914b851b4847cf85b9de..b085d4f5c7e547644154d43e13dea65dd372d39a 100644 --- a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioProjectTracker.WorkspaceHostState.cs +++ b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioProjectTracker.WorkspaceHostState.cs @@ -1,5 +1,6 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Collections.Generic; using System.Collections.Immutable; using System.IO; @@ -147,6 +148,16 @@ internal void StartPushingToWorkspaceAndNotifyOfOpenDocuments(IEnumerable host.OnProjectRemoved(project.Id)); + foreach (var hostState in _workspaceHosts) + { + hostState.RemoveProject(project); + } } } diff --git a/src/VisualStudio/Core/Test/ProjectSystemShim/VisualBasicProjectTests.vb b/src/VisualStudio/Core/Test/ProjectSystemShim/VisualBasicProjectTests.vb index c9b7f436eb79c5bec941913c27614b9b1d816b27..aac04f88c3318b892c2063e32b96427bdbdf206f 100644 --- a/src/VisualStudio/Core/Test/ProjectSystemShim/VisualBasicProjectTests.vb +++ b/src/VisualStudio/Core/Test/ProjectSystemShim/VisualBasicProjectTests.vb @@ -24,5 +24,19 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.ProjectSystemShim project.Disconnect() End Using End Sub + + + + Public Sub DisconnectingAProjectDoesNotLeak() + Using environment = New TestEnvironment() + Dim project = ObjectReference.CreateFromFactory(Function() CreateVisualBasicProject(environment, "Test")) + + Assert.Single(environment.Workspace.CurrentSolution.Projects) + + project.UseReference(Sub(p) p.Disconnect()) + + project.AssertReleased() + End Using + End Sub End Class End Namespace diff --git a/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs b/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs index ee0abcd6c6fe73cb3115a4247848e4d22523d2eb..797f215898a66f3480e6816a77f4d70370d6fe42 100644 --- a/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs +++ b/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs @@ -612,7 +612,7 @@ public void TestSyntaxRootNotKeptAlive() .AddDocument(did, "foo.cs", "public class Foo { }"); var observedRoot = GetObservedSyntaxTreeRoot(sol, did); - StopObservingAndWaitForReferenceToGo(observedRoot); + observedRoot.AssertReleased(); // re-get the tree (should recover from storage, not reparse) var root = sol.GetDocument(did).GetSyntaxRootAsync().Result; @@ -645,7 +645,7 @@ public void TestDocumentChangedOnDiskIsNotObserved() Assert.Equal(text2, textOnDisk); // stop observing it and let GC reclaim it - StopObservingAndWaitForReferenceToGo(observedText); + observedText.AssertReleased(); // if we ask for the same text again we should get the original content var observedText2 = sol.GetDocument(did).GetTextAsync().Result; @@ -659,51 +659,6 @@ private Solution CreateNotKeptAliveSolution() return workspace.CurrentSolution; } - [DllImport("dbghelp.dll")] - private static extern bool MiniDumpWriteDump(IntPtr hProcess, int ProcessId, IntPtr hFile, int DumpType, IntPtr ExceptionParam, IntPtr UserStreamParam, IntPtr CallbackParam); - - private static void DumpProcess(string dumpFileName) - { - using (var fs = File.Create(dumpFileName)) - { - var proc = System.Diagnostics.Process.GetCurrentProcess(); - MiniDumpWriteDump(proc.Handle, proc.Id, fs.SafeFileHandle.DangerousGetHandle(), /*MiniDumpWithFullMemory*/2, IntPtr.Zero, IntPtr.Zero, IntPtr.Zero); - } - } - - private void StopObservingAndWaitForReferenceToGo(ObjectReference observed, int delay = 0, string dumpFileName = null) - { - // stop observing it and let GC reclaim it - observed.Strong = null; - - DateTime start = DateTime.UtcNow; - TimeSpan maximumTimeToWait = TimeSpan.FromSeconds(120); - - while (observed.Weak.IsAlive && (DateTime.UtcNow - start) < maximumTimeToWait) - { - GC.Collect(); - GC.WaitForPendingFinalizers(); - } - - const int TimerPrecision = 30; - var actualTimePassed = DateTime.UtcNow - start + TimeSpan.FromMilliseconds(TimerPrecision); - var isTargetCollected = observed.Weak.Target == null; - - if (!isTargetCollected && !string.IsNullOrEmpty(dumpFileName)) - { - if (string.Compare(Path.GetExtension(dumpFileName), ".dmp", StringComparison.OrdinalIgnoreCase) != 0) - { - dumpFileName += ".dmp"; - } - - DumpProcess(dumpFileName); - Assert.True(false, $"Target object not collected. Process dump saved to '{dumpFileName}'"); - } - - Assert.True(isTargetCollected, - string.Format("Target object ({0}) was not collected after {1} ms", observed.Weak.Target, actualTimePassed)); - } - [Fact, Trait(Traits.Feature, Traits.Features.Workspace)] public void TestGetTextAsync() { @@ -758,7 +713,7 @@ public void TestGetRecoveredTextAsync() // observe the text and then wait for the references to be GC'd var observed = GetObservedText(sol, did, text); - StopObservingAndWaitForReferenceToGo(observed); + observed.AssertReleased(); // get it async and force it to recover from temporary storage var doc = sol.GetDocument(did); @@ -861,7 +816,7 @@ public void TestGetRecoveredSyntaxRootAsync() // observe the syntax tree root and wait for the references to be GC'd var observed = GetObservedSyntaxTreeRoot(sol, did); - StopObservingAndWaitForReferenceToGo(observed); + observed.AssertReleased(); // get it async and force it to be recovered from storage var doc = sol.GetDocument(did); @@ -921,11 +876,11 @@ public void TestGetTextDoesNotKeepTextAlive() // observe the text and then wait for the references to be GC'd var observed = GetObservedText(sol, did, text); - StopObservingAndWaitForReferenceToGo(observed); + observed.AssertReleased(); } [MethodImpl(MethodImplOptions.NoInlining)] - private ObjectReference GetObservedText(Solution solution, DocumentId documentId, string expectedText = null) + private ObjectReference GetObservedText(Solution solution, DocumentId documentId, string expectedText = null) { var observedText = solution.GetDocument(documentId).GetTextAsync().Result; @@ -934,7 +889,7 @@ private ObjectReference GetObservedText(Solution solution, DocumentId documentId Assert.Equal(expectedText, observedText.ToString()); } - return new ObjectReference(observedText); + return new ObjectReference(observedText); } [MethodImpl(MethodImplOptions.NoInlining)] @@ -951,11 +906,11 @@ public void TestGetTextAsyncDoesNotKeepTextAlive() // observe the text and then wait for the references to be GC'd var observed = GetObservedTextAsync(sol, did, text); - StopObservingAndWaitForReferenceToGo(observed); + observed.AssertReleased(); } [MethodImpl(MethodImplOptions.NoInlining)] - private ObjectReference GetObservedTextAsync(Solution solution, DocumentId documentId, string expectedText = null) + private ObjectReference GetObservedTextAsync(Solution solution, DocumentId documentId, string expectedText = null) { var observedText = solution.GetDocument(documentId).GetTextAsync().Result; @@ -964,7 +919,7 @@ private ObjectReference GetObservedTextAsync(Solution solution, DocumentId docum Assert.Equal(expectedText, observedText.ToString()); } - return new ObjectReference(observedText); + return new ObjectReference(observedText); } [MethodImpl(MethodImplOptions.NoInlining)] @@ -982,14 +937,14 @@ public void TestGetSyntaxRootDoesNotKeepRootAlive() // get it async and wait for it to get GC'd var observed = GetObservedSyntaxTreeRoot(sol, did); - StopObservingAndWaitForReferenceToGo(observed); + observed.AssertReleased(); } [MethodImpl(MethodImplOptions.NoInlining)] - private ObjectReference GetObservedSyntaxTreeRoot(Solution solution, DocumentId documentId) + private ObjectReference GetObservedSyntaxTreeRoot(Solution solution, DocumentId documentId) { var observedTree = solution.GetDocument(documentId).GetSyntaxRootAsync().Result; - return new ObjectReference(observedTree); + return new ObjectReference(observedTree); } [MethodImpl(MethodImplOptions.NoInlining)] @@ -1007,14 +962,14 @@ public void TestGetSyntaxRootAsyncDoesNotKeepRootAlive() // get it async and wait for it to get GC'd var observed = GetObservedSyntaxTreeRootAsync(sol, did); - StopObservingAndWaitForReferenceToGo(observed); + observed.AssertReleased(); } [MethodImpl(MethodImplOptions.NoInlining)] - private ObjectReference GetObservedSyntaxTreeRootAsync(Solution solution, DocumentId documentId) + private ObjectReference GetObservedSyntaxTreeRootAsync(Solution solution, DocumentId documentId) { var observedTree = solution.GetDocument(documentId).GetSyntaxRootAsync().Result; - return new ObjectReference(observedTree); + return new ObjectReference(observedTree); } [MethodImpl(MethodImplOptions.NoInlining)] @@ -1069,11 +1024,11 @@ End Sub TestRecoverableSyntaxTree(sol, did); } - private void TestRecoverableSyntaxTree(Solution sol, DocumentId did, [CallerMemberName] string callingTest = null) + private void TestRecoverableSyntaxTree(Solution sol, DocumentId did) { // get it async and wait for it to get GC'd var observed = GetObservedSyntaxTreeRootAsync(sol, did); - StopObservingAndWaitForReferenceToGo(observed, dumpFileName: callingTest); + observed.AssertReleased(); var doc = sol.GetDocument(did); @@ -1092,7 +1047,7 @@ private void TestRecoverableSyntaxTree(Solution sol, DocumentId did, [CallerMemb // get it async and wait for it to get GC'd var observed2 = GetObservedSyntaxTreeRootAsync(doc2.Project.Solution, did); - StopObservingAndWaitForReferenceToGo(observed2, dumpFileName: callingTest); + observed2.AssertReleased(); // access the tree & root again (recover it) var tree2 = doc2.GetSyntaxTreeAsync().Result; @@ -1118,14 +1073,14 @@ public void TestGetCompilationAsyncDoesNotKeepCompilationAlive() // get it async and wait for it to get GC'd var observed = GetObservedCompilationAsync(sol, pid); - StopObservingAndWaitForReferenceToGo(observed); + observed.AssertReleased(); } [MethodImpl(MethodImplOptions.NoInlining)] - private ObjectReference GetObservedCompilationAsync(Solution solution, ProjectId projectId) + private ObjectReference GetObservedCompilationAsync(Solution solution, ProjectId projectId) { var observed = solution.GetProject(projectId).GetCompilationAsync().Result; - return new ObjectReference(observed); + return new ObjectReference(observed); } [MethodImpl(MethodImplOptions.NoInlining)] @@ -1142,14 +1097,14 @@ public void TestGetCompilationDoesNotKeepCompilationAlive() // get it async and wait for it to get GC'd var observed = GetObservedCompilation(sol, pid); - StopObservingAndWaitForReferenceToGo(observed); + observed.AssertReleased(); } [MethodImpl(MethodImplOptions.NoInlining)] - private ObjectReference GetObservedCompilation(Solution solution, ProjectId projectId) + private ObjectReference GetObservedCompilation(Solution solution, ProjectId projectId) { var observed = solution.GetProject(projectId).GetCompilationAsync().Result; - return new ObjectReference(observed); + return new ObjectReference(observed); } [Fact, Trait(Traits.Feature, Traits.Features.Workspace)] diff --git a/src/Workspaces/CoreTest/WorkspaceTests/MSBuildWorkspaceTests.cs b/src/Workspaces/CoreTest/WorkspaceTests/MSBuildWorkspaceTests.cs index 726f40aff5ed90df20687d5b2895b31b2904ab9f..294aa700e833a0bc960721ca468ed09ffd556834 100644 --- a/src/Workspaces/CoreTest/WorkspaceTests/MSBuildWorkspaceTests.cs +++ b/src/Workspaces/CoreTest/WorkspaceTests/MSBuildWorkspaceTests.cs @@ -2706,9 +2706,11 @@ public void DisposeMSBuildWorkspaceAndServicesCollected() Assert.Equal(true, type.ToString().StartsWith("public class CSharpClass", StringComparison.Ordinal)); Assert.NotNull(compilation); - var cacheService = new WeakReference(sol.Workspace.CurrentSolution.Services.CacheService); - var weakSolution = new WeakReference(sol); - var weakCompilation = new WeakReference(compilation); + // MSBuildWorkspace doesn't have a cache service + Assert.Null(workspace.CurrentSolution.Services.CacheService); + + var weakSolution = ObjectReference.Create(sol); + var weakCompilation = ObjectReference.Create(compilation); sol.Workspace.Dispose(); project = null; @@ -2718,13 +2720,8 @@ public void DisposeMSBuildWorkspaceAndServicesCollected() sol = null; compilation = null; - GC.Collect(); - GC.WaitForPendingFinalizers(); - GC.Collect(); - - Assert.False(cacheService.IsAlive); - Assert.False(weakSolution.IsAlive); - Assert.False(weakCompilation.IsAlive); + weakSolution.AssertReleased(); + weakCompilation.AssertReleased(); } [Fact, WorkItem(1088127, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/1088127")]