Skip to content
Open
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 96 additions & 17 deletions src/progress_bar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ impl ProgressBar {
/// interval. This is useful to advance progress bars that are very slow by themselves.
///
/// When steady ticks are enabled, calling [`ProgressBar::tick()`] on a progress bar does not
/// have any effect.
/// have any effect. Consider using [`ProgressBar::enable_responsive_tick()`] when the progress
/// bar should update immediately instead.
pub fn enable_steady_tick(&self, interval: Duration) {
// The way we test for ticker termination is with a single static `AtomicBool`. Since cargo
// runs tests concurrently, we have a `TICKER_TEST` lock to make sure tests using ticker
Expand All @@ -199,21 +200,56 @@ impl ProgressBar {
return;
}

self.stop_and_replace_ticker(Some(interval));
let reaction = TickerReaction::OnTimeout;
self.stop_and_replace_ticker(Some((interval, reaction)));
}

/// Undoes [`ProgressBar::enable_steady_tick()`]
/// Spawns a background thread to tick the progress bar
///
/// When this is enabled a background thread will regularly tick the progress bar in the given
/// interval. This is useful to advance progress bars that are very slow by themselves.
///
/// When responsive ticks are enabled, calling [`ProgressBar::tick()`] will immediately update
/// the progress bar. Consider using [`ProgressBar::enable_steady_tick()`] for a steady periodic
/// tick.
pub fn enable_responsive_tick(&self, interval: Duration) {
// The way we test for ticker termination is with a single static `AtomicBool`. Since cargo
// runs tests concurrently, we have a `TICKER_TEST` lock to make sure tests using ticker
// don't step on each other. This check catches attempts to use tickers in tests without
// acquiring the lock.
#[cfg(test)]
{
let guard = TICKER_TEST.try_lock();
let lock_acquired = guard.is_ok();
// Drop the guard before panicking to avoid poisoning the lock (which would cause other
// ticker tests to fail)
drop(guard);
if lock_acquired {
panic!("you must acquire the TICKER_TEST lock in your test to use this method");
}
}

if interval.is_zero() {
return;
}

let reaction = TickerReaction::Immediately;
self.stop_and_replace_ticker(Some((interval, reaction)));
}

/// Undoes both [`ProgressBar::enable_steady_tick()`] and [`ProgressBar::enable_responsive_tick()`]
pub fn disable_steady_tick(&self) {
self.stop_and_replace_ticker(None);
}

fn stop_and_replace_ticker(&self, interval: Option<Duration>) {
fn stop_and_replace_ticker(&self, interval: Option<(Duration, TickerReaction)>) {
let mut ticker_state = self.ticker.lock().unwrap();
if let Some(ticker) = ticker_state.take() {
ticker.stop();
}

*ticker_state = interval.map(|interval| Ticker::new(interval, &self.state));
*ticker_state =
interval.map(|(interval, reaction)| Ticker::new(interval, reaction, &self.state));
}

/// Manually ticks the spinner or progress bar
Expand All @@ -224,8 +260,10 @@ impl ProgressBar {
}

fn tick_inner(&self, now: Instant) {
// Only tick if a `Ticker` isn't installed
if self.ticker.lock().unwrap().is_none() {
// If a ticker thread is installed, notify it to do the ticking
if let Some(ref ticker) = *self.ticker.lock().unwrap() {
ticker.stopping.1.notify_one();
} else {
self.state().tick(now);
}
}
Expand Down Expand Up @@ -684,6 +722,15 @@ impl WeakProgressBar {
}
}

/// Behavior of a steady ticker when its progress bar is ticked
#[derive(Debug, Clone, Copy)]
pub(crate) enum TickerReaction {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this should go below Ticker.

/// Only ever tick when the interval has passed, ignoring manual ticks
OnTimeout,
/// Immediately tick when done so manually, or after the interval has passed
Immediately,
}

pub(crate) struct Ticker {
stopping: Arc<(Mutex<bool>, Condvar)>,
join_handle: Option<thread::JoinHandle<()>>,
Expand All @@ -700,7 +747,11 @@ impl Drop for Ticker {
static TICKER_RUNNING: AtomicBool = AtomicBool::new(false);

impl Ticker {
pub(crate) fn new(interval: Duration, bar_state: &Arc<Mutex<BarState>>) -> Self {
pub(crate) fn new(
interval: Duration,
reaction: TickerReaction,
bar_state: &Arc<Mutex<BarState>>,
) -> Self {
debug_assert!(!interval.is_zero());

// A `Mutex<bool>` is used as a flag to indicate whether the ticker was requested to stop.
Expand All @@ -713,7 +764,7 @@ impl Ticker {
state: Arc::downgrade(bar_state),
};

let join_handle = thread::spawn(move || control.run(interval));
let join_handle = thread::spawn(move || control.run(interval, reaction));
Self {
stopping,
join_handle: Some(join_handle),
Expand All @@ -732,32 +783,60 @@ struct TickerControl {
}

impl TickerControl {
fn run(&self, interval: Duration) {
fn run(&self, interval: Duration, reaction: TickerReaction) {
#[cfg(test)]
TICKER_RUNNING.store(true, Ordering::SeqCst);

let mut last_tick = None;

while let Some(arc) = self.state.upgrade() {
let mut state = arc.lock().unwrap();
if state.state.is_finished() {
break;
}

state.tick(Instant::now());
let now = Instant::now();

let do_tick = match reaction {
TickerReaction::Immediately => true,
TickerReaction::OnTimeout => match last_tick {
None => true,
Some(last_tick) => {
let passed = now - last_tick;
passed >= interval
}
},
};

if do_tick {
state.tick(now);
last_tick = Some(now);
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is confused about what it wants to be. If you want .tick() to be responsive, IMO it should not affect a running ticker at all.

Copy link
Author

Choose a reason for hiding this comment

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

I know what you mean, but this would actually result in a different behavior for spinners. When the spinner thread keeps track of the last tick, then there will always be an interval pause until the next automatic tick happens. When we just do the tick outside with no communication with the thread, then these reactive ticks and the automatic ticks appear irregular.

The attached video demonstrates this, where all tickers tick once a second, while the bars increments every 1.2 seconds. The "Inside Ticker" bar uses a responsive ticker as implemented here, the "Outside Ticker" uses the original ticker implementation but also ticks on increments.

To me both approaches are fine, I just want you to be aware.

ticker-behavior.webm

Copy link
Author

Choose a reason for hiding this comment

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

After thinking more about it, I found a way to get the desired spinner behavior without that complicated timeout logic. Now the thread is only notified when we want it to tick, so the check is moved outside.


drop(state); // Don't forget to drop the lock before sleeping
drop(arc); // Also need to drop Arc otherwise BarState won't be dropped

// Wait for `interval` but return early if we are notified to stop
let timeout = match last_tick {
None => interval,
Some(last_tick) => {
let next_tick = last_tick + interval;
let Some(timeout) = next_tick.checked_duration_since(now) else {
// do next tick right away
continue;
};
timeout
}
};

// Wait for `timeout` but return early if we are notified
let result = self
.stopping
.1
.wait_timeout_while(self.stopping.0.lock().unwrap(), interval, |stopped| {
!*stopped
})
.wait_timeout(self.stopping.0.lock().unwrap(), timeout)
.unwrap();

// If the wait didn't time out, it means we were notified to stop
if !result.1.timed_out() {
// Stop the ticker when the mutex flag was set
if *result.0 {
break;
}
}
Expand Down