Bar updates race vs. cancelation

[mtrmac]

(Context: containers/image#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, and p.Wait() does not trigger an immediate re-render.

Reproducer: operation that times out does not report full progress.

package main

import (
	"context"
	"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))
	bar := p.AddBar(2,
		mpb.PrependDecorators(decor.Counters(decor.UnitKiB, "% .1f / % .1f")),
		mpb.AppendDecorators(decor.Percentage()),
	)
	bar.SetCurrent(0)
	time.Sleep(1 * time.Second) // Clearly enough time for the initial rendering

	// Perform an operation tracked by the progress bar, but it times out
	bar.SetCurrent(1) // Partial progress to report.
	if true {
		// Alternative 1: To terminate Bar.serve, cancel the whole container
		// (This is what would naturally happen with a context.WithTimeout() , with no extra code.)       
		// Note to self: that this doesn’t affect Progress.serve directly at all; it isn't checking ctx.
		// It only serves to cancel Bar.serve via Bar.cancel
		cancel()
	} else if false {
		// Alternative 2: To terminate Bar.serve, trigger Bar.cancel using bar.Abort()
		bar.Abort(false)
	} else {
		// Alternative 3: Hangs because Bar.serve never terminates.
	}

	// Not enough happens here for Progres.refreshCh to trigger,
	// and we don’t want to just heuristically sleep.

	// We must call this so that the rendering by Progress.serve finishes,
	// and other output can be safely written to stdout afterwards.
	// In turn, that blocks on the bar’s Bar.serve to finish first. So we must terminate Bar.serve.
	p.Wait()
}

Output with alternatives 1, 2:

0.0 b / 2.0 b [--------------------------------------------------------------] 0 %

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 recent bar.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 use defer immediately after creating a progress EDIT bar container, or after creating a bar, to ensure Bar.serve is terminated; that can be done using either the context passed to mpb.NewWithContext, which #92 advises against, or bar.Abort() on each individual progress bar, which documents:

// … It has no effect after
// completion event. …

Yet

package main

import (
	"time"

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

func main() {
	p := mpb.New(mpb.WithWidth(64))
	bar := p.AddBar(2,
		mpb.PrependDecorators(decor.Counters(decor.UnitKiB, "% .1f / % .1f")),
		mpb.AppendDecorators(decor.Percentage()),
	)
	bar.SetCurrent(0)
	time.Sleep(1 * time.Second) // Clearly enough time for the initial rendering

	bar.SetCurrent(2) // Finished!
	bar.Abort(false)

	// Not enough happens here for Progres.refreshCh to trigger,
	// and we don’t want to just heuristically sleep.

	// We must call this so that the rendering by Progress.serve finishes,
	// and other output can be safely written to stdout afterwards.
	// In turn, that blocks on the bar’s Bar.serve to finish first. So we must terminate Bar.serve.
	p.Wait()
}

renders

0.0 b / 2.0 b [--------------------------------------------------------------] 0 %

because Bar.serve is canceled by bar.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 do

bar := p.AddBar(…)
defer func() {
  if !p.Completed() {
    p.Abort()
  }
}

That’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 to Progress.AddBar could also work well for us — as long as that cancellation only served to terminate the p.Wait() call, but p.Wait() still waited for full rendering of the final state.

[vbauerster]

// … 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.

[vbauerster]

I have enforced completion check in Abort method: b1d0fee
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()
  }
}

[mtrmac]

Thank you!

[vbauerster]

You're welcome!
It's fixed in v7.0.5. Feel free to reopen if something wrong.