From 030427e249e9257008fa9ccb88c0d996fd04b40f Mon Sep 17 00:00:00 2001
From: Lee Howes <lwh@fb.com>
Date: Mon, 15 Feb 2016 13:22:30 -0800
Subject: [PATCH] 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
---
 folly/futures/Future-inl.h          |  7 +++++++
 folly/futures/test/ExecutorTest.cpp | 15 +++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/folly/futures/Future-inl.h b/folly/futures/Future-inl.h
index 453d1054..eabc61a5 100644
--- 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 997c9f4b..b29846a7 100644
--- 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;
-- 
2.34.1