Commit e1c7e6ce authored by Maged Michael's avatar Maged Michael Committed by Facebook GitHub Bot

RequestContext: Fix race condition in releasing clear and delete counts of RequestData

Summary:
Fix race condition between releaseRefClearDelete and releaseRefDeleteOnly by fixing handling in releaseRefClearDelete of the case where the clear count is 1 and the delete count is > 1.

The correct behavior of releaseRefClearDelete when delete count > 1 is to release the clear count first and then release the delete count, instead of releasing the two counts in one step.

The following sequence of events demonstrates the problem in the absence of the fix:
- clear count == 1 and delete count == 2
- Thread 1 calls releaseRefClearDeleteCount
- Thread 2 calls releaseRefDeleteOnly
- Thread 1 atomically decrements the counts to 0 and 1
- Thread 2 atomically decrements the delete count to 0
- Thread 2 deletes the RequestData object
- Thread 1 call to onClear accesses deleted object

Reviewed By: yfeldblum, davidtgoldblatt

Differential Revision: D21705266

fbshipit-source-id: 8ec2151c4e98968beaef7e9ccec0a786ca5ae267
parent ad1c3b8e
......@@ -103,18 +103,21 @@ void RequestData::releaseRefDeleteOnly() {
FOLLY_ALWAYS_INLINE
void RequestData::releaseRefClearDelete() {
auto rc = keepAliveCounter_.fetch_sub(
kClearCount + kDeleteCount, std::memory_order_acq_rel) -
(kClearCount + kDeleteCount);
DCHECK_GE(rc, 0);
if (rc < kClearCount) {
auto rc = keepAliveCounter_.load(std::memory_order_acquire);
if (FOLLY_LIKELY(rc == (kClearCount + kDeleteCount))) {
this->onClear();
}
if (rc == 0) {
delete this;
} else {
releaseRefClearDeleteSlow();
}
}
FOLLY_NOINLINE
void RequestData::releaseRefClearDeleteSlow() {
releaseRefClearOnly();
releaseRefDeleteOnly();
}
void RequestData::DestructPtr::operator()(RequestData* ptr) {
if (ptr) {
auto keepAliveCounter =
......
......@@ -140,6 +140,7 @@ class RequestData {
void releaseRefDeleteOnly();
// Decrement the reference count. Clear and delete if last.
void releaseRefClearDelete();
void releaseRefClearDeleteSlow();
// Unique ptr with custom destructor, decrement the counter
// and only free if 0
......
......@@ -455,3 +455,47 @@ TEST_F(RequestContextTest, OverwriteNullData) {
EXPECT_NE(folly::RequestContext::get()->getContextData("token"), nullptr);
}
}
TEST_F(RequestContextTest, ConcurrentDataRefRelease) {
for (int i = 0; i < 100; ++i) {
std::atomic<int> step{0};
std::shared_ptr<folly::RequestContext> sp1;
auto th1 = std::thread([&]() {
folly::RequestContextScopeGuard g0; // Creates ctx0.
setData(); // Creates data0 with one reference in ctx0.
{
folly::ShallowCopyRequestContextScopeGuard g1;
// g1 created ctx1 with second reference to data0.
EXPECT_NE(&getData(), nullptr);
// Keep shared_ptr to ctx1 to pass to th2
sp1 = folly::RequestContext::saveContext();
step.store(1); // sp1 is ready.
while (step.load() < 2)
/* Wait for th2 to clear reference to data0. */;
}
// End of g2 released shared_ptr to ctx1, switched back to ctx0
// At this point:
// - One shared_ptr to ctx0, held by th1.
// - One shared_ptr to ctx1, help by th2.
// - data0 has one clear count (for reference from ctx0) and
// two delete counts (one each from ctx0 and ctx1).
step.store(3);
// End of g1 will destroy ctx0, release clear/delete counts for data0.
});
auto th2 = std::thread([&]() {
while (step.load() < 1)
/* Wait for th1 to set sp1. */;
folly::RequestContextScopeGuard g2(std::move(sp1));
// g2 set context to ctx1.
EXPECT_EQ(sp1.get(), nullptr);
EXPECT_NE(&getData(), nullptr);
clearData();
step.store(2); // th2 cleared reference to data0 in ctx1.
while (step.load() < 3)
/* Wait for th1 to release shared_ptr to ctx1. */;
// End of g2 will destroy ctx1, release delete count for data0.
});
th1.join();
th2.join();
}
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment