Work to unify threading concepts between Workspace and ProjectSystemProjectFactory (part1) #67522
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| Workspace.ClearDocumentData(documentId); | ||
| }, | ||
| onAfterUpdate: null, | ||
| CancellationToken.None).ConfigureAwait(false); |
There was a problem hiding this comment.
this change at least moves this specific PSPF to use the workspace helper that makes stateless transformations against the it's current-snapshot and uses optimistic concurrency to try to apply it if nothing else intervened. If anything else did, it will back off and continue trying until it succeeds. Only once it does does it then update the current solution and then calls the other lambdas which can make stateful changes.
The goal is to move everything in this type over to that pattern. HOwever, it is difficult given the disparate patterns used here, as well as that this type is itself then called by other external entities making many changes and expecting them all to be atomic. It's going to take a fair amount of work to untangle everything.
There was a problem hiding this comment.
Yep, refactoring this one method at a time is going to be the right approach.
| WorkspaceChangeKind.SolutionRemoved, | ||
| onBeforeUpdate: (_, _) => | ||
| { | ||
| Workspace.ClearOpenDocuments(); |
There was a problem hiding this comment.
an example of moving mutation to the point that we know that setting the actual solution succeeded. It was previously possible for these two calls to have interleaving mutations run from some other source
| (_, _) => changeKind, | ||
| projectId, | ||
| documentId, | ||
| (_, _) => (changeKind, projectId, documentId), |
There was a problem hiding this comment.
this was just a simplifying transformation that made other changes/callers simpler.
| Action<Solution, Solution>? onBeforeUpdate, | ||
| Action<Solution, Solution>? onAfterUpdate, | ||
| CancellationToken cancellationToken) | ||
| { |
There was a problem hiding this comment.
general change here is that these helpers became async-capable, with a boolean flag to actually say if they can operate asynchronously or not. Existing sync callers pass 'false', but then know that they will complete synchronously. This is a pattern we use in many places in teh workspace already.
CyrusNajmabadi
changed the title
|
Agreed. This isn't merging until jason is ok with things. |
|
@jasonmalinowski ptal :) |
|
@jasonmalinowski I'd like a review from you on this. If you aren't able to, let me know and i'll merge and you can review at a better time for you later. |
|
@jasonmalinowski ptal. |
|
@jasonmalinowski ptal. |
1 similar comment
|
@jasonmalinowski ptal. |
| CancellationToken.None); | ||
| #pragma warning restore CA2012 // Use ValueTasks correctly | ||
|
|
||
| Contract.ThrowIfFalse(valueTask.IsCompleted, "Task must have completed synchronously as we passed 'useAsync: false' to SetCurrentSolutionAsync"); |
There was a problem hiding this comment.
For regular tasks, we wrote an extension method here for asserting this:
Want to write an equivalent "VerifyCompleteAndReturn" form for ValueTask? If nothing else, if you can do it where the you can keep the pragma disables to the helper it might make it look nicer.
There was a problem hiding this comment.
done. that is nice :)
| // If the accumulator showed no changes, return the old solution. This will ensure that | ||
| // SetCurrentSolutionAsync bails out immediately and no further work is done. | ||
| return solutionChanges.HasChange ? solutionChanges.Solution : oldSolution; |
There was a problem hiding this comment.
Was this check needed? I would have assumed that if HasChange returns false, solutionChanges.Solution already equals oldSolution...
There was a problem hiding this comment.
you are correct. simplified.
| Func<Solution, Solution, WorkspaceChangeKind> changeKind, | ||
| ProjectId? projectId = null, | ||
| DocumentId? documentId = null, | ||
| Func<Solution, Solution, (WorkspaceChangeKind changeKind, ProjectId? projectId, DocumentId? documentId)> changeKind, |
There was a problem hiding this comment.
I wonder if it makes sense just to have the transformation also return the change info too. But that can be another PR.
There was a problem hiding this comment.
that's a reasonable idea. thanks!
ghost
added this to the 
This is some preliminary work to help fix #67510. There is a lot more work to be done, but this creates an initial foundation to safely and incrementally move ProjectSystemProjectFactory and workspace off of having two separate locks that have to be coordinated, to just using a single lock that ensures consistent atomic changes to the workspace across all clients.
The core problem we want to solve is that currently it is possible to corrupt the workspace if it is within a host that both uses ProjectSystemProjectFactory to manipulate the workspace and which attempts to mutate the workspace through any other means. This can happen as the ProjectSystemProjectFactory expects all workspace edits to be done while a particular lock it owns is held. But the host can directly interact with the workspace, changing it out from underneath ProjectSystemProjectFactory while it is in the middle of making a series of changes.
Moving to a consistent change-application mechanism ensures that the workspace changes from the host and ProjectSystemProjectFactory happen atomically, and either mutator only ever sees complete changes from the other.