Fix Ticker issue #397

[chris-laplante]

I think there are two parts to #397:

1. In addition to dropping the MutexGuard<BarState>, we also need to drop the Arc<Mutex<BarState>>, otherwise the reference count will never drop to 0 while the thread sleeps. The main thread will exit, but the destructor (on the Ticker thread's stack) will not run.
2. Even with (1) in place (which seems to fix the issue), I think to be safe we also need to join the JoinHandle. Trouble is I am not yet sure where to put the join. It can't go in the Drop impl for BarState since then the thread would probably try to join itself. Perhaps we need to move the ticker state into an Arc<Mutex<Option<TickerState>>> in ProgressBar or something. Then we could rewrite Ticker::run to use, e.g. CondVar::wait_timeout instead of thread::sleep(self.interval).

So more experimenting is necessary.

[djc]

Why do we need to join the thread to be safe? I would argue that, once the Arc has been dropped, the Weak can no longer be upgraded and the ticker thread will just stop.

Dropping the Arc is a good call and I think it's worth merging that.

[chris-laplante]

Why do we need to join the thread to be safe? I would argue that, once the Arc has been dropped, the Weak can no longer be upgraded and the ticker thread will just stop.

I think there's a race condition if main() returns/panics while the Ticker loop is holding onto the Arc (i.e. before it drops it and then sleeps).

[djc]

I think there's a race condition if main() returns/panics while the Ticker loop is holding onto the Arc (i.e. before it drops it and then sleeps).

Have you seen this happen? I'm still confused, can you spell out the order of events for me and why something would be bad?

[chris-laplante]

Have you seen this happen? I'm still confused, can you spell out the order of events for me and why something would be bad?

I have not seen it happen, no. The order of events would be:

1. You install a Ticker and it starts chugging along.
2. main returns or panics, the ProgressBar(s) go out of scope and should be dropped.
3. Simultaneously, let's say the Ticker is executing this line:

   indicatif/src/state.rs

   Line 310 in 515a2a9

   state.draw(false, Instant::now()).ok();

4. Since main returned, the thread is killed while Ticker is executing that line.
5. The Arc is still held, so finish_using_style is never called and the spinner is not cleared.

Granted, it's a small window of opportunity. In practice I'd expect 99% of the time that the Ticker thread is sleeping with the Arc dropped. But I also don't see any reason why we shouldn't join the thread.

[chris-laplante]

Also, to be clear - I certainly may be mistaken :). I'm not super experienced with multi-threaded Rust yet (or Rust in general for that matter). So please take this with a grain of salt.

[djc]

I'm also not super experienced with joining threads -- the Arc<Mutex<Option<TickerState>>> sounds like a lot of complexity for what seems like an edge case (in other words, complexity might be a reason not to join the thread?). Maybe add a Drop impl on ProgressBar that will block on locking the Mutex and sets the BarState::ticker to None?

[djc]

(Also I still think we should merge this without blocking on that other stuff.)

[chris-laplante]

I'm also not super experienced with joining threads -- the Arc<Mutex<Option<TickerState>>> sounds like a lot of complexity for what seems like an edge case (in other words, complexity might be a reason not to join the thread?). Maybe add a Drop impl on ProgressBar that will block on locking the Mutex and sets the BarState::ticker to None?

I'm also fine with merging this for now. And yeah the complexity would kind of suck :/. Since ProgressBar can be cloned, I don't think we can add a drop implementation on it directly.

I'd also be fine with holding off on making any additional changes until (if) the hypothetical issue is uncovered in the wild.

[djc]

I suppose we could also add yet another layer of indirection, ProgressBarInner, which holds a Mutex<BarState>, with ProgressBar holding the ProgressBarInner? That way we could add a Drop on ProgressBarInner...

[chris-laplante]

I suppose we could also add yet another layer of indirection, ProgressBarInner, which holds a Mutex<BarState>, with ProgressBar holding the ProgressBarInner? That way we could add a Drop on ProgressBarInner...

That might be what we have to do. Just to clarify, ProgressBarInner would need to hold Arc<Mutex<BarState>> since we need to give a Weak<Mutex<BarState>> to the Ticker, right?

[djc]

If ProgressBarInner holds the Arc we're back at the current state, where ProgressBar holds the Arc. I'm not entirely sure we actually have a problem if ProgressBar can be cloned? We can still have a Drop impl for it and just make that impl idempotent? Like, it will do work depending on the Arc's strong refcount or something?

[chris-laplante]

If ProgressBarInner holds the Arc we're back at the current state, where ProgressBar holds the Arc.

The difference would be that Ticker would never hold an Arc to ProgressBarInner so it cannot prevent is destruction.

We can still have a Drop impl for it and just make that impl idempotent? Like, it will do work depending on the Arc's strong refcount or something?

Maybe just this?

impl Drop for ProgressBar {
    fn drop(&mut self) {
        let _ = self.state();
    }
}

EDIT: actually maybe that's not strong enough. The above would guarantee the mutex is not held, but not that an Arc isn't held. I'll have to think about your suggestion regarding refcount.

[chris-laplante]

I think checking the strong refcount in drop is off the table, because of this note:

Another thread can change the strong count at any time, including potentially between calling this method and acting on the result.

[chris-laplante]

I've been able to reproduce the race condition by using Barriers to force it to happen. Please give this fork a try: https://github.com/chris-laplante/indicatif/tree/cpl/fun_with_barriers and try running: cargo run --example=long-spinner

By default I have Barrier #1 and #2 uncommented. If you comment out barrier #2 and uncomment #3, you see correct behavior (since executing proceeds after the Arc is dropped). You can also comment out all the Barriers and you should get correct behavior (unless of course, the race happens organically 😉).

[djc]

Awesome work making it testable! Maybe file it as a separate issue? I'm going to try to not think about it too much more today until after work. 😄

[chris-laplante]

Thanks :). And sounds good, and will do!

I'm going to investigate the possibility of adding some automated tests for Ticker, using similar techniques and perhaps mocking with https://docs.rs/mockall/latest/mockall/ or something. For example, it would be cool to be able to single-step the Ticker n times and confirm that the output is what we would expect.

But that will also have to wait until I'm done with some actual work today haha.