Commit eec32a76 authored by Hans Fugal's avatar Hans Fugal Committed by dcsommer

(wangle) fix race in Core::detachOne()

Summary:
In D1618240 I introduced a race condition in `detachOne()`, where `detached_` is incremented and then tested. If the promise and future are racing, they can both see `detached_ == 2` in the conditional, and then they'll both try to free the Core object. This fixes that.

It also fixes a related problem (which actually showed up more often with the test I wrote), where we transition into the Done state before setting the value, and then `maybeCallback()` observes the state is Done (because we're just reading an atomic, not grabbing the lock, which is intentional), tries to execute the callback, but `folly::Optional` throws an exception because the value hasn't been set yet (at least in debug it does). I should have listened to my gut and kept the state assignment after the transition action in the first place.

Test Plan: New unit test

Reviewed By: jsedgwick@fb.com

Subscribers: trunkagent, net-systems@, fugalh, exa, njormrod, folly-diffs@, mnd

FB internal diff: D1636490

Tasks: 5438209

Blame Revision: D1618240
parent 3d6fc64f
...@@ -205,9 +205,10 @@ class Core : protected FSM<State> { ...@@ -205,9 +205,10 @@ class Core : protected FSM<State> {
} }
void detachOne() { void detachOne() {
++detached_; auto d = ++detached_;
assert(detached_ == 1 || detached_ == 2); assert(d >= 1);
if (detached_ == 2) { assert(d <= 2);
if (d == 2) {
// we should have already executed the callback with the value // we should have already executed the callback with the value
assert(calledBack_); assert(calledBack_);
delete this; delete this;
......
...@@ -48,13 +48,14 @@ public: ...@@ -48,13 +48,14 @@ public:
} }
/// Atomically do a state transition with accompanying action. /// Atomically do a state transition with accompanying action.
/// The action will see the old state.
/// @returns true on success, false and action unexecuted otherwise /// @returns true on success, false and action unexecuted otherwise
template <class F> template <class F>
bool updateState(Enum A, Enum B, F const& action) { bool updateState(Enum A, Enum B, F const& action) {
std::lock_guard<Mutex> lock(mutex_); std::lock_guard<Mutex> lock(mutex_);
if (state_ != A) return false; if (state_ != A) return false;
state_ = B;
action(); action();
state_ = B;
return true; return true;
} }
...@@ -82,6 +83,9 @@ public: ...@@ -82,6 +83,9 @@ public:
/// } /// }
/// /* do unprotected stuff */ /// /* do unprotected stuff */
/// return; // or otherwise break out of the loop /// return; // or otherwise break out of the loop
///
/// The protected action will see the old state, and the unprotected action
/// will see the new state.
template <class F1, class F2> template <class F1, class F2>
bool updateState(Enum A, Enum B, bool updateState(Enum A, Enum B,
F1 const& protectedAction, F2 const& unprotectedAction) { F1 const& protectedAction, F2 const& unprotectedAction) {
......
...@@ -108,8 +108,8 @@ TEST(FSM, noActionOnBadUpdate) { ...@@ -108,8 +108,8 @@ TEST(FSM, noActionOnBadUpdate) {
EXPECT_EQ(0, count); EXPECT_EQ(0, count);
} }
TEST(FSM, stateTransitionBeforeAction) { TEST(FSM, stateTransitionAfterAction) {
FSM<State> fsm(State::A); FSM<State> fsm(State::A);
fsm.updateState(State::A, State::B, fsm.updateState(State::A, State::B,
[&]{ EXPECT_EQ(State::B, fsm.getState()); }); [&]{ EXPECT_EQ(State::A, fsm.getState()); });
} }
...@@ -23,6 +23,7 @@ ...@@ -23,6 +23,7 @@
#include <thread> #include <thread>
#include <type_traits> #include <type_traits>
#include <unistd.h> #include <unistd.h>
#include <folly/Memory.h>
#include <folly/wangle/Executor.h> #include <folly/wangle/Executor.h>
#include <folly/wangle/Future.h> #include <folly/wangle/Future.h>
#include <folly/wangle/ManualExecutor.h> #include <folly/wangle/ManualExecutor.h>
...@@ -670,7 +671,6 @@ TEST(Future, waitWithSemaphore) { ...@@ -670,7 +671,6 @@ TEST(Future, waitWithSemaphore) {
}); });
flag = true; flag = true;
result.store(waitWithSemaphore(std::move(n)).value()); result.store(waitWithSemaphore(std::move(n)).value());
LOG(INFO) << result;
}, },
std::move(f) std::move(f)
); );
...@@ -847,3 +847,26 @@ TEST(Future, getFuture_after_setException) { ...@@ -847,3 +847,26 @@ TEST(Future, getFuture_after_setException) {
p.fulfil([]() -> void { throw std::logic_error("foo"); }); p.fulfil([]() -> void { throw std::logic_error("foo"); });
EXPECT_THROW(p.getFuture().value(), std::logic_error); EXPECT_THROW(p.getFuture().value(), std::logic_error);
} }
TEST(Future, detachRace) {
// Task #5438209
// This test is designed to detect a race that was in Core::detachOne()
// where detached_ was incremented and then tested, and that
// allowed a race where both Promise and Future would think they were the
// second and both try to delete. This showed up at scale but was very
// difficult to reliably repro in a test. As it is, this only fails about
// once in every 1,000 executions. Doing this 1,000 times is going to make a
// slow test so I won't do that but if it ever fails, take it seriously, and
// run the test binary with "--gtest_repeat=10000 --gtest_filter=*detachRace"
// (Don't forget to enable ASAN)
auto p = folly::make_unique<Promise<bool>>();
auto f = folly::make_unique<Future<bool>>(p->getFuture());
folly::Baton<> baton;
std::thread t1([&]{
baton.post();
p.reset();
});
baton.wait();
f.reset();
t1.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