Avoid deadlocks when unloading unloading multitargeted Asp.NET projects, or .NET Standard (CPS) Projects (directly or via Solution close) referencing Shared Projects #16889

dpoeschl · 2017-02-02T00:35:03Z

The problem here is that unloading projects takes a serialization lock,
which can cause Shared Project IVsHierarchys to notify us that their
context hierarchy has changed while the lock is still held. In response
to this, we try to set the new active context document for open files,
which also tries to take the serialization lock, and we deadlock.

For .NET Framework Projects that reference Shared Projects, two things
prevent deadlocks when projects unload. During solution close, any
Shared Projects are disconnected before the projects start to unload, so
no IVsHierarchy events are fired. During a single project unload, we
receive notification of the context hierarchy change before the project
is unloaded, avoiding any IVsHierarchy events if we tell the shared
hierarchy to set its context hierarchy to what it already is.

Neither of these behaviors are safe to rely on with .NET Standard (CPS)
or multitargeted Asp.NET projects, so we have to prevent the deadlock
ourselves. We do this by remembering if we're already in the
serialization lock due to project unload, and then not take the lock to
update document contexts if so (but continuing to lock if it's not
during a project unload).

Escrow Template

Customer scenario In a multitargeted Asp.NET project, closing the solution with open documents can cause the product to hang. In regular CPS projects (e.g. .NET Standard) that reference a Shared Project, closing the solution with open documents belonging to the Shared Project causes the product to hang. In both cases, it can hang before saving the solution.

Bugs this fixes:

#14479
[Tracking] https://devdiv.visualstudio.com/DevDiv/_workitems?_a=edit&id=296432

Workarounds, if any: Close all open documents before closing the solution.

Risk: Fairly low. We avoid taking a lock when the lock has already been taken for us.

Performance impact: Essentially none. No allocations or complexity changes.

Is this a regression from a previous update?: No. It's been broken since CPS projects started being used.

Root cause analysis: The Roslyn Workspace was getting lucky that the old project system handled Shared Projects in a way that naturally avoided this reentrancy.

How did we miss it? Everything worked fine with the old project system, so it was never noticed.

How was the bug found? Automated tests and exploratory testing by the CPS team. I don't know of any customer reports.

dpoeschl · 2017-02-02T00:55:46Z

Tagging for review @dotnet/roslyn-ide

Pilchie · 2017-02-02T00:53:56Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

@@ -424,6 +426,8 @@ protected internal virtual void OnProjectRemoved(ProjectId projectId)
                var newSolution = this.SetCurrentSolution(oldSolution.RemoveProject(projectId));

                this.RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind.ProjectRemoved, oldSolution, newSolution, projectId);
+
+                _isProjectUnloading = false;


Is anything ever expected to throw between these two points? Should this be in a finally?

+1 on the finally. If only because someone might put a return in this code in the future and completely f-up the system.

CyrusNajmabadi · 2017-02-02T01:59:50Z

src/Workspaces/Core/Portable/Workspace/Workspace_Editor.cs

@@ -22,6 +22,26 @@ public abstract partial class Workspace
        private readonly Dictionary<DocumentId, TextTracker> _textTrackers = new Dictionary<DocumentId, TextTracker>();

        /// <summary>
+        /// Unloading projects takes a serialization lock, which can cause Shared Project
+        /// IVsHierarchys to notify us that their context hierarchy has changed. In response to


i don't understand the very first statement. how is taking a serialization lock causing 'Shared Project Hierarchys to notify us that their context hierarchy has changed'?

"lock, which can cause" ==> "lock and can cause"

CyrusNajmabadi · 2017-02-02T02:00:31Z

src/Workspaces/Core/Portable/Workspace/Workspace_Editor.cs

