whosonfirst / go-reader

Common reader interface.
BSD 3-Clause "New" or "Revised" License
1 stars 1 forks source link

Streaming I/O #4

Closed missinglink closed 3 years ago

missinglink commented 3 years ago

I was checking out this package today and noticed that it doesn't currently support streaming I/O operations.

The new stdin scheme slurps the whole stream into an in memory buffer and then returns it, rather than returning a reader that can lazily request bytes as required.

I think this is mainly to conform to the ReadSeekCloser interface?

Things like unix pipes/network requests etc. unfortunately can't 'seek', which makes them hard to conform into this interface without buffering or dropping support for seek.

I just wanted to bring this to your attention, if we wanted to support streams larger than memory in the future (such as piping collections of features) then this pattern would be constrained by available memory.

diff --git a/stdin.go b/stdin.go
index 6a07fac..adee46a 100644
--- a/stdin.go
+++ b/stdin.go
@@ -2,11 +2,11 @@ package reader

 import (
        "bufio"
-       "bytes"
        "context"
        "github.com/whosonfirst/go-ioutil"
        "io"
        "os"
 )

 type StdinReader struct {
@@ -30,22 +30,10 @@ func NewStdinReader(ctx context.Context, uri string) (Reader, error) {
 }

 func (r *StdinReader) Read(ctx context.Context, uri string) (io.ReadSeekCloser, error) {
-
-       var b bytes.Buffer
-       wr := bufio.NewWriter(&b)
-
-       _, err := io.Copy(wr, os.Stdin)
-
-       if err != nil {
-               return nil, err
-       }
-
-       wr.Flush()
-
-       br := bytes.NewReader(b.Bytes())
-       return ioutil.NewReadSeekCloser(br)
+       return ioutil.NewReadSeekCloser(os.Stdin)
 }

 func (r *StdinReader) ReaderURI(ctx context.Context, uri string) string {
        return "-"
 }
thisisaaronland commented 3 years ago

That's much better, I agree. Send a PR and I will merge it?

thisisaaronland commented 3 years ago

This is available as of v0.8.1