Closed giuseppe closed 3 years ago
Can you please provide a snippet example where issue described at https://github.com/containers/image/issues/1013 is reproducible?
an example based on _examples/dynTotal
:
package main
import (
"context"
"io"
"math/rand"
"time"
"github.com/vbauerster/mpb/v7"
"github.com/vbauerster/mpb/v7/decor"
)
func main() {
ctx, cancel := context.WithCancel(context.Background())
p := mpb.NewWithContext(ctx, mpb.WithWidth(64))
var total int64
// new bar with 'trigger complete event' disabled, because total is zero
bar := p.AddBar(total,
mpb.PrependDecorators(decor.Counters(decor.UnitKiB, "% .1f / % .1f")),
mpb.AppendDecorators(decor.Percentage()),
)
read := makeStream(200)
for {
n, err := read()
total += int64(n)
if err == io.EOF {
break
}
// while total is unknown,
// set it to a positive number which is greater than current total,
// to make sure no complete event is triggered by next IncrBy call.
bar.SetTotal(total+2048, false)
bar.IncrBy(n)
time.Sleep(time.Duration(rand.Intn(10)+1) * time.Millisecond)
}
// force bar complete event, note true flag
bar.SetTotal(total, true)
cancel()
p.Wait()
}
func makeStream(limit int) func() (int, error) {
return func() (int, error) {
if limit <= 0 {
return 0, io.EOF
}
limit--
return rand.Intn(1024) + 1, nil
}
}
Any reason why do you call cancel()
before p.Wait()
? You can fix this by calling cancel()
after p.Wait()
.
Any reason why do you call
cancel()
beforep.Wait()
? You can fix this by callingcancel()
afterp.Wait()
.
the reason is that the job can be canceled at any time, so that could happen before the bars are set to completed with bar.SetTotal(..., true)
. In this case p.Wait()
will block indefinitely waiting for the bars to complete, and the code will never hit cancel()
So you cancel progress in the middle and expect bar to continue render afterwards? Then I don't quite understand what your PR is trying to fix?
I want to make sure that:
1) we can cancel at any time and don't risk that p.Wait()
can block for any reason.
2) if all the progress bars are completed, even if cancelled, they are rendered correctly.
Without this fix we would have to keep track of the status for each bar separately, and then handle differently the case when they are all completed and when they are not.
1. we can cancel at any time and don't risk that `p.Wait()` can block for any reason.
Instantiate p
without ctx
i.e. p = mpb.New(...)
, that way you'll able to cancel independently and control bar completion with bar.SetTotal(total, true)
at a granular level.
2. if all the progress bars are completed, even if cancelled, they are rendered correctly.
if all the progress bars are completed, call to p.Wait()
makes sure they are rendered correctly.
Without this fix we would have to keep track of the status for each bar separately, and then handle differently the case when they are all completed and when they are not.
This fix doesn't guarantee anything. You're trying to fix non existent problem.
1. we can cancel at any time and don't risk that `p.Wait()` can block for any reason.
won't that leak go routines if the job is cancelled but the progress bars are not?
Well you've decided it is not a problem for us and have closed the PR but but it is still not clear how to properly address it without tracking progress bars status separately
could we expose a way to force a render or set heapUpdated = true
?
Your RP doesn't guarantee that bar is rendered correctly. If bar had OnComplete
handler attached like
mpb.AppendDecorators(decor.OnComplete(decor.Percentage(), "done"))
"done"
message would never be printed in that case.
could we expose a way to force a render or set
heapUpdated = true
?
use p.Wait()
for that, I'm not going to expose internal API.
fix a race issue where a cancelled context might cause the progress bar to never be rendered.
More details here: https://github.com/containers/image/issues/1013
Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com