Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Reduce NamedPipe Chatter (take 2) #10813
base: main
Are you sure you want to change the base?
Reduce NamedPipe Chatter (take 2) #10813
Changes from all commits
23ef2a1
9fa70b2
ba2c257
4693b2c
3179c68
54d5a07
1f9d953
14f5d71
734fab6
0dd6a2c
aeebf20
c64d110
2c0bd4d
80b6c89
bf8283c
8fda8d0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Consider taking a look at
WorkspaceProjectStateChangeDetector
in the VS layer. This class also does some extra work to avoid enqueuing work. For example, for document changes, it does work to determine whether the change will affect components and tag helpers. Those extra checks might be useful here.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.
Let me do that in a follow up. Definitely think there can be some shared code there but it might be better to move it around and unify
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.
Absolutely! I just wanted to note some thoughts while reviewing. That feedback can definitely wait until later!
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.
Call me paranoid, but I still have flashbacks to the last time a disposed flag was changed to check a cancellation token, and that didn't go well.
If I'm reading this right, the assumption is that
stream
will never be null because it's only set to null inDIspose
, and we're checking cancellation here. BUT unless I missed it, theAsyncBatchingWorkQueue
doesn't create a linked cancellation token for an individual batch, and the whole queue. I think it might be possible for us to be disposed, and not necessarily have this token be cancelled.If I'm wrong, then great, but perhaps a comment explaining why it can't happen would be good. Alternatively, just checking both tokens here might make sense.
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.
The
AsyncBatchingWorkQueue
does indeed create a linked cancellation token so that a batch will be cancelled when the main cancellation token is cancelled. It uses aCancellationSeries
to do exactly that. So, if_disposeTokenSource.Cancel()
is called, the cancellation token passed to the batch will also be in a cancelled state.Here's the relevant code in
AsyncBatchingWorkQueue
:razor/src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/AsyncBatchingWorkQueue`2.cs
Lines 105 to 138 in 07e1382
CancellationService.CreateNext()
does the work of creating a linked token source.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.
In practice, won't this be quite noisy? In a project containing razor files, wouldn't it be sent for any text change in a C# file that doesn't affect tag helpers?