Skip to content

Conversation

@manandre
Copy link
Contributor

@manandre manandre force-pushed the copy-pipe-complete branch from 07b46b2 to 12e60c4 Compare April 12, 2021 21:46

consumed = position;

if (flushResult.IsCompleted)
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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!

Copy link
Member

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).

Copy link
Member

@halter73 halter73 Apr 16, 2021

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?

Copy link
Member

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.

@davidfowl davidfowl merged commit f1bafe6 into dotnet:main Apr 17, 2021
@davidfowl
Copy link
Member

Thanks @manandre !

@manandre manandre deleted the copy-pipe-complete branch April 17, 2021 08:17
manandre added a commit to manandre/runtime that referenced this pull request May 1, 2021
See disccussion in dotnet#51147 (comment) and associated open issue dotnet#51272
@ghost ghost locked as resolved and limited conversation to collaborators May 17, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PipeReader CopyToAsync to a PipeWriter doesn't notice when the destination Pipe is closed.

5 participants