bug: MultiProgress with insert_before and suspend destroys the previous progress bars

[budde25]

I noticed that when i added the suspend function to my cargo-like multi progress bar it kept deleting previous progress bars.

env:

$ rustc --version
rustc 1.72.1 (d5c2e9c34 2023-09-13)

cargo toml:

indicatif = "0.17.7"

After narrowing down the issue i found that it only happened if I used the insert_before or the insert_at function. The regular add did not cause this.

use indicatif::{MultiProgress, ProgressBar, ProgressStyle};

fn main() {
    let m = MultiProgress::new();
    let sty = ProgressStyle::with_template(
        "{prefix:<10} [{elapsed_precise}] {bar:40.cyan/blue} {pos:>7}/{len:7} {msg}",
    )
    .unwrap()
    .progress_chars("##-");

    let total = m.add(ProgressBar::new(10));
    total.set_style(sty.clone());
    total.set_prefix("total");
    for i in 0..10 {
        let name = format!("item-{i}");
        let pb = m.insert_before(&total, ProgressBar::new(100));
        pb.set_style(sty.clone());
        pb.set_prefix(name);
        for _ in 0..100 {
            total.suspend(|| ());
            pb.inc(1);
        }
        pb.finish();
        total.inc(1);
    }
    total.finish();
}

it gives the output of:

item-9     [00:00:00] ########################################     100/100     
total      [00:00:00] ########################################      10/10 

removing the suspend call gives me the expected output of:

item-0     [00:00:00] ########################################     100/100     
item-1     [00:00:00] ########################################     100/100     
item-2     [00:00:00] ########################################     100/100     
item-3     [00:00:00] ########################################     100/100     
item-4     [00:00:00] ########################################     100/100     
item-5     [00:00:00] ########################################     100/100     
item-6     [00:00:00] ########################################     100/100     
item-7     [00:00:00] ########################################     100/100     
item-8     [00:00:00] ########################################     100/100     
item-9     [00:00:00] ########################################     100/100     
total      [00:00:00] ########################################      10/10 

When i ran it with add, instead of insert_before it printed correctly both ways (expect the total is at the top rather than bottom hence why i wanted to use insert_before. I also tried using pb.suspend() instead of total.suspend() but got the same results.

Thanks!

[chris-laplante]

I will dig into this later (hopefully), but in the meantime, what is your use-case for using suspend? In general, suspend doesn't play too well with MultiProgress because it has to pessimistically assume that you wrote to the console in the closure. It can't possibly know how many lines you may have written, so it basically has to "forget" about any progress bars rendered prior to the suspend call.

^ Ignore this brainfart please

[budde25]

Ok that does make sense. Yes that is basically my use case, however, I was putting debug! (from log) statements in the closure. So 99% of the time i didn't write anything to the console but was still breaking my progress bars. I suppose i should probably just skip the progress bars when there is verbosity or put the suspend in a if verbosity block.
Thanks!

[chris-laplante]

Ah.... actually, sorry, I took another look and my explanation from before was wrong. suspend works like this:

• Clear the screen of all active ProgressBars
• Execute the closure
• Redraw all active ProgressBars

The issue is that your ProgressBars are going out of scope at the end of each iteration of the outer for-loop. (Remember that a ProgressBar is basically just an Arc).

The solution is to just extend the lifetime of all the ProgressBars by keeping a Vec or something of them in the outer scope:

use indicatif::{MultiProgress, ProgressBar, ProgressStyle};

fn main() {
    let m = MultiProgress::new();
    let sty = ProgressStyle::with_template(
        "{prefix:<10} [{elapsed_precise}] {bar:40.cyan/blue} {pos:>7}/{len:7} {msg}",
    )
        .unwrap()
        .progress_chars("##-");

    let mut pbs = vec![];

    let total = m.add(ProgressBar::new(10));
    total.set_style(sty.clone());
    total.set_prefix("total");
    for i in 0..10 {
        let name = format!("item-{i}");
        let pb = m.insert_before(&total, ProgressBar::new(100));
        pbs.push(pb.clone());
        pb.set_style(sty.clone());
        pb.set_prefix(name);
        for _ in 0..100 {
            total.suspend(|| ());
            pb.inc(1);
        }
        pb.finish();
        total.inc(1);
    }
    total.finish();
}

[chris-laplante]

If you'd like to avoid the manual suspend calls, you can use something like this with your logging backend btw: https://github.com/Agilent/yb/blob/main/yb/src/main.rs#L72. You still need to keep the ProgressBars alive though.

[chris-laplante]

The whole MultiProgress lifetime thing is something I really need to document better. I will submit an issue against myself to do it. Sorry again for the trouble.

[budde25]

Oh that is awesome, yeah that does seem to fix use case (though it might still be desirable to skip suspend when i can). Thanks for taking a stab at that and finding a solution!

Follow up question. Why doesn't the the Multiprogress bar keep the individual Arc's around by doing a clone into a inner vec itself (thus keeping ref count a 1) and keeping the old bars around?

[chris-laplante]

Oh that is awesome, yeah that does seem to fix use case (though it might still be desirable to skip suspend when i can). Thanks for taking a stab at that and finding a solution!

Sure thing!

Follow up question. Why doesn't the the Multiprogress bar keep the individual Arc's around by doing a clone into a inner vec itself (thus keeping ref count a 1) and keeping the old bars around?

It's a trade-off. We want to be able to redraw as much as possible in the case of a call to suspend, for instance. But we also don't want the memory usage of MultiProgress to grow forever in a long-running CLI. The big assumption is that if a ProgressBar has been dropped, then the user probably doesn't care about drawing it anymore.

Perhaps we could add an option for MultiProgress to always keep around an Arc of each ProgressBar added. This could make for a helpful opt-out of the behavior I just described, for cases when the user doesn't want to have to think about ProgressBar lifetime and is OK taking the potential hit on memory. And it would make code that like what you originally posted Just Work™️. What do you think @djc?

BTW for some additional context, see:

• MultiProgress: unclear if finished bars needs to be manually removed #419
• MultiProgress: should prune leading progress bars that have become DoneVisible or DoneHidden #426

[djc]

Do we already have good documentation for the constraints here? I feel like the Drop-based behavior is pretty natural for Rust, and adding an optional way to artificially keep ProgressBars alive feels like a bunch of extra complexity that we could do without.

[chris-laplante]

Do we already have good documentation for the constraints here? I feel like the Drop-based behavior is pretty natural for Rust, and adding an optional way to artificially keep ProgressBars alive feels like a bunch of extra complexity that we could do without.

That's fair. It is not well documented, but I submitted an issue to do so: #595