Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extremely unlikely race conditions in internal::AsyncRetryLoop() #7788

Closed
coryan opened this issue Dec 28, 2021 · 0 comments · Fixed by #7789
Closed

Extremely unlikely race conditions in internal::AsyncRetryLoop() #7788

coryan opened this issue Dec 28, 2021 · 0 comments · Fixed by #7789
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@coryan
Copy link
Contributor

coryan commented Dec 28, 2021

Consider the current implementation of the asynchronous retry loop, in particular how the pending_operation_ is set after the callback may have been invoked:

auto op =
functor_(cq_, std::move(context), request_).then([self](future<T> f) {
self->OnAttempt(f.get());
});
SetWaiting(std::move(op));

If the callback is immediately completed (or the scheduler puts a long sleep before line 115) the pending_operation_ may be set to a future that is no longer active, which means the cancel request has no effect:

void Cancel() {
std::unique_lock<std::mutex> lk(mu_);
cancelled_ = true;
if (state_ != kWaiting) return;
future<void> f = std::move(pending_operation_);
state_ = kIdle;
lk.unlock();
f.cancel();
}

This is extremely rare, it would require multiple operations completing very fast, and separate threads going to sleep. Nevertheless, we should fix this.

@coryan coryan added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Dec 28, 2021
@coryan coryan self-assigned this Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant