-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Work around potential race in PipeWriter (IIS Edition) #11165
Conversation
aa128c4
to
e8c3d21
Compare
Got some build errors there 😮 |
e8c3d21
to
236434d
Compare
Maybe I should try building before submitting my PR next time 😆 |
But that's what The ☁️ is for ;) |
236434d
to
6a3f573
Compare
} | ||
|
||
private async Task FlushAsyncAwaited(ValueTask<FlushResult> awaitable, CancellationToken cancellationToken) | ||
private Task FlushNowAsync(PipeWriter pipeWriter, CancellationToken cancellationToken) |
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.
FlushNow is... an interesting name. I can't think of anything better.
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.
Fix looks good, let's make sure the Kestrel changes are stable.
/azp run AspNetCore-ci |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run AspNetCore-ci |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run AspNetCore-ci |
Azure Pipelines successfully started running 1 pipeline(s). |
We should wait until #11065 (the Kestrel version of this) is merged and stable. If everything looks good for the Kestrel change, and it didn't have to detrimental of an impact to our benchmarks, I think we should take this despite the lack of IIS-related customer reports so far.
@jkotalik @davidfowl @pakrym if you're still interested.