Closed mtrmac closed 3 years ago
// … It has no effect after // completion event. …
Yet
There are 'OnCompete' handlers may be triggered at that point, so despite of completion has happened already, technically bar is still in cancellable state. Sorry for confusing Abort's
doc comment I'll reword it.
If completion has happened either by naturally reaching total or by manually bar.SetTotal(total, true)
, there is no need to call bar.Abort(bool)
afterwards, just let call to p.Wait()
handle the rest. p.Wait()
would block only if there is ongoing bar(s) running, waiting for its completion or abortion, also it guarantees that everything is flushed properly.
As for tracking operation timeout case, canceling context will cancel all bars in the container (pool) releasing p.Wait()
instantly after everything is flushed. If it's not what you want then there is bar.Abort(bool)
available. Aborting bar will leave it in incomplete state and it's what you're actually observing.
If your policy is just cancel (abort) particular bar on any unexpected event then you get what you get. If you do retry after timeout, then I personally wouldn't cancel (abort) the bar in that case.
I have enforced completion check in Abort method: https://github.com/vbauerster/mpb/commit/b1d0fee1d40e41ecdb0e99af4207c5f2e0b666cb so you don't need to check for it:
bar := p.AddBar(…)
defer func() {
if !bar.Completed() { // not necessary in next version of course
bar.Abort()
}
}
Thank you!
You're welcome! It's fixed in v7.0.5. Feel free to reopen if something wrong.
(Context: https://github.com/containers/image/issues/1013 and #92 . We might very well be doing something stupid, if so I apologize for wasting your time.)
At a high level, we’d like to differentiate between canceling the “tracked operation” (e.g. a download; we must support such cancelation, e.g. for externally-imposed timeouts) and cancelling the “progress bar machinery” (which we don’t want to do; we’d prefer to terminate it when rendered in a consistent state); and to cancel the tracked operation with minimum hassle.
One way to express this is that terminating
Bar.serve
drops recently-made updates, AFAICS progress is only rendered in intervals, andp.Wait()
does not trigger an immediate re-render.Reproducer: operation that times out does not report full progress.
Output with alternatives 1, 2:
i.e. not updated to the true progress of 1/2. I can’t find a public API to explicitly cause a flush either.
This is admittedly a pretty minor quibble — if the operation has failed, who cares what exactly the progress bars say? — but making this work seems like the same class or problem as making the
SetTotal
+cancel
case work.A variant: calling
bar.Abort()
races against recentbar.SetTotal()
.Motivation: As described above,
p.Wait()
should always be called, but we want to make sure it never hangs. So it would be convenient for EDIT us to usedefer
immediately after creating a progress EDITbarcontainer, or after creating a bar, to ensureBar.serve
is terminated; that can be done using either the context passed tompb.NewWithContext
, which #92 advises against, orbar.Abort()
on each individual progress bar, which documents:Yet
renders
because
Bar.serve
is canceled bybar.Abort
before fully processing the completion.So based on the above, if we wanted to ensure that
p.Wait()
never hangs, it seems that we would have to doThat’s certainly doable, but being able to just pass a cancellable context to
mpb.NewWithContext
and not worry about it would be much nicer.Alternatively, an ability to pass a
context.Context
toProgress.AddBar
could also work well for us — as long as that cancellation only served to terminate thep.Wait()
call, butp.Wait()
still waited for full rendering of the final state.