From 3703b1b00456c1ebf84937e579dca392905ab098 Mon Sep 17 00:00:00 2001
From: Sven Over <over@fb.com>
Date: Fri, 5 May 2017 09:12:51 -0700
Subject: [PATCH] Future: improve test with task discarding executors

Summary:
We have tests that check that the Future implementation deals
cleanly with executors discarding tasks. The existing tests destroy
the tasks immediately when they are passed to Executor::add. This
diff adds corresponding tests for the scenario where the task is
not destroyed right away, but after the call to Future::then has
completed.

This diff also adds a mechanism to detect that the passed callback
function is actually destroyed. We have tested already that the
promise returned by folly::then will be set to a BrokenPromise
exception when the executor discards the task. However, the task
passed to the executor is not only the callback we pass to
folly::then, as the Future implementation wraps it with some code
that stores the return value in a Promise. Existing tests check
that this Promise is destroyed. The added mechanism in this diff
checks that the orignal callback function itself gets destroyed.

Reviewed By: Krigpl

Differential Revision: D5002100

fbshipit-source-id: 4155f61b075d9fe8d1869ad229f4d350571ff4c6
---
 folly/futures/test/ViaTest.cpp | 72 ++++++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 4 deletions(-)

diff --git a/folly/futures/test/ViaTest.cpp b/folly/futures/test/ViaTest.cpp
index bbbd96a2..2bdabfc0 100644
--- a/folly/futures/test/ViaTest.cpp
+++ b/folly/futures/test/ViaTest.cpp
@@ -473,19 +473,83 @@ TEST(Via, viaRaces) {
 }
 
 TEST(Via, viaDummyExecutorFutureSetValueFirst) {
+  // The callback object will get destroyed when passed to the executor.
+
+  // A promise will be captured by the callback lambda so we can observe that
+  // it will be destroyed.
+  Promise<Unit> captured_promise;
+  auto captured_promise_future = captured_promise.getFuture();
+
   DummyDrivableExecutor x;
-  auto future = makeFuture().via(&x).then([] { return 42; });
+  auto future = makeFuture().via(&x).then(
+      [c = std::move(captured_promise)] { return 42; });
 
-  EXPECT_THROW(future.get(), BrokenPromise);
+  EXPECT_THROW(future.get(std::chrono::seconds(5)), BrokenPromise);
+  EXPECT_THROW(
+      captured_promise_future.get(std::chrono::seconds(5)), BrokenPromise);
 }
 
 TEST(Via, viaDummyExecutorFutureSetCallbackFirst) {
+  // The callback object will get destroyed when passed to the executor.
+
+  // A promise will be captured by the callback lambda so we can observe that
+  // it will be destroyed.
+  Promise<Unit> captured_promise;
+  auto captured_promise_future = captured_promise.getFuture();
+
   DummyDrivableExecutor x;
   Promise<Unit> trigger;
-  auto future = trigger.getFuture().via(&x).then([] { return 42; });
+  auto future = trigger.getFuture().via(&x).then(
+      [c = std::move(captured_promise)] { return 42; });
   trigger.setValue();
 
-  EXPECT_THROW(future.get(), BrokenPromise);
+  EXPECT_THROW(future.get(std::chrono::seconds(5)), BrokenPromise);
+  EXPECT_THROW(
+      captured_promise_future.get(std::chrono::seconds(5)), BrokenPromise);
+}
+
+TEST(Via, viaExecutorDiscardsTaskFutureSetValueFirst) {
+  // The callback object will get destroyed when the ManualExecutor runs out
+  // of scope.
+
+  // A promise will be captured by the callback lambda so we can observe that
+  // it will be destroyed.
+  Promise<Unit> captured_promise;
+  auto captured_promise_future = captured_promise.getFuture();
+
+  Optional<Future<int>> future;
+  {
+    ManualExecutor x;
+    future = makeFuture().via(&x).then(
+        [c = std::move(captured_promise)] { return 42; });
+  }
+
+  EXPECT_THROW(future->get(std::chrono::seconds(5)), BrokenPromise);
+  EXPECT_THROW(
+      captured_promise_future.get(std::chrono::seconds(5)), BrokenPromise);
+}
+
+TEST(Via, viaExecutorDiscardsTaskFutureSetCallbackFirst) {
+  // The callback object will get destroyed when the ManualExecutor runs out
+  // of scope.
+
+  // A promise will be captured by the callback lambda so we can observe that
+  // it will be destroyed.
+  Promise<Unit> captured_promise;
+  auto captured_promise_future = captured_promise.getFuture();
+
+  Optional<Future<int>> future;
+  {
+    ManualExecutor x;
+    Promise<Unit> trigger;
+    future = trigger.getFuture().via(&x).then(
+        [c = std::move(captured_promise)] { return 42; });
+    trigger.setValue();
+  }
+
+  EXPECT_THROW(future->get(std::chrono::seconds(5)), BrokenPromise);
+  EXPECT_THROW(
+      captured_promise_future.get(std::chrono::seconds(5)), BrokenPromise);
 }
 
 TEST(ViaFunc, liftsVoid) {
-- 
2.34.1