Release resources of whenAllSucceed/whenAllComplete once input future…

…s are complete. RELNOTES=Fixes potential memory leak in Futures.whenAllSucceed/whenAllComplete ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=265489523
google · Aug 26, 2019 · 494834b · 494834b
1 parent 11a2cf1
commit 494834b
Show file tree

Hide file tree

Showing 6 changed files with 72 additions and 0 deletions.
diff --git a/android/guava-tests/test/com/google/common/util/concurrent/FuturesTest.java b/android/guava-tests/test/com/google/common/util/concurrent/FuturesTest.java
@@ -2801,6 +2801,35 @@ public ListenableFuture<String> call() throws Exception {
     }
   }
 
+  @AndroidIncompatible
+  @GwtIncompatible
+  public void testWhenAllSucceed_releasesMemory() throws Exception {
+    SettableFuture<Long> future1 = SettableFuture.create();
+    SettableFuture<Long> future2 = SettableFuture.create();
+    WeakReference<SettableFuture<Long>> future1Ref = new WeakReference<>(future1);
+    WeakReference<SettableFuture<Long>> future2Ref = new WeakReference<>(future2);
+
+    AsyncCallable<Long> combiner =
+        new AsyncCallable<Long>() {
+          @Override
+          public ListenableFuture<Long> call() throws Exception {
+            return SettableFuture.create();
+          }
+        };
+
+    ListenableFuture<Long> unused =
+        whenAllSucceed(future1, future2).callAsync(combiner, directExecutor());
+
+    future1.set(1L);
+    future1 = null;
+    future2.set(2L);
+    future2 = null;
+
+    // Futures should be collected even if combiner future never finishes.
+    GcFinalization.awaitClear(future1Ref);
+    GcFinalization.awaitClear(future2Ref);
+  }
+
   /*
    * TODO(cpovirk): maybe pass around TestFuture instances instead of
    * ListenableFuture instances

diff --git a/android/guava/src/com/google/common/util/concurrent/AggregateFuture.java b/android/guava/src/com/google/common/util/concurrent/AggregateFuture.java
@@ -49,6 +49,10 @@ abstract class AggregateFuture<InputT, OutputT> extends AbstractFuture.TrustedFu
   @Override
   protected final void afterDone() {
     super.afterDone();
+    releaseResources();
+  }
+
+  protected final void releaseResources() {
     RunningState localRunningState = runningState;
     if (localRunningState != null) {
       // Let go of the memory held by the running state

diff --git a/android/guava/src/com/google/common/util/concurrent/CombinedFuture.java b/android/guava/src/com/google/common/util/concurrent/CombinedFuture.java
@@ -157,6 +157,9 @@ ListenableFuture<V> runInterruptibly() throws Exception {
     @Override
     void setValue(ListenableFuture<V> value) {
       setFuture(value);
+      // Eagerly release resources instead of waiting for afterDone. We are done with the inputs,
+      // but the actual future may not complete for arbitrarily long.
+      releaseResources();
     }
 
     @Override

diff --git a/guava-tests/test/com/google/common/util/concurrent/FuturesTest.java b/guava-tests/test/com/google/common/util/concurrent/FuturesTest.java
@@ -2801,6 +2801,35 @@ public ListenableFuture<String> call() throws Exception {
     }
   }
 
+  @AndroidIncompatible
+  @GwtIncompatible
+  public void testWhenAllSucceed_releasesMemory() throws Exception {
+    SettableFuture<Long> future1 = SettableFuture.create();
+    SettableFuture<Long> future2 = SettableFuture.create();
+    WeakReference<SettableFuture<Long>> future1Ref = new WeakReference<>(future1);
+    WeakReference<SettableFuture<Long>> future2Ref = new WeakReference<>(future2);
+
+    AsyncCallable<Long> combiner =
+        new AsyncCallable<Long>() {
+          @Override
+          public ListenableFuture<Long> call() throws Exception {
+            return SettableFuture.create();
+          }
+        };
+
+    ListenableFuture<Long> unused =
+        whenAllSucceed(future1, future2).callAsync(combiner, directExecutor());
+
+    future1.set(1L);
+    future1 = null;
+    future2.set(2L);
+    future2 = null;
+
+    // Futures should be collected even if combiner future never finishes.
+    GcFinalization.awaitClear(future1Ref);
+    GcFinalization.awaitClear(future2Ref);
+  }
+
   /*
    * TODO(cpovirk): maybe pass around TestFuture instances instead of
    * ListenableFuture instances

diff --git a/guava/src/com/google/common/util/concurrent/AggregateFuture.java b/guava/src/com/google/common/util/concurrent/AggregateFuture.java
@@ -49,6 +49,10 @@ abstract class AggregateFuture<InputT, OutputT> extends AbstractFuture.TrustedFu
   @Override
   protected final void afterDone() {
     super.afterDone();
+    releaseResources();
+  }
+
+  protected final void releaseResources() {
     RunningState localRunningState = runningState;
     if (localRunningState != null) {
       // Let go of the memory held by the running state

diff --git a/guava/src/com/google/common/util/concurrent/CombinedFuture.java b/guava/src/com/google/common/util/concurrent/CombinedFuture.java
@@ -157,6 +157,9 @@ ListenableFuture<V> runInterruptibly() throws Exception {
     @Override
     void setValue(ListenableFuture<V> value) {
       setFuture(value);
+      // Eagerly release resources instead of waiting for afterDone. We are done with the inputs,
+      // but the actual future may not complete for arbitrarily long.
+      releaseResources();
     }
 
     @Override