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

diff --git a/folly/wangle/Future-inl.h b/folly/wangle/Future-inl.h
index 06d225b02ade88f41cd7a69de21722fff8f01bb5..2df6fef2b38aeae3cb7c42275236cacc5bbd3ecd 100644 (file)
--- a/folly/wangle/Future-inl.h
+++ b/folly/wangle/Future-inl.h
@@ -328,7 +328,7 @@ Try<T>& Future<T>::getTry() {
 
 template <class T>
 template <typename Executor>
 
 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();
   throwIfInvalid();
 
   this->deactivate();


@@ -337,6 +337,17 @@ inline Future<T> Future<T>::via(Executor* executor) {
   return std::move(*this);
 }
 
   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();
 template <class T>
 bool Future<T>::isReady() const {
   throwIfInvalid();

diff --git a/folly/wangle/Future.h b/folly/wangle/Future.h
index 57f8636b8babaea859402bc9b3924d35a50c3650..f124d7b5f1a1e4116ef7545de91fc62a65be2cd7 100644 (file)
--- a/folly/wangle/Future.h
+++ b/folly/wangle/Future.h
@@ -91,8 +91,17 @@ class Future {
   ///
   ///   f = f.via(e).then(a);
   ///   f.then(b);
   ///
   ///   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>
   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;
 
   /** True when the result (or exception) is ready. */
   bool isReady() const;


@@ -322,6 +331,7 @@ class Future {
     core_->deactivate();
     return std::move(*this);
   }
     core_->deactivate();
     return std::move(*this);
   }

+

   bool isActive() {
     return core_->isActive();
   }
   bool isActive() {
     return core_->isActive();
   }

diff --git a/folly/wangle/test/FutureTest.cpp b/folly/wangle/test/FutureTest.cpp
index db15993c2b25c0db2ba55fd75d5327ffa5ca11dd..eaae5da11df456233a5f8ddb109b150274cd1c7b 100644 (file)
--- a/folly/wangle/test/FutureTest.cpp
+++ b/folly/wangle/test/FutureTest.cpp
@@ -829,17 +829,14 @@ TEST(Future, callbackAfterActivate) {
 }
 
 TEST(Future, activateOnDestruct) {
 }
 
 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;
 
   size_t count = 0;

-  f.then([&](Try<void>&&) { count++; });
-
-  p.setValue();
+  f->then([&](Try<void>&&) { count++; });

   EXPECT_EQ(0, count);
 
   EXPECT_EQ(0, count);
 

-  f = makeFuture(); // force destruction of old f
+  f.reset();

   EXPECT_EQ(1, count);
 }
 
   EXPECT_EQ(1, count);
 }
 

diff --git a/folly/wangle/test/ViaTest.cpp b/folly/wangle/test/ViaTest.cpp
index 9a3758c28c9e9875b0c1e06c323769088ae6cee8..3e9953e0bd80097edf7da177a49ed34f4c486baa 100644 (file)
--- a/folly/wangle/test/ViaTest.cpp
+++ b/folly/wangle/test/ViaTest.cpp
@@ -36,7 +36,6 @@ struct ManualWaiter {
 
 struct ViaFixture : public testing::Test {
   ViaFixture() :
 
 struct ViaFixture : public testing::Test {
   ViaFixture() :

-    future_(makeFuture().deactivate()),

     westExecutor(new ManualExecutor),
     eastExecutor(new ManualExecutor),
     waiter(new ManualWaiter(westExecutor)),
     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;
   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");
 }
 
   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();
 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()
     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();
 
 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) {
     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);
 }
 
   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());
+}