-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework ProjectSnapshotManagerDispatcher to avoid creating a dedicated thread #9984
Changes from 1 commit
246db4a
b1cc8e4
b79a2ae
e9cf196
bd43542
776fb79
6e04ffb
cbccdcb
a2d1fa4
d8e2d94
806ce2b
38a4cb0
6000ada
8e9bd62
89b9d02
881404e
ba1b78f
4ad44e3
41ad40c
9efea52
d4f3fc4
64f850e
5f45f41
57c315c
d268749
13fefbe
4520cd1
4763864
23c4e5a
583ae4a
c09eace
db0560f
ddea43c
8d4b9f3
1c3d0c3
76be965
a4f828e
2e8ea76
a760a34
501ffae
c7fe0d9
8fc1d8e
f4ce9af
97852da
39976f5
80ea498
06734a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -381,26 +381,16 @@ internal override void ReportError(Exception exception, ProjectKey projectKey) | |
|
||
private void NotifyListeners(IProjectSnapshot? older, IProjectSnapshot? newer, string? documentFilePath, ProjectChangeKind kind) | ||
{ | ||
// Change notifications should always be sent on the dispatcher. | ||
_dispatcher.AssertDispatcherThread(); | ||
|
||
NotifyListeners(new ProjectChangeEventArgs(older, newer, documentFilePath, kind, IsSolutionClosing)); | ||
} | ||
|
||
// virtual so it can be overridden in tests | ||
protected virtual void NotifyListeners(ProjectChangeEventArgs e) | ||
{ | ||
// For now, consumers of the Changed event often assume the threaded | ||
// behavior and can error. Once that is fixed we can remove this. | ||
// https://github.com/dotnet/razor/issues/9162 | ||
if (_dispatcher.IsDispatcherThread) | ||
{ | ||
NotifyListenersCore(e); | ||
} | ||
else | ||
{ | ||
_ = _dispatcher.RunOnDispatcherThreadAsync(() => | ||
{ | ||
NotifyListenersCore(e); | ||
}, CancellationToken.None); | ||
} | ||
NotifyListenersCore(e); | ||
} | ||
|
||
private void NotifyListenersCore(ProjectChangeEventArgs e) | ||
|
@@ -449,6 +439,9 @@ private bool TryChangeEntry_UsesLock( | |
return false; | ||
} | ||
|
||
// We're about to mutate, so assert that we're on the dispatcher thread. | ||
_dispatcher.AssertDispatcherThread(); | ||
|
||
var state = ProjectState.Create( | ||
_projectEngineFactoryProvider, | ||
projectAddedAction.HostProject, | ||
|
@@ -485,6 +478,10 @@ private bool TryChangeEntry_UsesLock( | |
{ | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
|
||
using (upgradeableLock.EnterWriteLock()) | ||
{ | ||
if (newEntry is null) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.