Rework ProjectSnapshotManagerDispatcher to avoid creating a dedicated thread #9984
Conversation
The dispatcher is used to ensure that updates to the project snapshot manager are applied in order. So, the project snapshot manager should assert that it is running on the dispatcher thread whenever it is mutating state or firing notifications.
TestProjectSnapshotManagerDispatcher is used by two test fixture base classes that have been superceded by `VisualStudioTestBase` and `VisualStudioWorkspaceTestBase`. So, removing the test dispatcher is achieved by updating tests to inherit from the newer base classes.
| } | ||
| else | ||
| { | ||
| _ = _dispatcher.RunOnDispatcherThreadAsync(() => |
There was a problem hiding this comment.
I'm slightly concerned with this going away, since this method was originally written without it, and it had to come back due to bugs. Perhaps we can move this to a test accessor somehow, and make the method not protected (or realistically, get rid of it since it is a one-liner), and then we'd know we can trust the assert in the method above?
(also I'm reviewing commit-at-a-time, and this is the first commit, so my whole comment could be moot)
There was a problem hiding this comment.
This can go away because the only callers assert that they're running on the dispatcher. I've run integration tests and all seems to be working fine.
| @@ -485,6 +478,10 @@ private static Func<Task<TextAndVersion>> CreateTextAndVersionFunc(TextLoader te | |||
| { | |||
| oldSnapshot = entry.GetSnapshot(); | |||
| newSnapshot = newEntry?.GetSnapshot() ?? oldSnapshot; | |||
|
|
|||
| // We're about to mutate, so assert that we're on the dispatcher thread. | |||
| _dispatcher.AssertDispatcherThread(); | |||
There was a problem hiding this comment.
I guess it doesn't hurt to assert close to the mutation, but given its not an async method, why not just assert once at the top of the method, rather than twice? Also, this whole thing is in a lock, and I thought the lock was added to remove the dispatcher requirement. Does this PR change anything in that regard? Or will we just always have both?
There was a problem hiding this comment.
The dispatcher is still a requirement until it is removed. Essentially, it's an all or nothing situation. If nothing runs on the dispatcher, mutations are pushed directly onto the project snapshot manager queue. If everything runs on the dispatcher, mutations are pushed onto the dispatcher queue and are pushed onto the project snapshot manager queue later. However, if some mutations go directly to the project snapshot manager and some go to the dispatcher, mutations are likely to get processed in the wrong order because those on the dispatcher will be handled later.
There was a problem hiding this comment.
The addition of the lock allowed us to retrieve snapshot information without having to use the dispatcher, which was the win there. It looks like this just makes sure mutation happens on the dispatcher thread. That's not technically required from the perspective of the snapshot manager, but is logically required by dependents of snapshot changes. That's the part that will need to be cleaned up before we can completely remove
There was a problem hiding this comment.
That's not technically required from the perspective of the snapshot manager.
It might not be required from the project snapshot manager's perspective, but it's required by the current system. Everything that calls mutation methods of the project snapshot manager does so by scheduling the work on the dispatcher. By placing assertions here, I found a race condition where one component (the FallbackProjectManager) was updating the project snapshot manager directly without going through the dispatcher, before other components that had already queued updates on the dispatcher.
There was a problem hiding this comment.
The addition of the lock allowed us to retrieve snapshot information without having to use the dispatcher, which was the win there.
Yes! And I really appreciate your work on this! The separation provided by the ReaderWriterLock and the refactoring you did made things so much clearer.
Can you elaborate? I tried to find what was fixed but didn't quite find it in the commit history |
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSnapshotManagerDispatcher.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSnapshotManagerDispatcher.cs
Show resolved
Hide resolved
| @@ -485,6 +478,10 @@ private static Func<Task<TextAndVersion>> CreateTextAndVersionFunc(TextLoader te | |||
| { | |||
| oldSnapshot = entry.GetSnapshot(); | |||
| newSnapshot = newEntry?.GetSnapshot() ?? oldSnapshot; | |||
|
|
|||
| // We're about to mutate, so assert that we're on the dispatcher thread. | |||
| _dispatcher.AssertDispatcherThread(); | |||
There was a problem hiding this comment.
The addition of the lock allowed us to retrieve snapshot information without having to use the dispatcher, which was the win there. It looks like this just makes sure mutation happens on the dispatcher thread. That's not technically required from the perspective of the snapshot manager, but is logically required by dependents of snapshot changes. That's the part that will need to be cleaned up before we can completely remove
The two product bugs were fixed in #9978. |
Fixes #4393
Fixes #5683
This is a pretty significant change that affected a lot of unit tests, so I was very meticulous with my commit history. If you're looking to review the "meat" of the PR, you can skip any commits that begin with "clean up". If you only want to review the significant product code changes, you'll find them in:
DefaultProjectSnapshotManagerProjectSnapshotManagerDispatcherimplementations onIErrorReporter.ProjectSnapshotManagerDispatcherandProjectSnapshotManagerDispatcherBaseAll of the other commits are related to updating tests or benchmarks. I've removed all custom test implementations of
ProjectSnapshotManagerDispatcherthat would cause tests to cheat and assume they were always running on the dispatcher thread. Instead, all tests now use a real dispatcher and model our threading behavior more faithfully. This work revealed a couple of product bugs that I've fixed along the way.