Modification to futures to remove deadlock in certain use cases for getVia(executor).

Summary: If getVia was called on a future modified using via, getVia could deadlock if the original future was updated to a new executor and there was no callback chained after the call to via. In effect: f.via(executor).getVia(executor); deadlocks. This can be a problem if the code is hidden in a library and the precise semantics are unclear. This diff adds a test that exposes the problem and a fix by forcing waitVia to add a callback that will satisfy the new exector, ensuring that drive() has a callback to trigger once the future is satisfied.

Reviewed By: andriigrynenko

Differential Revision: D2906858

fb-gh-sync-id: a3105079530f15d7a7d39a9381c4078665b721a7
shipit-source-id: a3105079530f15d7a7d39a9381c4078665b721a7

diff --git a/folly/futures/Future-inl.h b/folly/futures/Future-inl.h
index 453d10547d9d8c0908511e25e3df6e8f7b090d15..eabc61a524ded3835d0ee2990dee146c5ee0c4d0 100644 (file)
--- a/folly/futures/Future-inl.h
+++ b/folly/futures/Future-inl.h
@@ -983,9 +983,16 @@ void waitImpl(Future<T>& f, Duration dur) {
 
 template <class T>
 void waitViaImpl(Future<T>& f, DrivableExecutor* e) {
+  // Set callback so to ensure that the via executor has something on it
+  // so that once the preceding future triggers this callback, drive will
+  // always have a callback to satisfy it
+  if (f.isReady())
+    return;
+  f = f.then([](T&& t) { return std::move(t); });
   while (!f.isReady()) {
     e->drive();
   }
+  assert(f.isReady());
 }
 
 } // detail

diff --git a/folly/futures/test/ExecutorTest.cpp b/folly/futures/test/ExecutorTest.cpp
index 997c9f4b0a36694818628f4c5f4e896ca219c8a6..b29846a75ae3bbe6570f2ecc5282c90761b9be7f 100644 (file)
--- a/folly/futures/test/ExecutorTest.cpp
+++ b/folly/futures/test/ExecutorTest.cpp
@@ -106,6 +106,21 @@ TEST(ManualExecutor, waitForDoesNotDeadlock) {
   t.join();
 }
 
+TEST(ManualExecutor, getViaDoesNotDeadlock) {
+  ManualExecutor east, west;
+  folly::Baton<> baton;
+  auto f = makeFuture().via(&east).then([](Try<Unit>) {
+    return makeFuture();
+  }).via(&west);
+  std::thread t([&] {
+    baton.post();
+    f.getVia(&west);
+  });
+  baton.wait();
+  east.run();
+  t.join();
+}
+
 TEST(Executor, InlineExecutor) {
   InlineExecutor x;
   size_t counter = 0;