Skip to content

Commit

Permalink
SE: Fix possible race condition
Browse files Browse the repository at this point in the history
Consider this scenario:
- `retries_remaining` starts at 1, so we have 1 main execution and
  1 additional speculative execution to spawn.
- Main execution does some retries and exhausts execution plan this way.
- It sends the last attempt to DB and waits for response.
- Now the speculative execution starts.
- The plan is empty, so it immediately finishes and enters `None` block
  of the `match`.
- `async_tasks` is not empty, because main execution is still ongoing,
  so nothing happens.
- Main execution finishes with ignorable error, e.g. broken connection.
- `can_be_ignored` returns true, so we don't return and only assign to
  `last_error`
- Now `async_tasks` is empty, so `async_tasks.select_next_some()` branch
  of `select!` will never be executed again.
- Only `&mut sleep` will be executed (once) and do nothing because
  `retries_remaining == 0`.
- That means we are hanging forever - or until client timeout happens
  if one was configured.

This commit fixes this bug by simplifying the logic.
The new logic always checks if the resolved task is the last one, and
if so it returns, so it's not possible for the tasks to be exhausted
without finishing the `select!`.
  • Loading branch information
Lorak-mmk committed Oct 9, 2024
1 parent ea7c464 commit f44942d
Showing 1 changed file with 11 additions and 15 deletions.
26 changes: 11 additions & 15 deletions scylla/src/transport/speculative_execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,21 +127,17 @@ where
}
}
res = async_tasks.select_next_some() => {
match res {
Some(r) => {
if !can_be_ignored(&r) {
return r;
} else {
last_error = Some(r)
}
},
None => {
if async_tasks.is_empty() && retries_remaining == 0 {
return last_error.unwrap_or({
Err(EMPTY_PLAN_ERROR)
});
}
},
if let Some(r) = res {
if !can_be_ignored(&r) {
return r;
} else {
last_error = Some(r)
}
}
if async_tasks.is_empty() && retries_remaining == 0 {
return last_error.unwrap_or({
Err(EMPTY_PLAN_ERROR)
});
}
}
}
Expand Down

0 comments on commit f44942d

Please sign in to comment.