via and activate/deactivate chaining

Summary:
Better support and test chaining of `via` and `activate`/`deactivate`.

The real problem is that without the ref qualifier a multiple chain of lvalue references can end up with a destructed object being referenced after.

https://akrzemi1.wordpress.com/2014/06/02/ref-qualifiers/

Test Plan:
This is mostly new tests that would fail and now pass.
I think maybe the tests are a bit weak, it would be good to find a way to ensure we aren't going to see the access-after-free bugs in these tests.

Reviewed By: jsedgwick@fb.com

Subscribers: trunkagent, fugalh, exa, njormrod, folly-diffs@

FB internal diff: D1714873

Tasks: 5489801

Signature: t1:1714873:1417628538:9e610c5ba5e0a22c19a11d53aa956be45d585058

folly/wangle/Future-inl.h

@@ -328,7 +328,7 @@ Try<T>& Future<T>::getTry() {
 
 template <class T>
 template <typename Executor>
-inline Future<T> Future<T>::via(Executor* executor) {
+inline Future<T> Future<T>::via(Executor* executor) && {
   throwIfInvalid();
 
   this->deactivate();
@@ -337,6 +337,17 @@ inline Future<T> Future<T>::via(Executor* executor) {
   return std::move(*this);
 }
 
+template <class T>
+template <typename Executor>
+inline Future<T> Future<T>::via(Executor* executor) & {
+  throwIfInvalid();
+
+  MoveWrapper<Promise<T>> p;
+  auto f = p->getFuture();
+  then([p](Try<T>&& t) mutable { p->fulfilTry(std::move(t)); });
+  return std::move(f).via(executor);
+}
+
 template <class T>
 bool Future<T>::isReady() const {
   throwIfInvalid();

folly/wangle/Future.h

@@ -91,8 +91,17 @@ class Future {
   ///
   ///   f = f.via(e).then(a);
   ///   f.then(b);
+  // The ref-qualifier allows for `this` to be moved out so we
+  // don't get access-after-free situations in chaining.
+  // https://akrzemi1.wordpress.com/2014/06/02/ref-qualifiers/
   template <typename Executor>
-  Future<T> via(Executor* executor);
+  Future<T> via(Executor* executor) &&;
+
+  /// This variant creates a new future, where the ref-qualifier && version
+  /// moves `this` out. This one is less efficient but avoids confusing users
+  /// when "return f.via(x);" fails.
+  template <typename Executor>
+  Future<T> via(Executor* executor) &;
 
   /** True when the result (or exception) is ready. */
   bool isReady() const;
@@ -322,6 +331,7 @@ class Future {
     core_->deactivate();
     return std::move(*this);
   }
+
   bool isActive() {
     return core_->isActive();
   }

folly/wangle/test/FutureTest.cpp

@@ -829,17 +829,14 @@ TEST(Future, callbackAfterActivate) {
 }
 
 TEST(Future, activateOnDestruct) {
-  Promise<void> p;
-  auto f = p.getFuture();
-  f.deactivate();
+  auto f = std::make_shared<Future<void>>(makeFuture());
+  f->deactivate();
 
   size_t count = 0;
-  f.then([&](Try<void>&&) { count++; });
-
-  p.setValue();
+  f->then([&](Try<void>&&) { count++; });
   EXPECT_EQ(0, count);
 
-  f = makeFuture(); // force destruction of old f
+  f.reset();
   EXPECT_EQ(1, count);
 }
 

folly/wangle/test/ViaTest.cpp

@@ -36,7 +36,6 @@ struct ManualWaiter {
 
 struct ViaFixture : public testing::Test {
   ViaFixture() :
-    future_(makeFuture().deactivate()),
     westExecutor(new ManualExecutor),
     eastExecutor(new ManualExecutor),
     waiter(new ManualWaiter(westExecutor)),
@@ -61,7 +60,6 @@ struct ViaFixture : public testing::Test {
     });
   }
 
-  Future<void> future_;
   std::shared_ptr<ManualExecutor> westExecutor;
   std::shared_ptr<ManualExecutor> eastExecutor;
   std::shared_ptr<ManualWaiter> waiter;
@@ -117,9 +115,30 @@ TEST(Via, then_function) {
   EXPECT_EQ(f.value(), "start;static;class-static;class");
 }
 
+TEST_F(ViaFixture, deactivateChain) {
+  bool flag = false;
+  auto f = makeFuture().deactivate();
+  EXPECT_FALSE(f.isActive());
+  auto f2 = f.then([&](Try<void>){ flag = true; });
+  EXPECT_FALSE(flag);
+}
+
+TEST_F(ViaFixture, deactivateActivateChain) {
+  bool flag = false;
+  // you can do this all day long with temporaries.
+  auto f1 = makeFuture().deactivate().activate().deactivate();
+  // Chaining on activate/deactivate requires an rvalue, so you have to move
+  // one of these two ways (if you're not using a temporary).
+  auto f2 = std::move(f1).activate();
+  f2.deactivate();
+  auto f3 = std::move(f2.activate());
+  f3.then([&](Try<void>){ flag = true; });
+  EXPECT_TRUE(flag);
+}
+
 TEST_F(ViaFixture, thread_hops) {
   auto westThreadId = std::this_thread::get_id();
-  auto f = future_.via(eastExecutor.get()).then([=](Try<void>&& t) {
+  auto f = via(eastExecutor.get()).then([=](Try<void>&& t) {
     EXPECT_NE(std::this_thread::get_id(), westThreadId);
     return makeFuture<int>(1);
   }).via(westExecutor.get()
@@ -135,7 +154,7 @@ TEST_F(ViaFixture, thread_hops) {
 
 TEST_F(ViaFixture, chain_vias) {
   auto westThreadId = std::this_thread::get_id();
-  auto f = future_.via(eastExecutor.get()).then([=](Try<void>&& t) {
+  auto f = via(eastExecutor.get()).then([=](Try<void>&& t) {
     EXPECT_NE(std::this_thread::get_id(), westThreadId);
     return makeFuture<int>(1);
   }).then([=](Try<int>&& t) {
@@ -156,3 +175,12 @@ TEST_F(ViaFixture, chain_vias) {
   EXPECT_EQ(f.value(), 1);
 }
 
+TEST_F(ViaFixture, bareViaAssignment) {
+  auto f = via(eastExecutor.get());
+}
+TEST_F(ViaFixture, viaAssignment) {
+  // via()&&
+  auto f = makeFuture().via(eastExecutor.get());
+  // via()&
+  auto f2 = f.via(eastExecutor.get());
+}