execute callbacks as rvalue references

Summary:
Callable objects may implement a separate `operator()` for when
they are called as rvalue references. For example, `folly::partial`
will move captured objects to the function call when invoked as
rvalue reference. That allows e.g. to capture pass a `unique_ptr`
to `folly::partial` for a function that takes a `unique_ptr` by
value or rvalue-reference.

Callbacks for `folly::Future`s are only ever executed once. They
may consume captured data. If the callback is an object that
implements a `operator()() &&`, then that one should be invoked.

This diff makes sure, callbacks passed to `folly::Future` are
invoked as rvalue references.

Reviewed By: ericniebler, fugalh

Differential Revision: D4404186

fbshipit-source-id: 9f33e442a634acb8797183d3d869840d85bd5d15

diff --git a/folly/futures/Future-inl.h b/folly/futures/Future-inl.h
index cf3e334437ca440899e7a468770e6b3494c1a09f..28a5ffaa8f3f611ccdd44dfadbf04446ae77e137 100644 (file)
--- a/folly/futures/Future-inl.h
+++ b/folly/futures/Future-inl.h
@@ -159,14 +159,16 @@ Future<T>::thenImplementation(F&& func, detail::argResult<isTry, F, Args...>) {
      in some circumstances, but I think it should be explicit not implicit
      in the destruction of the Future used to create it.
      */
-  setCallback_([ funcm = std::forward<F>(func), pm = std::move(p) ](
-      Try<T> && t) mutable {
-    if (!isTry && t.hasException()) {
-      pm.setException(std::move(t.exception()));
-    } else {
-      pm.setWith([&]() { return funcm(t.template get<isTry, Args>()...); });
-    }
-  });
+  setCallback_(
+      [ func = std::forward<F>(func), pm = std::move(p) ](Try<T> && t) mutable {
+        if (!isTry && t.hasException()) {
+          pm.setException(std::move(t.exception()));
+        } else {
+          pm.setWith([&]() {
+            return std::move(func)(t.template get<isTry, Args>()...);
+          });
+        }
+      });
 
   return f;
 }
@@ -189,14 +191,14 @@ Future<T>::thenImplementation(F&& func, detail::argResult<isTry, F, Args...>) {
   auto f = p.getFuture();
   f.core_->setExecutorNoLock(getExecutor());
 
-  setCallback_([ funcm = std::forward<F>(func), pm = std::move(p) ](
+  setCallback_([ func = std::forward<F>(func), pm = std::move(p) ](
       Try<T> && t) mutable {
     auto ew = [&] {
       if (!isTry && t.hasException()) {
         return std::move(t.exception());
       } else {
         try {
-          auto f2 = funcm(t.template get<isTry, Args>()...);
+          auto f2 = std::move(func)(t.template get<isTry, Args>()...);
           // that didn't throw, now we can steal p
           f2.setCallback_([p = std::move(pm)](Try<B> && b) mutable {
             p.setTry(std::move(b));
@@ -263,13 +265,14 @@ Future<T>::onError(F&& func) {
   p.core_->setInterruptHandlerNoLock(core_->getInterruptHandler());
   auto f = p.getFuture();
 
-  setCallback_([ funcm = std::forward<F>(func), pm = std::move(p) ](
-      Try<T> && t) mutable {
-    if (!t.template withException<Exn>(
-            [&](Exn& e) { pm.setWith([&] { return funcm(e); }); })) {
-      pm.setTry(std::move(t));
-    }
-  });
+  setCallback_(
+      [ func = std::forward<F>(func), pm = std::move(p) ](Try<T> && t) mutable {
+        if (!t.template withException<Exn>([&](Exn& e) {
+              pm.setWith([&] { return std::move(func)(e); });
+            })) {
+          pm.setTry(std::move(t));
+        }
+      });
 
   return f;
 }
@@ -290,12 +293,12 @@ Future<T>::onError(F&& func) {
   Promise<T> p;
   auto f = p.getFuture();
 
-  setCallback_([ pm = std::move(p), funcm = std::forward<F>(func) ](
+  setCallback_([ pm = std::move(p), func = std::forward<F>(func) ](
       Try<T> && t) mutable {
     if (!t.template withException<Exn>([&](Exn& e) {
           auto ew = [&] {
             try {
-              auto f2 = funcm(e);
+              auto f2 = std::move(func)(e);
               f2.setCallback_([pm = std::move(pm)](Try<T> && t2) mutable {
                 pm.setTry(std::move(t2));
               });
@@ -346,11 +349,11 @@ Future<T>::onError(F&& func) {
   Promise<T> p;
   auto f = p.getFuture();
   setCallback_(
-      [ pm = std::move(p), funcm = std::forward<F>(func) ](Try<T> t) mutable {
+      [ pm = std::move(p), func = std::forward<F>(func) ](Try<T> t) mutable {
         if (t.hasException()) {
           auto ew = [&] {
             try {
-              auto f2 = funcm(std::move(t.exception()));
+              auto f2 = std::move(func)(std::move(t.exception()));
               f2.setCallback_([pm = std::move(pm)](Try<T> t2) mutable {
                 pm.setTry(std::move(t2));
               });
@@ -387,9 +390,9 @@ Future<T>::onError(F&& func) {
   Promise<T> p;
   auto f = p.getFuture();
   setCallback_(
-      [ pm = std::move(p), funcm = std::forward<F>(func) ](Try<T> t) mutable {
+      [ pm = std::move(p), func = std::forward<F>(func) ](Try<T> t) mutable {
         if (t.hasException()) {
-          pm.setWith([&] { return funcm(std::move(t.exception())); });
+          pm.setWith([&] { return std::move(func)(std::move(t.exception())); });
         } else {
           pm.setTry(std::move(t));
         }

diff --git a/folly/futures/test/FutureTest.cpp b/folly/futures/test/FutureTest.cpp
index f5adcf44195401d51cff1b654bb57865a485eea3..bad230bcb6644b5d0b466b54a310f16595bd208f 100644 (file)
--- a/folly/futures/test/FutureTest.cpp
+++ b/folly/futures/test/FutureTest.cpp
@@ -846,3 +846,49 @@ TEST(Future, RequestContext) {
 TEST(Future, makeFutureNoThrow) {
   makeFuture().value();
 }
+
+TEST(Future, invokeCallbackReturningValueAsRvalue) {
+  struct Foo {
+    int operator()(int x) & {
+      return x + 1;
+    }
+    int operator()(int x) const& {
+      return x + 2;
+    }
+    int operator()(int x) && {
+      return x + 3;
+    }
+  };
+
+  Foo foo;
+  Foo const cfoo;
+
+  // The callback will be copied when given as lvalue or const ref, and moved
+  // if provided as rvalue. Either way, it should be executed as rvalue.
+  EXPECT_EQ(103, makeFuture<int>(100).then(foo).value());
+  EXPECT_EQ(203, makeFuture<int>(200).then(cfoo).value());
+  EXPECT_EQ(303, makeFuture<int>(300).then(Foo()).value());
+}
+
+TEST(Future, invokeCallbackReturningFutureAsRvalue) {
+  struct Foo {
+    Future<int> operator()(int x) & {
+      return x + 1;
+    }
+    Future<int> operator()(int x) const& {
+      return x + 2;
+    }
+    Future<int> operator()(int x) && {
+      return x + 3;
+    }
+  };
+
+  Foo foo;
+  Foo const cfoo;
+
+  // The callback will be copied when given as lvalue or const ref, and moved
+  // if provided as rvalue. Either way, it should be executed as rvalue.
+  EXPECT_EQ(103, makeFuture<int>(100).then(foo).value());
+  EXPECT_EQ(203, makeFuture<int>(200).then(cfoo).value());
+  EXPECT_EQ(303, makeFuture<int>(300).then(Foo()).value());
+}