vbauerster / mpb

multi progress bar for Go cli applications
The Unlicense
2.29k stars 123 forks source link

Add proxywriter #52

Closed donny-dont closed 4 years ago

donny-dont commented 4 years ago

The io.Copy function has optimizations where the Reader implements io.ReadFrom.

// copyBuffer is the actual implementation of Copy and CopyBuffer.
// if buf is nil, one is allocated.
func copyBuffer(dst Writer, src Reader, buf []byte) (written int64, err error) {
    // If the reader has a WriteTo method, use it to do the copy.
    // Avoids an allocation and a copy.
    if wt, ok := src.(WriterTo); ok {
        return wt.WriteTo(dst)
    }
    // Similarly, if the writer has a ReadFrom method, use it to do the copy.
    if rt, ok := dst.(ReaderFrom); ok {
        return rt.ReadFrom(src)
    }
        ....

For example sftp.File implements io.ReadFrom. So if you attempt to download a file from sftp and wrap it with a mpb.proxyReader then io.Copy is unable to use the io.ReaderFrom optimization.

I was thinking about migrating from https://github.com/cheggaaa/pb to mpb since it has multiple progress bar support and noticed that mpb just proxies a reader. Would it be of interest to do a writer proxy as well?

vbauerster commented 4 years ago

Hello! Thanks for pointing to io.ReadFrom, indeed it wasn't accounted for. Commit 9436aa1 should fix this. If you have time and interest to implement a proxy for writer side, I'd be glad to review a PR. You mentioned sftp.File which is a good use case for proxy reader, can you think of any use case for proxy writer?

nektro commented 4 years ago

I've done this externally in a library I wrote to wrap mpb. Most relavant parts: are here: https://github.com/nektro/go-util/blob/master/mbpp/mbpp.go#L144 and here: https://github.com/nektro/go-util/blob/master/mbpp/headlessbar.go

vbauerster commented 4 years ago

I think the title of the issue is a bit misleading, because @donny-dont asked for io.WriterTo interface to be implemented, so that io.Copy can benefit, if used with internal proxy reader. As this is done I'm closing this issue.

@nektro thanks for interesting code, anyway.