[RFC] Consider redesigning the API to make ProgressBar always require a parent MultiProgress

[chris-laplante]

Briefly, the way MultiProgress works is as follows. When you add a ProgressBar to a MultiProgress, the pb's draw target is replaced by a special draw target that redirects any draws to MultiState. On the one hand this is nice because it keeps ProgressBar relatively simple - it needn't be aware of whether or not it is part of a MultiProgress. In my experience however this design leads to some deadlocks (particularly when you involve Ticker).

In each loop of Ticker it first gets the BarState of the target ProgressBar, then it calls draw.

indicatif/src/progress_bar.rs

Lines 588 to 598 in 496a605

	while let Some(arc) = self.state.upgrade() {
	let mut state = arc.lock().unwrap();
	if state.state.is_finished() {
	break;
	}
	state.state.tick = state.state.tick.saturating_add(1);
	state.draw(false, Instant::now()).ok();
	drop(state); // Don't forget to drop the lock before sleeping
	drop(arc); // Also need to drop Arc otherwise BarState won't be dropped

BarState::draw:

indicatif/src/state.rs

Lines 145 to 151 in 496a605

	pub(crate) fn draw(&mut self, mut force_draw: bool, now: Instant) -> io::Result<()> {
	let width = self.draw_target.width();
	force_draw |= self.state.is_finished();
	let mut drawable = match self.draw_target.drawable(force_draw, now) {
	Some(drawable) => drawable,
	None => return Ok(()),
	};

When Ticker is installed on a ProgressBar that is part of a MultiProgress, the calls above to draw_target.width() and draw_target.drawable() will end up locking MultiState (because draw_target.kind is TargetKind::Multi).

This gives a lock order of BarState, MultiState.

The problem is that it's easy to accidentally create a lock order of MultiState, BarState. When operating on a MultiProgress, it's more natural to lock in that order anyway - I already fixed an issue stemming from this lock inversion here: #424.

Another consequence of the current design is that the lifecycle of ProgressBars as they relate to MultiProgress is a little hard to reason about. Currently, I am working on fixing #426. The issue there is that when a ProgressBar drops it is not automatically removed from the MultiProgress which can cause some visual artifacts (as well as wasting memory). The first attempt I made was to impl drop on TargetKind such that when it is TargetKind::Multi we automatically call MultiState::remove_idx. This doesn't work however, because you don't want to remove the pb if, for example, if it hasn't actually been drawn yet (depending on ProgressFinish).

Right now I'm thinking of having MultiState maintain a Weak<BarState> for each pb. Then each time MultiState::draw is called, we look for pb's that have been dropped and prune them appropriately. I anticipate locking issues with this approach however :(.

I'd like to pitch a somewhat major API change: require that every ProgressBar be part of a MultiProgress, in which case we could rename MultiProgress to something more appropriate e.g. ProgressBarContainer or similar. For example, the single.rs example could be modified like such:

use std::thread;
use std::time::Duration;

use indicatif::ProgressBar;

fn main() {
    let container = ProgressBarContainer::new();
    let pb = container.new_progress_bar(1024); // Or possibly builder pattern, e.g. container.add(ProgressBar::build(1024).with_message("msg"))
    for _ in 0..1024 {
        pb.inc(1);
        thread::sleep(Duration::from_millis(5));
    }
    pb.finish_with_message("done");
}

Anticipated benefits of doing it this way:

• A single code path for case of single vs. multiple progress bars
• Clearer lifecycle management. In the scheme I am envisioning, essentially each ProgressBar will hold an Arc<ContainerState>. ContainerState will hold a Weak to each ProgressBar that it holds.
• Simpler code - the burden of drawing will be moved entirely onto ProgressBarContainer.

The progress bar library I built (before investing in indicatif) worked this way and I found it simpler to reason about. I will work on getting that library open-sourced so you can see it.

[chris-laplante]

If I were to summarize this issue, I'd say that IMHO coordinating multiple progress bars is too complicated to be shoe-horned in is as it is currently done.

[djc]

We can do a whole bunch of refactoring, but here are some principles that I think we should uphold:

• Should not impose API (ergonomics) overhead for users who only need a single progress bar
• Should not impose performance overhead for users who only need a single progress bar

The former implies that the top-level API should probably not involve something like an explicit ProgressBarContainer; the latter implies that we shouldn't change the single bar case to require a bunch of hash maps like we currently have in MultiProgressState.

[chris-laplante]

OK, sounds fair. I will think about some alternative fixes.