folly/futures: keep Core alive until callback has returned

Summary:Callbacks passed to e.g. folly::Future::then (or
folly::Future::ensure) may delete the promise that keeps the
Core of the same future alive. The Core must protect itself from
destruction during callback execution.

This commit also adds a unit test to check for correct behaviour
in the "self destruction" scenario. The test should usually pass
even without the fix in Core.h. However, when you run the test
in Valgrind or ASAN, it will report problems unless the fix in
Core.h is applied.

Reviewed By: mhx

Differential Revision: D2938094

fb-gh-sync-id: 22796e168e1876ef2e3c7d7619da020be6a22073
shipit-source-id: 22796e168e1876ef2e3c7d7619da020be6a22073

diff --git a/folly/futures/detail/Core.h b/folly/futures/detail/Core.h
index ecacb280591590ab8a4044d17174ff5541ef1cc5..74c9ef28795ebe9b1bb291b7f61701d48a759c6c 100644 (file)
--- a/folly/futures/detail/Core.h
+++ b/folly/futures/detail/Core.h
@@ -327,9 +327,10 @@ class Core {
       executorLock_.unlock();
     }
 
+    // keep Core alive until callback did its thing
+    ++attached_;
+
     if (x) {
-      // keep Core alive until executor did its thing
-      ++attached_;
       try {
         if (LIKELY(x->getNumPriorities() == 1)) {
           x->add([this]() mutable {
@@ -354,6 +355,7 @@ class Core {
         callback_(std::move(*result_));
       }
     } else {
+      SCOPE_EXIT { detachOne(); };
       RequestContext::setContext(context_);
       SCOPE_EXIT { callback_ = {}; };
       callback_(std::move(*result_));

diff --git a/folly/futures/test/SelfDestructTest.cpp b/folly/futures/test/SelfDestructTest.cpp
new file mode 100644 (file)
index 0000000..52842f4
--- /dev/null
+++ b/folly/futures/test/SelfDestructTest.cpp
@@ -0,0 +1,38 @@
+/*
+ * Copyright 2016 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <gtest/gtest.h>
+
+#include <folly/futures/Future.h>
+
+using namespace folly;
+
+TEST(SelfDestruct, then) {
+  auto* p = new Promise<int>();
+  auto future = p->getFuture().then([p](int x) {
+    delete p;
+    return x + 1;
+  });
+  p->setValue(123);
+  EXPECT_EQ(future.get(), 124);
+}
+
+TEST(SelfDestruct, ensure) {
+  auto* p = new Promise<int>();
+  auto future = p->getFuture().ensure([p] { delete p; });
+  p->setValue(123);
+  EXPECT_EQ(future.get(), 123);
+}

diff --git a/folly/test/Makefile.am b/folly/test/Makefile.am
index 02e5727816d2c9c86aad6793e7276c011d70f757..e5414567a1d306751b430d5ca311c0836dff2069 100644 (file)
--- a/folly/test/Makefile.am
+++ b/folly/test/Makefile.am
@@ -229,6 +229,7 @@ futures_test_SOURCES = \
     ../futures/test/PromiseTest.cpp \
     ../futures/test/ReduceTest.cpp \
     ../futures/test/RetryingTest.cpp \
+    ../futures/test/SelfDestructTest.cpp \
     ../futures/test/SharedPromiseTest.cpp \
     ../futures/test/ThenCompileTest.cpp \
     ../futures/test/ThenTest.cpp \