Small tweaks to PipeReader behaviors#27596
Conversation
- Throw if AdvanceTo is called after completing the reader - Don't throw if Complete is called without AdvanceTo - Swapped the reading and writing exceptions - Added AdvanceReader that takes BufferSegment and int, this cleans up the API a bit as we touch SequencePosition in less places.
| _readingState.End(); | ||
| } | ||
|
|
||
| // REVIEW: We should consider cleaning up all of the allocated memory |
There was a problem hiding this comment.
I was intending to do something like this for a long time, the biggest problem is the producer that may continue writing after reader completed, so we don't want to return everything but keep a single segment around to return to the writer over and over again.
There was a problem hiding this comment.
How do we currently handle backpressure after reader completion?
There was a problem hiding this comment.
We signal the writer with IsCompleted true when the reader completes.
There was a problem hiding this comment.
What if it keeps writing? We'll just expand the pipe forever?
There was a problem hiding this comment.
Yes. That's the case today right? This doesn't fix any problems there. I originally made this thing auto advance but there's no point because if you do call advance and you consume nothing, you're in the same boat.
This line of code is supposed to handle the case where the reader is completed:
I'll add a test that does the following:
- Writes until there's backpressure
- Complete the reader
- Write again, awaitable returned should be completed since there's nobody to drain.
| ReadResult result = await _pipe.Reader.ReadAsync(); | ||
| ReadOnlySequence<byte> buffer = result.Buffer; | ||
|
|
||
| _pipe.Reader.Complete(); |
There was a problem hiding this comment.
Does the pipe get reset correctly after this?
There was a problem hiding this comment.
What do you mean? Are you asking if Reset works correctly if I complete both sides where the reader never called Advance? I can add a test for that.
|
Add default threshold here too. |
I'm not doing that in this change. |
|
@pakrym added more tests |
| AvailableMemory = default; | ||
| } | ||
|
|
||
| internal OwnedMemory<byte> OwnedMemory => _ownedMemory; |
There was a problem hiding this comment.
You could've used AvailableMemory == default instead exposing OM
|
|
||
| var startSegment = (BufferSegment)start; | ||
| var endSegment = (BufferSegment)end; | ||
| Assert.NotNull(startSegment.OwnedMemory); |
There was a problem hiding this comment.
Assert.NotEqual(default(Memory<byte>), startSegment.AvailableMemory)
There was a problem hiding this comment.
Meh, I don't trust it 😄
There was a problem hiding this comment.
It gets reset on the next line that the OM does.
* Small tweaks to PipeReader behaviors - Throw if AdvanceTo is called after completing the reader - Don't throw if Complete is called without AdvanceTo - Swapped the reading and writing exceptions - Added AdvanceReader that takes BufferSegment and int, this cleans up the API a bit as we touch SequencePosition in less places. Commit migrated from dotnet/corefx@c6018ef
3 participants
the API a bit as we touch SequencePosition in less places.
Fixes https://github.com/dotnet/corefx/issues/27467 and https://github.com/dotnet/corefx/issues/27465