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

API Proposal - Stream to Pipe Wrapper #26676

Closed
Drawaes opened this issue Jul 2, 2018 · 7 comments
Closed

API Proposal - Stream to Pipe Wrapper #26676

Drawaes opened this issue Jul 2, 2018 · 7 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.IO.Pipelines
Milestone

Comments

@Drawaes
Copy link
Contributor

Drawaes commented Jul 2, 2018

Rationale

Currently pipelines has no "endpoints" to speak of. There is a vast array of endpoints and "middleware" available in streams. In order to bridge the gap a class should be provided to wrap a stream and provide a pipe on top of it. This would provide access to the rich tapestry of streams to the new world of pipelines.

I believe that both myself @mgravell and aspnetcore have implemented these a number of times so it is worth this being a type provided by the BCL

API Shape

The following API is proposed.

namespace System.IO.Pipelines
{
    public class StreamPipe : IDuplexPipe, IDisposable
    {
        public StreamPipe(Stream stream, PipeOptions pipeOptions)
            : this(stream, pipeOptions, true) { }

        public StreamPipe(Stream stream, PipeOptions pipeOptions, bool ownsStream) { }
        
        public PipeReader Input => throw new NotImplementedException();
        public PipeWriter Output => throw new NotImplementedException();
        public void Dispose() => throw new NotImplementedException();
    }
}

Open questions

  1. Should the stream be available as a property on the wrapper?
  2. Should there be a read only write only version (single pipe not a duplex pipe?)

/cc @geoffkizer @stephentoub @mgravell @davidfowl @benaadams

Related #25087
Related #26668

@mgravell
Copy link
Member

mgravell commented Jul 2, 2018

re your 2 - indeed, not all Streams are duplex. There's also read-only streams that can be meaningfully and gainfully exposed as a PipeReader, and write-only streams blah blah PipeWriter. Any API should be very clear which is being supported; "create a StreamPipe but only use the .Input is ugly.

for my pre-existing implementation, I ended up using factory methods: CreateReader, CreateWriter, CreateDuplex etc. I'm not saying it is perfect, but the reality is that Stream is hugely ambiguous. Even testing .CanRead / .CanWrite doesn't tell you whether it is duplex (see: FileStream, MemoryStream, etc).

happy to provide my implementation for examples/discussion, but actively not linking to it yet to not poison the well

@Drawaes
Copy link
Contributor Author

Drawaes commented Jul 2, 2018

I would be happy with a StreamPipeWriter and StreamPipeReader and have this as well but I think they could be separate unless there is a drive for both in one review. This should throw if the pipe isn't writable and readable in my opinion.

@benaadams
Copy link
Member

re your 2 - indeed, not all Streams are duplex.

https://github.com/dotnet/corefx/issues/27268 "Add IPipeReader and IPipeWriter and have IDuplexPipe inherit from them" 😢

@mgravell
Copy link
Member

mgravell commented Jul 2, 2018

@benaadams I'm just going to point out the other missed opportunity here... IDuplexPipe : IDisposable. It probably should have been. But that ship has sailed, so...

@Drawaes
Copy link
Contributor Author

Drawaes commented Jul 2, 2018

In saying all that..... There aren't any endpoints so maybe it's a blessing rather than a curse?

@mgravell
Copy link
Member

mgravell commented Jul 3, 2018

In support of why we need at least a Stream wrapper, please see Pipe Dreams, part 2 - note part 1 is also relevant as a discussion of the problems that plague the Stream API, as further support for the "pipelines > stream" camp :)

@davidfowl
Copy link
Member

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.IO.Pipelines
Projects
None yet
Development

No branches or pull requests

5 participants