Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: MultiProgress insert_from_back #326

Merged
merged 1 commit into from Nov 25, 2021
Merged

feat: MultiProgress insert_from_back #326

merged 1 commit into from Nov 25, 2021

Conversation

omjadas
Copy link
Contributor

@omjadas omjadas commented Nov 22, 2021

Add insert_from_back method to MultiProgress

@calmofthestorm
Copy link

It may not matter that much, but wouldn't that be subject to a race condition if another thread added a new progress bar between the call to len and the call to insert? Granted in practice this would result in the bars being added in unspecified order which is probably fine.

An alternative would be insert_from_back or to change insert to take negative numbers Python-style to indicate offset from the end.

I run into this when I have an overall status bar at the bottom, and then add subactions above, when there are enough subactions (or a more are added as we go) that the overall status bar would scroll offscreen.

@omjadas
Copy link
Contributor Author

omjadas commented Nov 22, 2021

It may not matter that much, but wouldn't that be subject to a race condition if another thread added a new progress bar between the call to len and the call to insert? Granted in practice this would result in the bars being added in unspecified order which is probably fine.

I think just using insert without relying on the length of the MultiProgress could also result in a race condition. For example, if you were to insert multiple bars with the same index concurrently, you wouldn't have any guarantees as to which eventually resides at that index.

use indicatif::{MultiProgress, ProgressBar};
use std::{sync::Arc, thread};

fn main() {
    let mp = Arc::new(MultiProgress::new());
    let first_bar = ProgressBar::new(100);
    let second_bar = ProgressBar::new(100);

    let mp_1 = Arc::clone(&mp);
    let thread_1 = thread::spawn(move || {
        mp_1.insert(0, first_bar);
    });

    let mp_2 = Arc::clone(&mp);
    let thread_2 = thread::spawn(move || {
        mp_2.insert(0, second_bar);
    });

    thread_1.join().unwrap();
    thread_2.join().unwrap();

    // bars in mp have unknown order
}

A potential way this could be mitigated would be to change insert (and a potential insert_from_back) to require mutable references to self, requiring the user to deal with locking themselves.

@djc
Copy link
Collaborator

djc commented Nov 24, 2021

I'd prefer an insert_from_back()-like solution over this if that's the actual goal. I think externalizing the locking could be a different PR (or, at least, a different commit).

@omjadas omjadas changed the title feat: expose length of MultiProgress feat: MultiProgress insert_from_back Nov 24, 2021
@omjadas
Copy link
Contributor Author

omjadas commented Nov 24, 2021

I'd prefer an insert_from_back()-like solution over this if that's the actual goal. I think externalizing the locking could be a different PR (or, at least, a different commit).

@djc I have changed this PR to add an insert_from_back method. I agree that if the locks were to be externalized it should be a separate PR.

src/progress_bar.rs Outdated Show resolved Hide resolved
@djc djc merged commit 16e755c into console-rs:main Nov 25, 2021
@djc
Copy link
Collaborator

djc commented Nov 25, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants