MultiProgress:insert_{after, before}: grab pb index immediately to avoid deadlock with Ticker #424
Conversation
…oid deadlock with Ticker
Before this change it is possible for a combination of Ticker and
MultiProgress to deadlock due a lock inversion. Consider this code:
let mp = MultiProgress::new();
let s = mp.add(ProgressBar::new_spinner());
s.enable_steady_tick(Duration::from_millis(50));
let p0 = mp.insert_after(&s, ProgressBar::new(10));
Inside `Ticker`, a lock on `BarState` is acquired and then the state is
drawn to the draw target via `BarState::draw()`. `draw` calls
`self.draw_target.width()` which in the case of `MultiProgress` locks
`MultiState`.
Meanwhile, the call to `insert_after` calls `MultiProgress::internalize`
which locks the `MultiState`. Then `MultiState::insert` (in the case of
`insert_after` and `insert_before`) locks the `BarState` in order to
read the index.
So we have thread 1 locking in this order: `BarState`, `MultiState`
and thread 2 locking in this order: `MultiState`, `BarState`
== boom
The fix is to change `insert_after` and `insert_before` to immediately
grab the index of the progress bar rather than deferring it to be done
when the `MultiState` is already locked. This makes the locking order of
thread 2 the same as that of 1.
|
Are there any structural/type-level changes we could put in place to make this harder to regress? Is it possible to write a test for this? Is this a regression from 0.16? |
I think I would be able to come up with a test using our friend the It may be a regression, I'm not sure - the code for handling draw states was changed after I added the Not sure about structural changes. I think as long as multiple mutexes are in the play the danger is always there. I'm going to play with using |
Before this change it is possible for a combination of Ticker and
MultiProgress to deadlock due a lock inversion. Consider this code:
Inside
Ticker, a lock onBarStateis acquired and then the state isdrawn to the draw target via
BarState::draw().drawcallsself.draw_target.width()which in the case ofMultiProgresslocksMultiState.Meanwhile, the call to
insert_aftercallsMultiProgress::internalizewhich locks the
MultiState. ThenMultiState::insert(in the case ofinsert_afterandinsert_before) locks theBarStatein order toread the index.
So we have thread 1 locking in this order:
BarState,MultiStateand thread 2 locking in this order:
MultiState,BarState== boom
The fix is to change
insert_afterandinsert_beforeto immediatelygrab the index of the progress bar rather than deferring it to be done
when the
MultiStateis already locked. This makes the locking order ofthread 2 the same as that of 1.