Deadlock closing solutions with CPS projects referencing Shared Projects. #14479
Comments
|
I'm pretty sure there is already a bug on @dpoeschl about this (maybe an internal one?) |
Pilchie
added
the
Tenet-Reliability
|
Another callsite for SetDocument context already ensures that it is invoked outside the lock: http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/Workspace/Workspace_Editor.cs,556. Might be a matter of just refactoring this code path to do the same... |
See dotnet/roslyn#14479 for more details.
|
See also internal bug 296432. |
|
Pulling back into RTM to investigate, since there are several reports of this. |
|
Two observations of differences between the old & new project systems. There are a few questions in there for the CPS team (@davkean @mavasani @srivatsn). FYI @Pilchie
|
dpoeschl
changed the title
|
Thanks David. I need a little more context before I can say whether we should fix something here:
|
|
Basically, I don't have enough information to make any actionable changes - it's not clear what/how we would change the project system to avoid this. |
|
@davkean Okay, I definitely should have been more clear on both fronts.
|
|
Let's Skype tomorrow afternoon and walk through the code, it's still not clear to me where/how this all works on the Roslyn side. |
|
Sorry, I looked at the stack above - you are saying that us raising OnPropertyChanged after you called SetProperty on us is causing the deadlock? If so, I get that the behavior is probably different from the legacy project system, but you just set a property on us - what would expect us to do in this situation? |
|
Can you see if https://devdiv.visualstudio.com/DevDiv/_workitems/edit/245867 is also caused by this? |
|
The way to think about this is; imagine that language service was only alive for half the lifetime of the project. We should be able to shut you down without the project system being required to close. |
|
@davkean That bug is almost certainly a dupe. |
I'd expect the same behavior as before. In the case of Project Unload, I'd expect the SharedContextHierarchy to already have been updated before the project unloads. In the case of Solution Close, I'd expect the Shared Project Hierarchy to stop listening to property sets / stop issuing events. If that's an unrealistic hope, then I can work around this deadlock (not in a very happy way, but it works), I just worry that there might be more fallout from the behavioral differences that we haven't discovered yet. |
|
Firstly, note that we have already worked around this deadlock in the project system with this commit - I don't think we need to do anything for RTM. The UnconfiguredProjectHostObject on project system side keeps track of whether or not we are currently disposing the workspace project context, and avoid attempting to raise hierarchy events during this phase, avoiding deadlock. The deadlock here is unrelated to project systems, it can happen from any project systems that:
We just need to ensure on the Roslyn side that SetDocumentContext is never invoked within a lock - we do so at all callsites except this one, and we need to fix this. |
I still see the deadlock on the most recent d15rel builds. Am I missing something? |
|
@dpoeschl - I also hit the deadlock right now when playing with MVC solution and attempting to close it(dotnet/project-system#1384) Seems like the workaround doesn't completely avoid it. Is it possible to put some defensive fix here? |
@dpoeschl This is by design. For the new project system, we support cross targeting, so you can specify multiple target frameworks for your project. We create an AbstractProject for every target framework for the same underlying project (it likely has different references, sources, etc.) and they all have the same shared hierarchy based off the underlying VS project. When the user edits the TargetFrameworks property to add or remove frameworks, we dispose off the AbstractProject which are no longer in the TargetFrameworks, but the underlying VS project (shared hierarchy) still needs to be active. I had put a workaround in CPS that whenever we are disposing the AbstractProject(s), the underlying project queue's the SetProperty changed events and fires them after the Dispose is done - it seems to not be working for all cases. |
…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).
|
@davkean @mavasani @srivatsn (FYI @Pilchie @mattwar): I still argue that the project system should be setting the new context hierarchy for shared projects before unloading the requested project. This used to be done in this stack: StackRoslyn currently only triggers the context change notification to the hierarchy if there are open documents from the shared project (the very thing that's causing the hang). So we still have a gap here even with my proposed deadlock avoidance "fix", when there are no open documents. If CPS can set the new context hierarchy itself, then I can completely avoid the reentrancy problem instead of hackily work around it. I don't think the Roslyn Workspace should be responsible for updating this. Is this particular new behavior "by design" for some reason? |
|
Workspace.ClearProjectData should not be doing anything but cleaning up additional dictionaries/collections that the individual implementation may be keeping around. Nothing should be causing events to fire here. This code is called under the state lock. It should not call into arbitrary code. |
|
Is this meant to be closed? |
|
This looks like it was accidently closed. Looking at our code on our side and we got a large complicated way of avoiding this deadlock. We shouldn't be calling this under a lock, @jasonmalinowski can put this on radar as you change this code? |
|
OK, state here is confusing. I do know @dpoeschl made a change here as observed. And we can make that better. So what's the ask here for me? Don't regress this? 😄 |
|
I'm thinking this is now fixed and going to assume it is as I change our workaround on our side. |
I am hitting a deadlock in the CPS project system with the below callstack.
ClearProjectData method on the workspace ends up invoking SetDocumentContext within a lock, which may end up invoking back into workspace causing this deadlock. The comment here indicates we shouldn't be invoking SetDocumentContext when project/solution is closing, but it seems there are code paths like this one where we do invoke it.
The text was updated successfully, but these errors were encountered: