folly/futures: fix early release of non-embedded callbacks

Summary:
folly::Future (more precisely folly::detail::Core) can store
callback functors (typically lambdas) up to a certain size
(8*sizeof(void*)) inside the main object. Only bigger functors
are stored in the std::function object in folly::detail::Core,
which will put them on the heap.

The behaviour of folly::Future is slightly different depending
on whether the callback can be embedded in the main object
or not. Small functors will be destructed after the callback
is executed. Functors too big to fit in the lambda space in
the Core object will be deleted when Core is deleted.

Some code assumes that functor objects are deleted as soon
as the callback has been executed. The implementations of
folly::Future::collect and variants do so. If you switch
off this optimisation temporarily (which can be done easily
by setting lambdaBufSize in folly/futures/detail/Core.h to
0), a number of unit tests fail.

Given that the lambda buffer is reasonably large, most
functors can be stored in the Core object. The different
behaviour for very large lambdas may not have been observed.

This diff fixes the inconsitent behaviour.

Firstly, it changes the unit test Future:finish to explicitly
check that the callback functor is deleted after the callback
has been executed. This should be tested, because this
behaviour is assumed in other parts of the futures
implementation (e.g. Future::collect et al.).

Secondly, it adds another unit test Future:finishBigLambda,
similar to Future:finish. The lambda captures much more data
to make sure the lambda won't be stored in the Core object,
but in the std::function object inside Core. The test verifies
that the behaviour is the same, i.e. the callback functor
is destructed after the callback has been executed.

Thirdly, it fixes Core and makes sure that functors of any
size are destructued after the callback has been called. The
new unit test fails without this.

Reviewed By: fugalh

Differential Revision: D2883772

fb-gh-sync-id: 21a410f6592b3e090772a7b55bef6729d8739922

folly/futures/detail/Core.h

@@ -335,12 +335,14 @@ class Core {
           x->add([this]() mutable {
             SCOPE_EXIT { detachOne(); };
             RequestContext::setContext(context_);
+            SCOPE_EXIT { callback_ = {}; };
             callback_(std::move(*result_));
           });
         } else {
           x->addWithPriority([this]() mutable {
             SCOPE_EXIT { detachOne(); };
             RequestContext::setContext(context_);
+            SCOPE_EXIT { callback_ = {}; };
             callback_(std::move(*result_));
           }, priority);
         }
@@ -348,10 +350,12 @@ class Core {
         --attached_; // Account for extra ++attached_ before try
         RequestContext::setContext(context_);
         result_ = Try<T>(exception_wrapper(std::current_exception()));
+        SCOPE_EXIT { callback_ = {}; };
         callback_(std::move(*result_));
       }
     } else {
       RequestContext::setContext(context_);
+      SCOPE_EXIT { callback_ = {}; };
       callback_(std::move(*result_));
     }
   }

folly/futures/test/FutureTest.cpp

@@ -558,21 +558,51 @@ TEST(Future, makeFuture) {
 
 TEST(Future, finish) {
   auto x = std::make_shared<int>(0);
-  {
-    Promise<int> p;
-    auto f = p.getFuture().then([x](Try<int>&& t) { *x = t.value(); });
 
-    // The callback hasn't executed
-    EXPECT_EQ(0, *x);
+  Promise<int> p;
+  auto f = p.getFuture().then([x](Try<int>&& t) { *x = t.value(); });
 
-    // The callback has a reference to x
-    EXPECT_EQ(2, x.use_count());
+  // The callback hasn't executed
+  EXPECT_EQ(0, *x);
 
-    p.setValue(42);
+  // The callback has a reference to x
+  EXPECT_EQ(2, x.use_count());
+
+  p.setValue(42);
+
+  // the callback has executed
+  EXPECT_EQ(42, *x);
+
+  // the callback has been destructed
+  // and has released its reference to x
+  EXPECT_EQ(1, x.use_count());
+}
+
+TEST(Future, finishBigLambda) {
+  auto x = std::make_shared<int>(0);
+
+  // bulk_data, to be captured in the lambda passed to Future::then.
+  // This is meant to force that the lambda can't be stored inside
+  // the Future object.
+  std::array<char, sizeof(detail::Core<int>)> bulk_data = {0};
+
+  // suppress gcc warning about bulk_data not being used
+  EXPECT_EQ(bulk_data[0], 0);
+
+  Promise<int> p;
+  auto f = p.getFuture().then([x, bulk_data](Try<int>&& t) { *x = t.value(); });
+
+  // The callback hasn't executed
+  EXPECT_EQ(0, *x);
+
+  // The callback has a reference to x
+  EXPECT_EQ(2, x.use_count());
+
+  p.setValue(42);
+
+  // the callback has executed
+  EXPECT_EQ(42, *x);
 
-    // the callback has executed
-    EXPECT_EQ(42, *x);
-  }
   // the callback has been destructed
   // and has released its reference to x
   EXPECT_EQ(1, x.use_count());