Skip to content

Add PipeReader.Create from ReadOnlySequence<byte> #48369

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Mar 18, 2021
Merged

Add PipeReader.Create from ReadOnlySequence<byte> #48369

merged 6 commits into from
Mar 18, 2021

Conversation

Alxandr
Copy link
Contributor

@Alxandr Alxandr commented Feb 16, 2021

Add new factory method to PipeReader to create a PipeReader from a ReadOnlySequence.

Fix #47990

@Alxandr
Add PipeReader.Create from ReadOnlySequence<byte>
927e2d0 
Add new factory method to PipeReader to create a PipeReader from a
ReadOnlySequence<byte>.

Fix dotnet#47990
@ghost
Copy link

ghost commented Feb 16, 2021

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@Alxandr
Copy link
Contributor Author

Alxandr commented Feb 16, 2021

I'm going to go through this again in a couple of days but I believe this is mostly done and ready for review.

@davidfowl said you wanted to take a look at it?

I've mostly copied tests from the stream pipe reader implementation, and removed tests that weren't applicable (like testing that the stream is disposed, etc.)

davidfowl
davidfowl reviewed Feb 16, 2021
View reviewed changes
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/SequencePipeReader.cs Outdated

private bool TryReadInternal(CancellationTokenSource source, out ReadResult result)
{
bool isCancellationRequested = source.IsCancellationRequested;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the cancellation logic is needed.

davidfowl
davidfowl reviewed Feb 16, 2021
View reviewed changes
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/SequencePipeReader.cs Outdated
return false;
}

private void ClearCancellationToken()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same (not needed)

@Alxandr
Simplify cancel behaviour
74c7315 
Remove cancellation token source and replace with atomic int operations
to keep track of whether or not a cancellation has been requested.
Alxandr
Alxandr commented Feb 18, 2021
View reviewed changes
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/PipeReader.cs
}

/// <summary>
/// Creates a <see cref="PipeReader"/> wrapping the specified <see cref="ReadOnlySequence{T}"/>.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this fine? It points to the docs for ReadOnlySequence<T>, with no indication of it being only for bytes. Is this just how things are?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that how the other docs are?

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 tried to google this issue (because you can't do ReadOnlySequence<byte>), and the idea here is that there is no docs page to link to for ReadOnlySequence<byte>, but the is for ReadOnlySequence<T>, so that is why I believe. I'm not sure where else I would find docs like this in .NET as I can't recall any off the top of my head at least.

@Alxandr

This comment has been minimized.

Sign in to view
@Alxandr Alxandr marked this pull request as ready for review February 18, 2021 20:33
@Alxandr Alxandr requested a review from davidfowl February 26, 2021 13:10
Base automatically changed from master to main March 1, 2021 09:07
@davidfowl
Copy link
Member

This looks good.

@Alxandr @halter73
Use @halter73's suggestion for clarity
034e1c3 
Use `ReadOnlySequence<byte>.Empty` instead of `_sequence` when `_sequence` is known to be empty.

Co-authored-by: Stephen Halter <[email protected]>
@davidfowl
Copy link
Member

Arg, now there's a conflict. Sorry @Alxandr . Can you resolve it?

@Alxandr
Copy link
Contributor Author

Alxandr commented Mar 17, 2021

Yep. Will do :)

@Alxandr @stephentoub
Apply suggestions from code review
e4fc411 
Replace `var` with `bool` and `ReadResult` in 2 places, and make `SequencePipeReader` sealed.

Co-authored-by: Stephen Toub <[email protected]>
@davidfowl
Copy link
Member

@Alxandr still need to fix the merge conflict

@Alxandr
Copy link
Contributor Author

Alxandr commented Mar 17, 2021

@Alxandr still need to fix the merge conflict

I know. I just had to be on a machine with a dev environment set up for that. The previous pushes has just been done through GitHub web UI.

@davidfowl
Copy link
Member

/azp run runtime-staging

@davidfowl
Copy link
Member

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Alxandr
Copy link
Contributor Author

Alxandr commented Mar 18, 2021

I'm not sure whether or not these errors are related to code I did or not? I'm not seeing what went wrong in ADO (though, I'm probably just reading it wrong).

@davidfowl
Copy link
Member

Errors are unrelated.

@davidfowl davidfowl mentioned this pull request Mar 18, 2021
Closed
@davidfowl davidfowl merged commit dd61f0c into dotnet:main Mar 18, 2021
@davidfowl
Copy link
Member

Thanks @Alxandr !

This was referenced Mar 22, 2021
Closed
Closed
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Apr 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.
Reviewers

@halter73 halter73 halter73 approved these changes

@stephentoub stephentoub stephentoub left review comments

@davidfowl davidfowl davidfowl approved these changes

Assignees
No one assigned
Labels
area-System.IO.Pipelines new-api-needs-documentation
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Provide a way to get a PipeReader from a ReadOnlySequence<byte>
5 participants
@Alxandr @davidfowl @halter73 @stephentoub @karelz