-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Handle completed FlushResult on CopyToAsync #51147
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
Conversation
07b46b2 to
12e60c4
Compare
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/PipeReader.cs
Outdated
Show resolved
Hide resolved
|
|
||
| consumed = position; | ||
|
|
||
| if (flushResult.IsCompleted) |
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.
Do we have a test for this case? If you PipeReader.CopyToAsync(PipeWriter) and that pipe writer is complete before writing all of the data, and you copy the PipeReader to another Pipewriter and it resumes where it left off and continues copying.
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 have added a test proposal. Comments are welcome.
I have checked that without the fix this test case hangs... as expected.
However, I have a doubt around the update of the consumed position when the flush result is "completed". Indeed, by design, flush comes after write so data is considered as consumed (if not failed), but the Pipe.WriteAsync method can return a completed FlushResult before consuming the provided data...
Do I miss something 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.
That's a really good point. Lets file another issue. There's basically no way to know if the data has been consumed or not. Great find!
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.
You shouldn't have this check if you're doing other important stuff in this loop (which you are).
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 was written during triage when we didn't have the full context yet. I've now read through most of the related issues.
It seems the problem is that when you have a PipeWriter/Reader pair with arbitrary Pause/ResumeThreshold, you cannot observe exactly when the read loop completed just using PipeWriter. This was already the case though. Checking FlushResult.IsCompleted doesn't change that. If anything, it helps you observe the read loop completed earlier than before.
You used by be able to use PipeWriter.OnReaderCompleted and a TCS for this, but that's been obsoleted. If we decide an API for this is important again, we'll want to directly expose a ReaderCompletionTask or something like that anyway.
Edit: I see the issue is knowing where the read loop completed, not when. Does this matter? What scenario does PipeReader.CopyToAsync() complete successfully and the app tries to copy from the same, non-reset PipeReader again?
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 think we leave this as it was the existing behavior. Assuming it was consumed.
|
Thanks @manandre ! |
See disccussion in dotnet#51147 (comment) and associated open issue dotnet#51272
Fixes #1958
/cc @davidfowl @chriswelles