mesos git commit: Fixed memory leak in implementation of Future::after().

[jo...@apache.org]

Repository: mesos
Updated Branches:
  refs/heads/master b2d36ed69 -> be127e6ec


Fixed memory leak in implementation of Future<T>::after().

Removes a reference counted pointer that futures kept to themselves
when using the method `Future<T>::after()`.

Review: https://reviews.apache.org/r/53299/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/be127e6e
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/be127e6e
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/be127e6e

Branch: refs/heads/master
Commit: be127e6eca1312bf8c2b039646f6909fa42cd342
Parents: b2d36ed
Author: Alexander Rojas <al...@mesosphere.io>
Authored: Thu Jan 5 10:43:02 2017 -0500
Committer: Joris Van Remoortere <jo...@gmail.com>
Committed: Thu Jan 5 11:32:23 2017 -0500

----------------------------------------------------------------------
 3rdparty/libprocess/include/process/future.hpp | 23 ++++++++++++------
 3rdparty/libprocess/src/tests/future_tests.cpp | 26 +++++++++++++++++++++
 2 files changed, 42 insertions(+), 7 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/be127e6e/3rdparty/libprocess/include/process/future.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/include/process/future.hpp b/3rdparty/libprocess/include/process/future.hpp
index 26bf585..71171c6 100644
--- a/3rdparty/libprocess/include/process/future.hpp
+++ b/3rdparty/libprocess/include/process/future.hpp
@@ -1334,7 +1334,7 @@ void expired(
     const lambda::function<Future<T>(const Future<T>&)>& f,
     const std::shared_ptr<Latch>& latch,
     const std::shared_ptr<Promise<T>>& promise,
-    const Future<T>& future)
+    const WeakFuture<T>& weak_future)
 {
   if (latch->trigger()) {
     // Note that we don't bother checking if 'future' has been
@@ -1344,7 +1344,14 @@ void expired(
     // if the future has been discarded and rather than hiding a
     // non-deterministic bug we always call 'f' if the timer has
     // expired.
-    promise->associate(f(future));
+    Option<Future<T>> future = weak_future.get();
+    if (future.isSome()) {
+      promise->associate(f(future.get()));
+    } else {
+      // The original future may be out of scope, but the caller of
+      // after may still hold a reference to the promise's future.
+      promise->associate(f(promise->future()));
+    }
   }
 }
 
@@ -1435,19 +1442,21 @@ Future<T> Future<T>::after(
   std::shared_ptr<Promise<T>> promise(new Promise<T>());
 
   // Set up a timer to invoke the callback if this future has not
-  // completed. Note that we do not pass a weak reference for this
-  // future as we don't want the future to get cleaned up and then
-  // have the timer expire.
+  // completed. A weak reference is needed to avoid memory leaks
+  // when storing the timer inside the future onAny callbacks vector.
+  // If the future has been cleaned, we use the `promise` future
+  // instead.
+  WeakFuture<T> weak_future(*this);
   Timer timer = Clock::timer(
       duration,
-      lambda::bind(&internal::expired<T>, f, latch, promise, *this));
+      lambda::bind(&internal::expired<T>, f, latch, promise, weak_future));
 
   onAny(lambda::bind(&internal::after<T>, latch, promise, timer, lambda::_1));
 
   // Propagate discarding up the chain. To avoid cyclic dependencies,
   // we keep a weak future in the callback.
   promise->future().onDiscard(
-      lambda::bind(&internal::discard<T>, WeakFuture<T>(*this)));
+      lambda::bind(&internal::discard<T>, weak_future));
 
   return promise->future();
 }

http://git-wip-us.apache.org/repos/asf/mesos/blob/be127e6e/3rdparty/libprocess/src/tests/future_tests.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/src/tests/future_tests.cpp b/3rdparty/libprocess/src/tests/future_tests.cpp
index 7c411c7..fbf6995 100644
--- a/3rdparty/libprocess/src/tests/future_tests.cpp
+++ b/3rdparty/libprocess/src/tests/future_tests.cpp
@@ -18,6 +18,8 @@
 #include <process/future.hpp>
 #include <process/gtest.hpp>
 
+#include <stout/duration.hpp>
+#include <stout/nothing.hpp>
 #include <stout/try.hpp>
 
 using process::Clock;
@@ -256,6 +258,30 @@ TEST(FutureTest, After2)
 }
 
 
+// Verifies that a a future does not leak memory after calling
+// `after()`. This behavior ocurred because a future indirectly
+// kept a reference counted pointer to itself.
+TEST(FutureTest, After3)
+{
+  Future<Nothing> future;
+  process::WeakFuture<Nothing> weak_future(future);
+
+  EXPECT_SOME(weak_future.get());
+
+  {
+    // The original future disappears here. After this call the
+    // original future goes out of scope and should not be reachable
+    // anymore.
+    future = future.after(
+        Milliseconds(1), [](const Future<Nothing>&) { return Nothing(); });
+
+    AWAIT_READY(future);
+  }
+
+  EXPECT_NONE(weak_future.get());
+}
+
+
 static Future<bool> readyFuture()
 {
   return true;