Avoid having to take locks repeatedly when cleaning out workspace #62986
Conversation
|
@CyrusNajmabadi What was the motivation here? Perf benefit? |
| using (_stateLock.DisposableWait()) | ||
| { | ||
| docIds = _projectToOpenDocumentsMap.Values.SelectMany(x => x).ToList(); | ||
| } |
There was a problem hiding this comment.
we would initially grab the lock (in a non-cancellable fashion) just to grab a copy of the list of IDs. We'd then go an have to process each ID, grabbing the lock again, and mutating the workspace. This means N+1 acquisitions of the lock. It also opens things up for races (imo) because we now may be clearing out open-document-info while it's possible the doc to get reopened which will then update the info we then clear from thsi thread. While presumably very unlikely, this gives me the oogies.
NOte: auditing all the code in ClearOpenDocument_NoLock (and below), the work we do is all very lightweight (just clearing out collections, and disconnecting event handlers), so it seems reasonable to hold the lock the entire time while doing this. Fine-grained locking doesn't seem to help us here.
There was a problem hiding this comment.
@CyrusNajmabadi If I had to guess why this might have been done (and to be very clear: I don't like what I'm about to write), is clearing documents for a specific project potentially meant that an open file that was using that project as its context meant we had to go change the context to some other open project, and then we ended up calling into VS APIs and deadlocked. But I think I fixed that awhile back because it meant removing projects wasn't thread-safe and we wanted that as a part of #26931.
So we should be on the lookout for deadlocks here, but if we find one we should just fix it.
Primarily (as well as the comment i made about fear of race conditions). The reason for this is that we call through this codepath in RemoteWorkspace when synchronizing oop. In the RemoteWorkspace itself i need to take another lock while i'm updating sttae there, and then we call into the normal Workspace code to do this clearing. I want to ensure that these locks are extremely short lived and ideally just get grabbed, do their work, and immediately get back out again. The fine-grained locking here where the inner lock may get taken N+1 times means that hte outer lock has the potential for much more waiting, which def worries me. This basically gives me higher confidence that we should not have to worry about locking here and that the different workspace layers are grabbing and holding locks the minimal number of times necessary for the minimal amount of time necessary as well. |
| using (_stateLock.DisposableWait()) | ||
| { | ||
| docIds = _projectToOpenDocumentsMap.Values.SelectMany(x => x).ToList(); | ||
| } |
There was a problem hiding this comment.
@CyrusNajmabadi If I had to guess why this might have been done (and to be very clear: I don't like what I'm about to write), is clearing documents for a specific project potentially meant that an open file that was using that project as its context meant we had to go change the context to some other open project, and then we ended up calling into VS APIs and deadlocked. But I think I fixed that awhile back because it meant removing projects wasn't thread-safe and we wanted that as a part of #26931.
So we should be on the lookout for deadlocks here, but if we find one we should just fix it.
|
Ok. if we think this is risky i can hold off on when thsi goes in. |
|
@CyrusNajmabadi Is this obsolete given your other changes, or still worth getting in? |
|
Going to move to draft for now. |
Found while debugging through remote OOP sync code.