@@ -335,7 +355,14 @@ protected void OnDocumentContextUpdated(DocumentId documentId)

            if (container != null)
            {
-                OnDocumentContextUpdated(documentId, container);
+                if (_isProjectUnloading)


reading of mutable state outside of the lock. How do we know this is safe? Do we have threading asserts about when these things will happen?

We do not have threading asserts, but they both happen on the UI thread in practice...

This code is considered free threaded, which is why there is no threading asserts. It should be made safe.

jasonmalinowski · 2017-02-02T18:20:40Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

@@ -424,6 +426,8 @@ protected internal virtual void OnProjectRemoved(ProjectId projectId)
                var newSolution = this.SetCurrentSolution(oldSolution.RemoveProject(projectId));

                this.RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind.ProjectRemoved, oldSolution, newSolution, projectId);
+
+                _isProjectUnloading = false;


jasonmalinowski · 2017-02-02T18:21:51Z

src/Workspaces/Core/Portable/Workspace/Workspace_Editor.cs

@@ -22,6 +22,26 @@ public abstract partial class Workspace
        private readonly Dictionary<DocumentId, TextTracker> _textTrackers = new Dictionary<DocumentId, TextTracker>();

        /// <summary>
+        /// Unloading projects takes a serialization lock, which can cause Shared Project


Can you name which function that lock is being taken in here? With a cref if possible? 😉

jasonmalinowski · 2017-02-02T18:23:34Z

src/Workspaces/Core/Portable/Workspace/Workspace_Editor.cs

@@ -335,7 +355,14 @@ protected void OnDocumentContextUpdated(DocumentId documentId)

            if (container != null)
            {
-                OnDocumentContextUpdated(documentId, container);
+                if (_isProjectUnloading)


This code is considered free threaded, which is why there is no threading asserts. It should be made safe.

jasonmalinowski

We now have a deadlock, unless we decide it's unimportant.

jasonmalinowski · 2017-02-02T18:46:17Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

+        /// </summary>
+        private bool _isProjectUnloading;
+
+        private object _isProjectUnloadingLock = new object();


Please add a comment explaining the lock hierarchy here.

jasonmalinowski · 2017-02-02T18:52:35Z

src/Workspaces/Core/Portable/Workspace/Workspace_Editor.cs

@@ -335,7 +335,17 @@ protected void OnDocumentContextUpdated(DocumentId documentId)

            if (container != null)
            {
-                OnDocumentContextUpdated(documentId, container);
+                lock (_isProjectUnloadingLock)


This is now a deadlock. If OnProjectRemoved takes the serialization lock but hadn't yet taken _isProjectUnloadingLock, and then this acquires this lock here, we will call OnDocumentContextUpdated and try to take the serialization lock. The first thread now can't acquire _isProjectUnloadingLock and we're toasted.

Code changes since last review.

dpoeschl · 2017-02-02T19:16:24Z

Hold off on reviewing for a minute. I forgot how to ThreadLocal but now I remember.

jasonmalinowski · 2017-02-02T19:21:24Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

@@ -58,6 +78,7 @@ protected Workspace(HostServices host, string workspaceKind)
        {
            _primaryBranchId = BranchId.GetNextId();
            _workspaceKind = workspaceKind;
+            _isProjectUnloading.Value = false;


Oops you're right this is broken.

dpoeschl · 2017-02-02T19:25:37Z

Okay, good to review again. @jasonmalinowski @CyrusNajmabadi @Pilchie @dotnet/roslyn-ide

jasonmalinowski · 2017-02-02T19:26:46Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

@@ -50,6 +50,26 @@ public abstract partial class Workspace : IDisposable
        private Action<string> _testMessageLogger;

        /// <summary>
+        /// Unloading projects takes a serialization lock, but can also cause Shared Project


Can you change this to a cref to the OnProjectRemoved since that's the operation here from the perspective of this type?

jasonmalinowski · 2017-02-02T19:27:24Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

@@ -50,6 +50,26 @@ public abstract partial class Workspace : IDisposable
        private Action<string> _testMessageLogger;

        /// <summary>
+        /// Unloading projects takes a serialization lock, but can also cause Shared Project
+        /// IVsHierarchys to notify us that their context hierarchy has changed. In response to


"...that their context hierarchy has changed while we are holding the lock." since that's the CS101 rule we're violating here.

…a Solution close) referencing Shared Projects Fixes dotnet#14479 The problem here is that unloading projects takes a serialization lock, which can cause Shared Project IVsHierarchys to notify us that their context hierarchy has changed. In response to this, we try to set the new active context document for open files, which also tries to take the serialization lock, and we deadlock. For .NET Framework Projects that reference Shared Projects, two things prevent deadlocks when projects unload. During solution close, any Shared Projects are disconnected before the projects start to unload, so no IVsHierarchy events are fired. During a single project unload, we receive notification of the context hierarchy change before the project is unloaded, avoiding any IVsHierarchy events if we tell the shared hierarchy to set its context hierarchy to what it already is. Neither of these behaviors are safe to rely on with .NET Standard projects, so we have to prevent the deadlock ourselves. We do this by remembering if we're already in the serialization lock due to project unload, and then not take the lock to update document contexts if so (but continuing to lock if it's not during a project unload).

dpoeschl · 2017-02-02T23:04:27Z

@Pilchie @CyrusNajmabadi I have two signoffs, but you both showed interest in this so I want to give you a chance to opine before moving forward.

Pilchie · 2017-02-02T23:37:41Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

+        /// the serialization lock due to project unload, and then not take the lock to update 
+        /// document contexts if so (but continuing to lock if it's not during a project unload).
+        /// </summary>
+        private ThreadLocal<bool> _isProjectUnloading = new ThreadLocal<bool>(() => false);


It's still not obvious to me why this needs to be a ThreadLocal. Can you clarify?

mattwar

This is seriously a bad idea.

mattwar · 2017-02-02T23:50:19Z

There shouldn't be any reentrancy at the workspace layer regardless of unloading or not. Why is there a deadlock? What code is causing the loop back to the lock such that you need to hack it to let it execute? That code is the problem.

Pilchie · 2017-02-03T01:43:07Z

@mattwar can you and @dpoeschl discuss this tomorrow morning? This is a frequently reported hang, and we are running out of time to address it.

dpoeschl · 2017-02-03T19:14:19Z

Talked to @mattwar about this this morning. We have ideas for how to do it better, but agreed it's acceptable to go with this for now. We'll leave #14479 open after this workaround to track the work of doing it in a less hacky way.

mattwar

On condition that we do the right fix after RTM.

dpoeschl · 2017-02-03T19:17:58Z

Tagging @MattGertz for Escrow consideration.

natidea · 2017-02-03T22:25:49Z

Approved to merge via 296432

dpoeschl added the Area-IDE label Feb 2, 2017

dnfclas added the cla-already-signed label Feb 2, 2017

Pilchie added this to the 2.0 (RTM) milestone Feb 2, 2017

Pilchie previously approved these changes Feb 2, 2017

View reviewed changes

CyrusNajmabadi reviewed Feb 2, 2017

View reviewed changes

jasonmalinowski reviewed Feb 2, 2017

View reviewed changes

dpoeschl force-pushed the NoCPSSharedProjectDeadlocks branch from 8d620be to 9a8f5d9 Compare February 2, 2017 18:44

jasonmalinowski previously requested changes Feb 2, 2017

View reviewed changes

dpoeschl force-pushed the NoCPSSharedProjectDeadlocks branch from 9a8f5d9 to db4350c Compare February 2, 2017 19:13

dpoeschl added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Feb 2, 2017

jasonmalinowski previously approved these changes Feb 2, 2017

View reviewed changes

dpoeschl force-pushed the NoCPSSharedProjectDeadlocks branch from db4350c to 6323e91 Compare February 2, 2017 19:24

dpoeschl removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Feb 2, 2017

jasonmalinowski approved these changes Feb 2, 2017

View reviewed changes

jmarolf approved these changes Feb 2, 2017

View reviewed changes

dpoeschl force-pushed the NoCPSSharedProjectDeadlocks branch from 6323e91 to e2cc9b6 Compare February 2, 2017 22:42

Pilchie reviewed Feb 2, 2017

View reviewed changes

mattwar requested changes Feb 2, 2017

View reviewed changes

mattwar approved these changes Feb 3, 2017

View reviewed changes

MattGertz added the Needs Shiproom Approval label Feb 3, 2017

natidea added the Approved to merge label Feb 3, 2017

jmarolf merged commit bcd4c9d into dotnet:master Feb 3, 2017

CyrusNajmabadi mentioned this pull request Feb 4, 2017

Visual Studio 2017 RC hangs on closing VF# solution dotnet/fsharp#2022

Closed

mavasani mentioned this pull request Feb 6, 2017

Solution with 56 projects is unsable once migrated to csproj. dotnet/project-system#1384

Closed

dpoeschl mentioned this pull request Feb 24, 2017

Avoid deadlock on solution close with linked files #17372

Merged

dpoeschl mentioned this pull request Mar 14, 2017

Avoid deadlock on solution close with linked files #17845

Merged

davkean mentioned this pull request Mar 23, 2018

Deadlock closing solutions with CPS projects referencing Shared Projects. #14479

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid deadlocks when unloading unloading multitargeted Asp.NET projects, or .NET Standard (CPS) Projects (directly or via Solution close) referencing Shared Projects #16889

Avoid deadlocks when unloading unloading multitargeted Asp.NET projects, or .NET Standard (CPS) Projects (directly or via Solution close) referencing Shared Projects #16889

dpoeschl commented Feb 2, 2017 •

edited

Loading

dpoeschl commented Feb 2, 2017

Pilchie Feb 2, 2017

CyrusNajmabadi Feb 2, 2017

jasonmalinowski Feb 2, 2017

CyrusNajmabadi Feb 2, 2017

dpoeschl Feb 2, 2017

CyrusNajmabadi Feb 2, 2017

dpoeschl Feb 2, 2017

jasonmalinowski Feb 2, 2017

jasonmalinowski Feb 2, 2017

jasonmalinowski Feb 2, 2017

jasonmalinowski Feb 2, 2017

jasonmalinowski left a comment

jasonmalinowski Feb 2, 2017

jasonmalinowski Feb 2, 2017

dpoeschl commented Feb 2, 2017 •

edited

Loading

jasonmalinowski Feb 2, 2017

dpoeschl commented Feb 2, 2017

jasonmalinowski Feb 2, 2017

jasonmalinowski Feb 2, 2017

dpoeschl commented Feb 2, 2017

Pilchie Feb 2, 2017

mattwar left a comment

mattwar commented Feb 2, 2017

Pilchie commented Feb 3, 2017

dpoeschl commented Feb 3, 2017 •

edited

Loading

mattwar left a comment

dpoeschl commented Feb 3, 2017

natidea commented Feb 3, 2017

Avoid deadlocks when unloading unloading multitargeted Asp.NET projects, or .NET Standard (CPS) Projects (directly or via Solution close) referencing Shared Projects #16889

Avoid deadlocks when unloading unloading multitargeted Asp.NET projects, or .NET Standard (CPS) Projects (directly or via Solution close) referencing Shared Projects #16889

Conversation

dpoeschl commented Feb 2, 2017 • edited Loading

Escrow Template

dpoeschl commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpoeschl commented Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

dpoeschl commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpoeschl commented Feb 2, 2017

Choose a reason for hiding this comment

mattwar left a comment

Choose a reason for hiding this comment

mattwar commented Feb 2, 2017

Pilchie commented Feb 3, 2017

dpoeschl commented Feb 3, 2017 • edited Loading

mattwar left a comment

Choose a reason for hiding this comment

dpoeschl commented Feb 3, 2017

natidea commented Feb 3, 2017

dpoeschl commented Feb 2, 2017 •

edited

Loading

dpoeschl commented Feb 2, 2017 •

edited

Loading

dpoeschl commented Feb 3, 2017 •

edited

Loading