Skip to content
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

Provide a way to get a PipeReader from a ReadOnlySequence<byte> #47990

Closed
Alxandr opened this issue Feb 8, 2021 · 7 comments · Fixed by #48369
Closed

Provide a way to get a PipeReader from a ReadOnlySequence<byte> #47990

Alxandr opened this issue Feb 8, 2021 · 7 comments · Fixed by #48369
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.IO.Pipelines
Milestone

Comments

@Alxandr
Copy link
Contributor

Alxandr commented Feb 8, 2021

Background and Motivation

Sometimes you have data available in memory (either already downloaded or read from file or generated by your process), represented as a ReadOnlySequence<byte> (which may or may not be a single contiguous Span<byte>), and you need to feed that into an API that expects a PipeReader. This is basically the same usecase as a MemoryStream over a byte[], except it's in the new APIs of Pipes and ReadOnlySequence<byte>s. The exact signature for this API isn't too terribly important (wether it's an extension method on ReadOnlySequence<byte>, an overload of PipeReader.Create or new MemoryPipeReader(sequence)), since this template requires a proposed API, I will be writing the static Create, as that is closest to what exist for Stream.

Proposed API

namespace System.IO.Pipelines
{
    public abstract class PipeReader
    {
+       public static PipeReader Create(ReadOnlySequence<byte> sequence);

Usage Examples

var data = GenerateDataAsSequence();
var obj = await DeserializeAsync(PipeReader.Create(data));

Alternative Designs

Using the package Nerdbank.Streams, it's possible to achieve a similar result going through a Stream;

var data = GenerateDataAsSequence();
var obj = await DeserializeAsync(PipeReader.Create(data.AsStream()));

Alternatively, the fake "generate" method here needs to be rewritten to take a writer:

var pipe = new Pipe();
var task = GenerateDataIntoPipe(pipe.Writer);
var obj = await DeserializeAsync(pipe.Reader);
await task;

While this is probably the better solution in many cases, you don't always have access to rewrite the APIs that generate the data.

Risks

The only risk I can see is an expanded API surface area to teach/maintain.

@Alxandr Alxandr added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 8, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.IO.Pipelines untriaged New issue has not been triaged by the area owner labels Feb 8, 2021
@BrennanConroy BrennanConroy added this to the Future milestone Feb 8, 2021
@BrennanConroy BrennanConroy removed the untriaged New issue has not been triaged by the area owner label Feb 8, 2021
@BrennanConroy
Copy link
Member

Triage: This would definitely make some of the tests in AspNetCore nicer.

You should be able to write an extension method for this and party on that.

@Alxandr
Copy link
Contributor Author

Alxandr commented Feb 9, 2021

I have written one of these for our tests actually, however it contains some extra features for testing purposes, like being able to simulate "network delays" (not actual delays, but how the data is split up for the reads). I use this to test APIs that consume PipeReaders to ensure they are agnostic with regards to where any buffer boundaries ends up being, so I run tests with a few different strategies ranging from "give all data on the first read" to "give a single byte more for every call to read". That's very useful for testing, this however I imagine would be useful whenever you have a data in memory already (for instance if you've downloaded a pipe already, and want to reuse the data somewhere that expects a pipe).

@davidfowl
Copy link
Member

I think the API as proposed is good, I don't think it should be an extension method as the pattern we have so far introduced is a factory method.

@davidfowl davidfowl modified the milestones: Future, 6.0.0 Feb 11, 2021
@davidfowl davidfowl added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Feb 11, 2021
@davidfowl
Copy link
Member

@Alxandr once the API is approved would you be OK doing the work?

@Alxandr
Copy link
Contributor Author

Alxandr commented Feb 11, 2021

I could probably do that, sure :).

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 16, 2021
@terrajobst
Copy link
Member

terrajobst commented Feb 16, 2021

Video

  • We could also overloads to create a reader straight from a byte[] or ReadOnlyMemory<byte> but those can be constructed by creating a sequence, so we can add them later if we think that's common enough.
  • We don't believe this method needs any options
namespace System.IO.Pipelines
{
    public abstract class PipeReader
    {
        public static PipeReader Create(ReadOnlySequence<byte> sequence);
    }
}

@davidfowl
Copy link
Member

@Alxandr the API's been approved! Have at it. I'll gladly review the implementation.

Alxandr added a commit to Alxandr/runtime that referenced this issue Feb 16, 2021
Add new factory method to PipeReader to create a PipeReader from a
ReadOnlySequence<byte>.

Fix dotnet#47990
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 16, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 18, 2021
@davidfowl davidfowl self-assigned this Mar 18, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.IO.Pipelines
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants