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

Implement ProgressBar::suspend #333

Merged
merged 3 commits into from Dec 20, 2021
Merged

Implement ProgressBar::suspend #333

merged 3 commits into from Dec 20, 2021

Conversation

ishitatsuyuki
Copy link
Contributor

This function hides the progress bar temporarily, execute f, then redraws the progress bar, which is useful for external code that writes to the standard output.

The idea is originally raised in #92.

It is planned to eventually create a global function like ProgressBar::instance that allows calling suspend from e.g. a log handler, but this currently doesn't generalize to MultiProgressBar which could be an obstacle.

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! How hard would it be to add some kind of test for this?

is_finished has always been ORed with force_draw, and removing it
allows the logic to be simplified.
@ishitatsuyuki
Copy link
Contributor Author

Actually I'm thinking about an alternative way to implement this: instead of taking a closure, we return a SuspendHandle that when dropped, restores the progress bar and unlocks the mutex.

The reason is that the tracing crate's API exposes the writer like this:

pub trait MakeWriter<'a> {
    type Writer: Write;
    fn make_writer(&'a self) -> Self::Writer;
    (... optional methods)
}

If we used the closure-style API then we would need to wrap the Write trait and call suspend() for each of the Write function, which could be inefficient depending on how things are implemented. The Drop based approach would be easier, only requiring a wrapper struct to be returned as the Writer.

Or maybe we could expose both. BTW I found that it might be useful for us to pass on the return value from the closure so I'll add it in an upcoming commit.

As for automated testing, I don't have a good idea how it could be implemented yet. Any suggestions?

@djc
Copy link
Collaborator

djc commented Dec 16, 2021

I think using a drop guard makes sense for this. Unless there's a good reason I'd prefer not to have both.

@ishitatsuyuki
Copy link
Contributor Author

ishitatsuyuki commented Dec 16, 2021

I realized the closure one is better after all, as the drop guard will have a lifetime specifier, making various things harder to do. Especially when dealing with a static Mutex<WeakProgressBar>, it's not possible to obtain a &'static mut ProgressBar so the mutex guard cannot really be held.

Let's go with the closure one. It's kinda ugly to rely on the implementation detail, but with the current tracing-subscriber implementation it isn't less efficient than the drop guard implementation since the write is performed with a single write_all call.

Code I'm using in my prototype, for reference
struct SuspendProgressBarWriter<T: Write>(T);

impl<T: Write> SuspendProgressBarWriter<T> {
    fn suspend<F: FnOnce(&mut Self) -> R, R>(&mut self, f: F) -> R {
        let handle = PROGRESS_BAR.lock().unwrap().upgrade();
        if let Some(mut p) = handle {
            p.suspend(|| f(self))
        } else {
            f(self)
        }
    }
}

static PROGRESS_BAR: Lazy<Mutex<WeakProgressBar>> = Lazy::new(|| Mutex::new(WeakProgressBar::default()));

impl<T: Write> Write for SuspendProgressBarWriter<T> {
    #[inline]
    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
        self.suspend(|this| this.0.write(buf))
    }

    #[inline]
    fn write_vectored(&mut self, bufs: &[io::IoSlice<'_>]) -> io::Result<usize> {
        self.suspend(|this| this.0.write_vectored(bufs))
    }

    #[inline]
    fn flush(&mut self) -> io::Result<()> {
        self.suspend(|this| this.0.flush())
    }

    #[inline]
    fn write_all(&mut self, buf: &[u8]) -> io::Result<()> {
        self.suspend(|this| this.0.write_all(buf))
    }

    #[inline]
    fn write_fmt(&mut self, fmt: std::fmt::Arguments<'_>) -> io::Result<()> {
        self.suspend(|this| this.0.write_fmt(fmt))
    }
}

@ishitatsuyuki
Copy link
Contributor Author

Patch updated:

  • suspend takes &self instead of &mut self (I forgot that ProgressBar is Arc/Mutex wrapped)
  • Fixed broken doc intra-links
  • Add in a change to WeakProgressBar to make tracking progress bar globally easier

@djc djc merged commit ffb3927 into console-rs:main Dec 20, 2021
@djc
Copy link
Collaborator

djc commented Dec 20, 2021

Thanks!

@hasezoey
Copy link
Contributor

hasezoey commented Jan 7, 2022

@ishitatsuyuki could you update your message to be properly formatted and/or is there another example (/documentation) of working with this function for the log crate and indicatif?

@ishitatsuyuki
Copy link
Contributor Author

I fixed the formatting. For env_logger, I think using env_logger::fmt::Target with a SuspendProgressBarWriter(std::io::stdout()) will work.

@hasezoey
Copy link
Contributor

hasezoey commented Jan 20, 2022

I fixed the formatting. For env_logger, I think using env_logger::fmt::Target with a SuspendProgressBarWriter(std::io::stdout()) will work.

thanks for the response and the prototype, though env_logger does not support outputting to something else than stderr and stdout, see rust-cli/env_logger#208

for now i use a similar logger / crate: simplelog


for anyone that wants to use a "improved" version of the prototype from #333 (comment), with some paths from sentry-cli's logging:

use std::io::Write;

use indicatif::{
	ProgressBar,
	WeakProgressBar,
};
use parking_lot::RwLock;

// Utils file, may contain various small helper functions for main binary

#[derive(Debug)]
pub struct ProgressHandler<T: Write + Send>(T);

lazy_static::lazy_static! {
	/// Stores the reference to the current Progressbar, if there even is one
	/// uses "parking_lot::RwLock", because it is faster than the standart libraries and to allow many readers (read-only access to the variable)
	static ref CURRENT_PROGRESS_BAR: RwLock<Option<WeakProgressBar>> = RwLock::new(None);
}

impl<T: Write + Send> ProgressHandler<T> {
	/// Helper function to execute everything before running "inner_function"
	fn handle<F: FnOnce(&mut Self) -> R, R>(&mut self, inner_function: F) -> R {
		let handle = get_progress_bar();

		return match handle {
			Some(pb) => pb.suspend(|| return inner_function(self)),
			None => inner_function(self),
		};
	}

	/// Create a new instance of "Self"
	pub fn new(pipe: T) -> Self {
		return Self(pipe);
	}
}

impl<T: Write + Send> Write for ProgressHandler<T> {
	#[inline]
	fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
		return self.handle(|this| this.0.write(buf));
	}

	#[inline]
	fn flush(&mut self) -> std::io::Result<()> {
		return self.handle(|this| this.0.flush());
	}
}

/// Sets the reference to the current ProgressBar for the writer
pub fn set_progress_bar(pb: Option<WeakProgressBar>) {
	*CURRENT_PROGRESS_BAR.write() = pb;
}

/// Returns the Progressbar, if it exists, otherwise "None"
fn get_progress_bar() -> Option<ProgressBar> {
	return CURRENT_PROGRESS_BAR.read().as_ref()?.upgrade();
}

and then the simplelog integration:

use simplelog::*;
WriteLogger::init(
	LevelFilter::Debug,
	ConfigBuilder::new().set_thread_mode(ThreadLogMode::Both).build(),
	ProgressHandler::new(std::io::stdout()),
)
.expect("Failed to create Logger");

@ishitatsuyuki
Copy link
Contributor Author

In that case, it's also possible to use Builder::format which essentially allows control over the whole writeln! call. The drawback of this is that you need to can't use env_logger's default formatting and need to write your own.

@wbogocki
Copy link
Contributor

wbogocki commented Feb 2, 2022

The workaround is problematic because if the PB is either finished or hidden then it also discards all log messages. This is impossible to get around because .is_hidden(), .is_finished() and .println() each lock and unlock the mutex individually so there's a race condition if we tried to do the checks like this:

if pb.is_hidden() || pb.is_finished() { // Race condition on the ||
    println!(...); // Race condition because the if check is not necessarily valid anymore
} else {
   pb.println(...);
}

.suspend() is likewise problematic because the PB can't be accessed inside the suspend closure because all of the methods try to lock the mutex which would've already been locked inside of the closure and result in a deadlock.

Perhaps it'd be better to make a .println_always() that ignores whether the PB is hidden or finished?

Personally, I'm tempted to either patch the crate locally or get the private mutex with some trickery, lock it, do my things, unlock it.

This is just my two cents, once 0.17 is released and suspend() works it's good enough for my use case 👍

@ishitatsuyuki
Copy link
Contributor Author

.suspend() is likewise problematic because the PB can't be accessed inside the suspend closure because all of the methods try to lock the mutex which would've already been locked inside of the closure and result in a deadlock.

Is this a problem for you? I thought that you won't need to access the progress bar inside suspend. It's supposed that you call println!() or whatever write operation to stdout.

@wbogocki
Copy link
Contributor

wbogocki commented Feb 2, 2022

No, it's not a problem for me, you're good. I was unsure whether println!() and pb.println() would have the same effect, I'm guessing they would so I'll be good. I'm only slightly annoyed with how closed the internals are so you can't easily patch over them when needed.

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

4 participants