[RFC] Remove MultiProgress::join()

[marienz]

This should not be merged yet, but I'd like to know if this looks like something worth pursuing.

This change moves drawing of MultiProgress from the join() thread to the child ProgressBar updates.

This has the following advantages:

• It addresses MultiProgress should support tokio (async) #125 (async MultiProgress), as long as users are willing to assume the writes to the screen do not block (and they do not add enough child progress bars for lock contention on the MultiProgress state to be an issue). It also makes it trivial to use MultiProgress without any threads or async runtime at all.
• It helps with Add println method to MultiProgress #205 (add println to MultiProgress) because println on MultiProgress can now be made to work while the MultiProgress does not have any unfinished progress bars / join() hasn't been called yet.
• Arguably the code is simpler (and it removes the one use of unsafe in indicatif: MultiProgress is now automatically Sync).

It also has disadvantages, most of which I think can be mitigated:

• It removes grouping, which regresses the problem fixed by Implement update grouping #166. I think it is possible to largely mitigate this by implementing a draw target that uses a timer (via an async runtime or a thread) to schedule a draw rate - last_draw.elapsed() in the future if we don't want to draw immediately. I haven't written the code for this yet, though, so it might not be as easy as I think...
• It is backwards-incompatible. This could be mitigated by implementing this as a new type (and a new ProgressDrawTargetKind, which is fine as those are not public) if the old behavior is deemed useful enough to keep around and/or we want to avoid the semver bump.
• It removes the ability to use the MultiProgress as a synchronization point. For the examples, this was very easy to fix by joining the worker threads instead. I don't know if this is also true for "real" code.
• It moves work from the join() thread to the threads updating the progress bars. I suspect this is not a problem because that work is already fast enough (relative to rendering the child progress bar to a ProgressDrawState, which was already happening on the worker threads), but I haven't benchmarked it. It might be a problem for code that has at least one progress bar that is updated at a very high rate.

I think the DrawTarget-with-a-timer is useful anyway: it should mitigate a problem very similar to #166 when doing something like:

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

use indicatif::ProgressBar;

fn main() {
    let p = ProgressBar::new_spinner();
    for _ in 0..10 {
        p.set_message("performing fast work (you should rarely see this)");
        // Just to demonstrate they don't need to be back-to-back. Removing this sleep does not change behavior.
        thread::sleep(Duration::from_millis(5));
        p.set_message("performing slow work (you should see this)");
        thread::sleep(Duration::from_secs(1));
    }
}

This currently only shows "you should not see this" for the same reason the MultiProgress example didn't stay in sync. With a DrawTarget-with-a-timer, it would show "you should not see this" for only one update interval. So I'll probably see if I can get that to work even if this change isn't deemed useful.

Please let me know what you think!

[mdekstrand]

My main question on this direction is whether it creates new problems for scoping of the MultiProgress and threads. I don't know that it does, but also don't currently know that it doesn't.

[marienz]

I'm not sure if I understand the question about scoping. Can you give an example you're concerned about?

To port to the new code, all you should have to do is remove the call to join() and make sure this does not cause the program to exit too soon (you may have to wait for worker threads explicitly to do so). It is ok if dropping the call to join() causes the MultiProgress to be dropped before the worker threads are done with their ProgressBars: any ProgressBars already added to the MultiProgress before it was dropped will continue to work correctly, you just can't add any additional ones without a reference to the MultiProgress.

[marienz]

#234 adds the promised DrawTarget-with-a-timer.

[jackinloadup]

This branch solved a problem I was having with my application structure. Thanks for the work!

[djc]

Sorry for taking so long to get to this. I do think this idea makes sense. Would you be willing to rebase it on top of current main? It looks like some of the structural changes that you made are already part of main now, but there have been quite a few changes, so the rebase might be a little painful.

[aj-bagwell]

I like this too, I have rebased @marienz's changes onto main and added a leaky bucket to the rate limiting to fix the regression on #166 in aj-bagwell:multiprogress-unjoined if that helps keep this moving.

[djc]

Sounds great, would you mind submitting a separate PR for this?

[aj-bagwell]

@djc sure, new pull request is #284

[djc]

Okay, I'll close this for now.