coro::SharedPromise move deadlock avoidance

Summary: tsan reports a lock order inversion when using the swap algorithm as the move-construct/assign has nested locks which are naturally acquired in the opposite order by swap.

Reviewed By: ispeters

Differential Revision: D70357682

fbshipit-source-id: dd1bbe6b60fc01b4819ef1eca59e044363093fbe

folly/coro/SharedPromise.h

@@ -16,6 +16,11 @@
 
 #pragma once
 
+#include <cstddef>
+#include <type_traits>
+#include <utility>
+
+#include <folly/Likely.h>
 #include <folly/Synchronized.h>
 #include <folly/Utility.h>
 #include <folly/coro/Promise.h>
@@ -98,19 +103,17 @@ class SharedPromise {
 };
 
 template <typename T>
-SharedPromise<T>::SharedPromise(SharedPromise&& other) noexcept {
-  *this = std::move(other);
-}
+SharedPromise<T>::SharedPromise(SharedPromise&& other) noexcept
+    : state_{std::exchange(*other.state_.wlock(), {})} {}
 
 template <typename T>
 SharedPromise<T>& SharedPromise<T>::operator=(SharedPromise&& other) noexcept {
-  // unlike folly::SharedPromise, we synchronize here
-  state_.withWLock([&](auto& state) {
-    other.state_.withWLock([&](auto& otherState) {
-      state = std::exchange(otherState, {});
-    });
-  });
-
+  if (FOLLY_LIKELY(this != &other)) {
+    synchronized(
+        [](auto self, auto other) { *self = std::exchange(*other, {}); },
+        wlock(state_),
+        wlock(other.state_));
+  }
   return *this;
 }
 

folly/coro/test/BUCK

@@ -305,6 +305,7 @@ cpp_unittest(
     deps = [
         "//folly/coro:blocking_wait",
         "//folly/coro:detach_on_cancel",
+        "//folly/coro:gtest_helpers",
         "//folly/coro:shared_promise",
         "//folly/executors:cpu_thread_pool_executor",
         "//folly/portability:gtest",

folly/coro/test/SharedPromiseTest.cpp

@@ -14,8 +14,11 @@
  * limitations under the License.
  */
 
+#include <utility>
+
 #include <folly/coro/BlockingWait.h>
 #include <folly/coro/DetachOnCancel.h>
+#include <folly/coro/GtestHelpers.h>
 #include <folly/coro/SharedPromise.h>
 #include <folly/executors/CPUThreadPoolExecutor.h>
 
@@ -304,4 +307,39 @@ TEST(SharedPromiseTest, BasicVoid) {
   }
 }
 
+CO_TEST(SharedPromiseTest, Swap) {
+  SharedPromise<int> p1, p2;
+  p1.setValue(42);
+  p2.setValue(43);
+  using std::swap;
+  swap(p1, p2);
+  EXPECT_EQ(co_await p1.getFuture(), 43);
+  EXPECT_EQ(co_await p2.getFuture(), 42);
+}
+
+CO_TEST(SharedPromiseTest, MoveFrom) {
+  SharedPromise<int> p1;
+  p1.setValue(42);
+  SharedPromise<int> p2;
+  p2.setValue(43);
+  p2 = std::move(p1);
+  EXPECT_EQ(co_await p2.getFuture(), 42);
+  EXPECT_FALSE(p1.isFulfilled());
+}
+
+CO_TEST(SharedPromiseTest, SelfMove) {
+  SharedPromise<int> p;
+  p.setValue(1);
+  auto& alias = p; // defeat -Wself-move
+  p = std::move(alias);
+  CO_ASSERT_TRUE(p.isFulfilled());
+  EXPECT_EQ(co_await p.getFuture(), 1);
+}
+
+TEST(SharedPromiseTest, Exchange) {
+  SharedPromise<void> p1;
+  using std::exchange;
+  SharedPromise<void> p2 = exchange(p1, {});
+}
+
 #endif