vbauerster / mpb

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

Old progressbars are removed too late #21

Closed Naatan closed 6 years ago

Naatan commented 6 years ago

Take the following sample code:

package main

import (
    "fmt"
    "io"
    "net/http"
    "os"
    "path/filepath"
    "strconv"

    "github.com/vbauerster/mpb"
    "github.com/vbauerster/mpb/decor"
)

func main() {
    workers := 3
    tiny := "https://cdn.jsdelivr.net/npm/jquery@3.2.1/dist/jquery.min.js"
    urls := []string{tiny, tiny, tiny, tiny, tiny, tiny, "http://ipv4.download.thinkbroadband.com/512MB.zip"}

    jobs := make(chan string, len(urls))
    done := make(chan bool, workers)

    p := mpb.New(mpb.WithWidth(64))

    for w := 1; w <= workers; w++ {
        go func(jobs <-chan string) {
            for entry := range jobs {
                download(entry, p)
            }
            done <- true
        }(jobs)
    }

    for _, url := range urls {
        jobs <- url
    }
    close(jobs)

    for w := 1; w <= workers; w++ {
        <-done
    }

    p.Wait()

    fmt.Println("done")
}

func download(url string, p *mpb.Progress) {
    resp, err := http.Head(url)
    if err != nil {
        panic(err)
    }

    size, _ := strconv.Atoi(resp.Header.Get("Content-Length"))

    resp, err = http.Get(url)
    if err != nil {
        panic(err)
    }
    defer resp.Body.Close()

    if resp.StatusCode != http.StatusOK {
        fmt.Printf("Server return non-200 status: %s\n", resp.Status)
        return
    }

    // create dest
    destName := filepath.Base(url)
    dest, err := os.Create(destName)
    if err != nil {
        fmt.Printf("Can't create %s: %v\n", destName, err)
        return
    }
    defer dest.Close()

    bar := p.AddBar(int64(size),
        mpb.BarRemoveOnComplete(),
        mpb.PrependDecorators(
            decor.CountersKibiByte("% 6.1f / % 6.1f", 18, 0)),
        mpb.AppendDecorators(decor.ETA(0, decor.DwidthSync)))

    // create proxy reader
    reader := bar.ProxyReader(resp.Body)

    // and copy from reader, ignoring errors
    io.Copy(dest, reader)
}

Note that this is using 3 channel workers, so only 3 downloads occur at a time. A new worker (and thus progressbar) isnt created until an old one has completed.

When you run the code you briefly see all progressbars spawned and then see the expired ones disappear soon after. It seems it is prioritising creating new progressbars over destroying old ones, making it so your screen gets filled with stale progressbars and creating a glitchy user experience.

vbauerster commented 6 years ago

I can't see anything wrong here. It is expected behaviour. I suppose, you'd like to achieve smooth bar replacement effect, if so see complex example.

In the code you provided, you can avoid Atoi:

size := resp.ContentLength

Also size could be unknown, i.e. less than zero, so you'd better add following after io.Copy line:

n, err := io.Copy(dest, reader)
if size <= 0 {
  bar.SetTotal(n, err == nil)
}
Naatan commented 6 years ago

Strange, this is what I am seeing:

https://asciinema.org/a/7t8pAwshsi5oyQWxyMX1hide2

I can repro it in few different terminals.

Smooth bar replacement looks interesting, I'll see if that helps. Still seems like there's something weird going on here though.

resp.ContentLength didn't work for me, it was coming up empty.

vbauerster commented 6 years ago

Make sure, you're on latest commit. Tiny URL in your example returns no content length, so I had to set total at the end to reproduce.