Skip to content

Commit ce08fd2

Browse files
David Devecseryfacebook-github-bot
authored andcommitted
Fix data-race in folly::coro::detachOnCancel
Summary: the relaxed memory order on the atomic exchange operations in detachOnCancel does not enforce a happens-before relationship between the release of the unique_ptr and the visibility of the exhanged value. This means that any future accesses to the unique_ptr will actually (technically) race with its destruction. This was causing failures in at least one test case for cpentity. A more detailed writeup of the investigation can be found here: https://fb.workplace.com/groups/cpentity.investigations/permalink/9858481294195756/ The change from relaxed to acq_rel consistency should have no performance impact on x86 systems (due to the TSO memory ordering), and likely neglibile impact on systems with weaker memory models. Reviewed By: iahs Differential Revision: D73457949 fbshipit-source-id: 0ef02486069db59396192a2f04b64dc78bdf551a
1 parent 59ef424 commit ce08fd2

File tree

1 file changed

+2
-2
lines changed

1 file changed

+2
-2
lines changed

third-party/folly/src/folly/coro/DetachOnCancel.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ Task<semi_await_result_t<Awaitable>> detachOnCancel(Awaitable awaitable) {
6262
.startInlineUnsafe(
6363
[postedPtr = posted.get(), &baton, &result](auto&& r) {
6464
std::unique_ptr<std::atomic<bool>> p(postedPtr);
65-
if (!p->exchange(true, std::memory_order_relaxed)) {
65+
if (!p->exchange(true, std::memory_order_acq_rel)) {
6666
p.release();
6767
tryAssign(result, std::move(r));
6868
baton.post();
@@ -74,7 +74,7 @@ Task<semi_await_result_t<Awaitable>> detachOnCancel(Awaitable awaitable) {
7474
{
7575
CancellationCallback cancelCallback(
7676
co_await co_current_cancellation_token, [&posted, &baton, &result] {
77-
if (!posted->exchange(true, std::memory_order_relaxed)) {
77+
if (!posted->exchange(true, std::memory_order_acq_rel)) {
7878
posted.release();
7979
result.emplaceException(folly::OperationCancelled{});
8080
baton.post();

0 commit comments

Comments
 (0)