Implement ProgressBar::suspend

[ishitatsuyuki]

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.

[djc]

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

[ishitatsuyuki]

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]

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

[ishitatsuyuki]

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]

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]

Thanks!

[hasezoey]

@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]

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

[hasezoey]

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]

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]

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]

.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]

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.