Skip to content

Commit 9938818

Browse files
cpovirkGoogle Java Core Libraries
authored andcommitted
Use @RetainedLocalRef to keep J2ObjC from deallocating objects when the fields they were stored in are nulled out.
I put together this CL by looking for `@LazyInit` annotations, but I skipped `ExecutionSequencer` and `SequentialExecutor`, honestly mostly because they are complicated and not directly implicated in the problems we've heard about. I don't know if they're used much under J2ObjC. RELNOTES=n/a PiperOrigin-RevId: 689762804
1 parent f9f3fff commit 9938818

File tree

14 files changed

+94
-70
lines changed

14 files changed

+94
-70
lines changed

android/guava/src/com/google/common/util/concurrent/AbstractCatchingFuture.java

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import com.google.common.util.concurrent.internal.InternalFutures;
2828
import com.google.errorprone.annotations.ForOverride;
2929
import com.google.errorprone.annotations.concurrent.LazyInit;
30+
import com.google.j2objc.annotations.RetainedLocalRef;
3031
import java.util.concurrent.ExecutionException;
3132
import java.util.concurrent.Executor;
3233
import javax.annotation.CheckForNull;
@@ -80,9 +81,9 @@ abstract class AbstractCatchingFuture<
8081

8182
@Override
8283
public final void run() {
83-
ListenableFuture<? extends V> localInputFuture = inputFuture;
84-
Class<X> localExceptionType = exceptionType;
85-
F localFallback = fallback;
84+
@RetainedLocalRef ListenableFuture<? extends V> localInputFuture = inputFuture;
85+
@RetainedLocalRef Class<X> localExceptionType = exceptionType;
86+
@RetainedLocalRef F localFallback = fallback;
8687
if (localInputFuture == null | localExceptionType == null | localFallback == null
8788
// This check, unlike all the others, is a volatile read
8889
|| isCancelled()) {
@@ -152,9 +153,9 @@ public final void run() {
152153
@Override
153154
@CheckForNull
154155
protected String pendingToString() {
155-
ListenableFuture<? extends V> localInputFuture = inputFuture;
156-
Class<X> localExceptionType = exceptionType;
157-
F localFallback = fallback;
156+
@RetainedLocalRef ListenableFuture<? extends V> localInputFuture = inputFuture;
157+
@RetainedLocalRef Class<X> localExceptionType = exceptionType;
158+
@RetainedLocalRef F localFallback = fallback;
158159
String superString = super.pendingToString();
159160
String resultString = "";
160161
if (localInputFuture != null) {
@@ -184,7 +185,8 @@ protected String pendingToString() {
184185

185186
@Override
186187
protected final void afterDone() {
187-
maybePropagateCancellationTo(inputFuture);
188+
@RetainedLocalRef ListenableFuture<? extends V> localInputFuture = inputFuture;
189+
maybePropagateCancellationTo(localInputFuture);
188190
this.inputFuture = null;
189191
this.exceptionType = null;
190192
this.fallback = null;

android/guava/src/com/google/common/util/concurrent/AbstractTransformFuture.java

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import com.google.common.base.Function;
2424
import com.google.errorprone.annotations.ForOverride;
2525
import com.google.errorprone.annotations.concurrent.LazyInit;
26+
import com.google.j2objc.annotations.RetainedLocalRef;
2627
import java.util.concurrent.CancellationException;
2728
import java.util.concurrent.ExecutionException;
2829
import java.util.concurrent.Executor;
@@ -73,8 +74,8 @@ abstract class AbstractTransformFuture<
7374
@Override
7475
@SuppressWarnings("CatchingUnchecked") // sneaky checked exception
7576
public final void run() {
76-
ListenableFuture<? extends I> localInputFuture = inputFuture;
77-
F localFunction = function;
77+
@RetainedLocalRef ListenableFuture<? extends I> localInputFuture = inputFuture;
78+
@RetainedLocalRef F localFunction = function;
7879
if (isCancelled() | localInputFuture == null | localFunction == null) {
7980
return;
8081
}
@@ -186,16 +187,17 @@ public final void run() {
186187

187188
@Override
188189
protected final void afterDone() {
189-
maybePropagateCancellationTo(inputFuture);
190+
@RetainedLocalRef ListenableFuture<? extends I> localInputFuture = inputFuture;
191+
maybePropagateCancellationTo(localInputFuture);
190192
this.inputFuture = null;
191193
this.function = null;
192194
}
193195

194196
@Override
195197
@CheckForNull
196198
protected String pendingToString() {
197-
ListenableFuture<? extends I> localInputFuture = inputFuture;
198-
F localFunction = function;
199+
@RetainedLocalRef ListenableFuture<? extends I> localInputFuture = inputFuture;
200+
@RetainedLocalRef F localFunction = function;
199201
String superString = super.pendingToString();
200202
String resultString = "";
201203
if (localInputFuture != null) {

android/guava/src/com/google/common/util/concurrent/AggregateFuture.java

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import com.google.errorprone.annotations.ForOverride;
2929
import com.google.errorprone.annotations.OverridingMethodsMustInvokeSuper;
3030
import com.google.errorprone.annotations.concurrent.LazyInit;
31+
import com.google.j2objc.annotations.RetainedLocalRef;
3132
import java.util.Set;
3233
import java.util.concurrent.ExecutionException;
3334
import java.util.concurrent.Future;
@@ -80,7 +81,7 @@ abstract class AggregateFuture<InputT extends @Nullable Object, OutputT extends
8081
protected final void afterDone() {
8182
super.afterDone();
8283

83-
ImmutableCollection<? extends Future<?>> localFutures = futures;
84+
@RetainedLocalRef ImmutableCollection<? extends Future<?>> localFutures = futures;
8485
releaseResources(OUTPUT_FUTURE_DONE); // nulls out `futures`
8586

8687
if (isCancelled() & localFutures != null) {
@@ -98,7 +99,7 @@ protected final void afterDone() {
9899
@Override
99100
@CheckForNull
100101
protected final String pendingToString() {
101-
ImmutableCollection<? extends Future<?>> localFutures = futures;
102+
@RetainedLocalRef ImmutableCollection<? extends Future<?>> localFutures = futures;
102103
if (localFutures != null) {
103104
return "futures=" + localFutures;
104105
}
@@ -156,22 +157,24 @@ final void init() {
156157
* Future.get() when we don't need to (specifically, for whenAllComplete().call*()), and it
157158
* lets all futures share the same listener.
158159
*
159-
* We store `localFutures` inside the listener because `this.futures` might be nulled out by
160-
* the time the listener runs for the final future -- at which point we need to check all
161-
* inputs for exceptions *if* we're collecting values. If we're not, then the listener doesn't
162-
* need access to the futures again, so we can just pass `null`.
160+
* We store `localFuturesOrNull` inside the listener because `this.futures` might be nulled
161+
* out by the time the listener runs for the final future -- at which point we need to check
162+
* all inputs for exceptions *if* we're collecting values. If we're not, then the listener
163+
* doesn't need access to the futures again, so we can just pass `null`.
163164
*
164165
* TODO(b/112550045): Allocating a single, cheaper listener is (I think) only an optimization.
165166
* If we make some other optimizations, this one will no longer be necessary. The optimization
166167
* could actually hurt in some cases, as it forces us to keep all inputs in memory until the
167168
* final input completes.
168169
*/
169-
ImmutableCollection<? extends Future<? extends InputT>> localFutures =
170-
collectsValues ? futures : null;
171-
Runnable listener = () -> decrementCountAndMaybeComplete(localFutures);
172-
for (ListenableFuture<? extends InputT> future : futures) {
170+
@RetainedLocalRef
171+
ImmutableCollection<? extends ListenableFuture<? extends InputT>> localFutures = futures;
172+
ImmutableCollection<? extends Future<? extends InputT>> localFuturesOrNull =
173+
collectsValues ? localFutures : null;
174+
Runnable listener = () -> decrementCountAndMaybeComplete(localFuturesOrNull);
175+
for (ListenableFuture<? extends InputT> future : localFutures) {
173176
if (future.isDone()) {
174-
decrementCountAndMaybeComplete(localFutures);
177+
decrementCountAndMaybeComplete(localFuturesOrNull);
175178
} else {
176179
future.addListener(listener, directExecutor());
177180
}

android/guava/src/com/google/common/util/concurrent/CollectionFuture.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import com.google.common.collect.ImmutableCollection;
2222
import com.google.common.collect.Lists;
2323
import com.google.errorprone.annotations.concurrent.LazyInit;
24+
import com.google.j2objc.annotations.RetainedLocalRef;
2425
import java.util.Collections;
2526
import java.util.List;
2627
import javax.annotation.CheckForNull;
@@ -59,15 +60,15 @@ abstract class CollectionFuture<V extends @Nullable Object, C extends @Nullable
5960

6061
@Override
6162
final void collectOneValue(int index, @ParametricNullness V returnValue) {
62-
List<@Nullable Present<V>> localValues = values;
63+
@RetainedLocalRef List<@Nullable Present<V>> localValues = values;
6364
if (localValues != null) {
6465
localValues.set(index, new Present<>(returnValue));
6566
}
6667
}
6768

6869
@Override
6970
final void handleAllCompleted() {
70-
List<@Nullable Present<V>> localValues = values;
71+
@RetainedLocalRef List<@Nullable Present<V>> localValues = values;
7172
if (localValues != null) {
7273
set(combine(localValues));
7374
}

android/guava/src/com/google/common/util/concurrent/CombinedFuture.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import com.google.common.annotations.GwtCompatible;
2121
import com.google.common.collect.ImmutableCollection;
2222
import com.google.errorprone.annotations.concurrent.LazyInit;
23+
import com.google.j2objc.annotations.RetainedLocalRef;
2324
import com.google.j2objc.annotations.WeakOuter;
2425
import java.util.concurrent.Callable;
2526
import java.util.concurrent.CancellationException;
@@ -61,7 +62,7 @@ void collectOneValue(int index, @CheckForNull Object returnValue) {}
6162

6263
@Override
6364
void handleAllCompleted() {
64-
CombinedFutureInterruptibleTask<?> localTask = task;
65+
@RetainedLocalRef CombinedFutureInterruptibleTask<?> localTask = task;
6566
if (localTask != null) {
6667
localTask.execute();
6768
}
@@ -84,7 +85,7 @@ void releaseResources(ReleaseResourcesReason reason) {
8485

8586
@Override
8687
protected void interruptTask() {
87-
CombinedFutureInterruptibleTask<?> localTask = task;
88+
@RetainedLocalRef CombinedFutureInterruptibleTask<?> localTask = task;
8889
if (localTask != null) {
8990
localTask.interruptTask();
9091
}

android/guava/src/com/google/common/util/concurrent/Futures.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import com.google.common.util.concurrent.internal.InternalFutures;
3636
import com.google.errorprone.annotations.CanIgnoreReturnValue;
3737
import com.google.errorprone.annotations.concurrent.LazyInit;
38+
import com.google.j2objc.annotations.RetainedLocalRef;
3839
import java.time.Duration;
3940
import java.util.Collection;
4041
import java.util.List;
@@ -811,7 +812,7 @@ private static final class NonCancellationPropagatingFuture<V extends @Nullable
811812
public void run() {
812813
// This prevents cancellation from propagating because we don't call setFuture(delegate) until
813814
// delegate is already done, so calling cancel() on this future won't affect it.
814-
ListenableFuture<V> localDelegate = delegate;
815+
@RetainedLocalRef ListenableFuture<V> localDelegate = delegate;
815816
if (localDelegate != null) {
816817
setFuture(localDelegate);
817818
}
@@ -820,7 +821,7 @@ public void run() {
820821
@Override
821822
@CheckForNull
822823
protected String pendingToString() {
823-
ListenableFuture<V> localDelegate = delegate;
824+
@RetainedLocalRef ListenableFuture<V> localDelegate = delegate;
824825
if (localDelegate != null) {
825826
return "delegate=[" + localDelegate + "]";
826827
}

android/guava/src/com/google/common/util/concurrent/TimeoutFuture.java

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import com.google.common.annotations.J2ktIncompatible;
2222
import com.google.common.base.Preconditions;
2323
import com.google.errorprone.annotations.concurrent.LazyInit;
24+
import com.google.j2objc.annotations.RetainedLocalRef;
2425
import java.util.concurrent.ExecutionException;
2526
import java.util.concurrent.Future;
2627
import java.util.concurrent.ScheduledExecutorService;
@@ -102,7 +103,7 @@ public void run() {
102103
if (timeoutFuture == null) {
103104
return;
104105
}
105-
ListenableFuture<V> delegate = timeoutFuture.delegateRef;
106+
@RetainedLocalRef ListenableFuture<V> delegate = timeoutFuture.delegateRef;
106107
if (delegate == null) {
107108
return;
108109
}
@@ -124,7 +125,7 @@ public void run() {
124125
timeoutFuture.setFuture(delegate);
125126
} else {
126127
try {
127-
ScheduledFuture<?> timer = timeoutFuture.timer;
128+
@RetainedLocalRef ScheduledFuture<?> timer = timeoutFuture.timer;
128129
timeoutFuture.timer = null; // Don't include already elapsed delay in delegate.toString()
129130
String message = "Timed out";
130131
// This try-finally block ensures that we complete the timeout future, even if attempting
@@ -162,8 +163,8 @@ public synchronized Throwable fillInStackTrace() {
162163
@Override
163164
@CheckForNull
164165
protected String pendingToString() {
165-
ListenableFuture<? extends V> localInputFuture = delegateRef;
166-
ScheduledFuture<?> localTimer = timer;
166+
@RetainedLocalRef ListenableFuture<? extends V> localInputFuture = delegateRef;
167+
@RetainedLocalRef ScheduledFuture<?> localTimer = timer;
167168
if (localInputFuture != null) {
168169
String message = "inputFuture=[" + localInputFuture + "]";
169170
if (localTimer != null) {
@@ -180,9 +181,10 @@ protected String pendingToString() {
180181

181182
@Override
182183
protected void afterDone() {
183-
maybePropagateCancellationTo(delegateRef);
184+
@RetainedLocalRef ListenableFuture<? extends V> delegate = delegateRef;
185+
maybePropagateCancellationTo(delegate);
184186

185-
Future<?> localTimer = timer;
187+
@RetainedLocalRef Future<?> localTimer = timer;
186188
// Try to cancel the timer as an optimization.
187189
// timer may be null if this call to run was by the timer task since there is no happens-before
188190
// edge between the assignment to timer and an execution of the timer task.

guava/src/com/google/common/util/concurrent/AbstractCatchingFuture.java

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import com.google.common.util.concurrent.internal.InternalFutures;
2828
import com.google.errorprone.annotations.ForOverride;
2929
import com.google.errorprone.annotations.concurrent.LazyInit;
30+
import com.google.j2objc.annotations.RetainedLocalRef;
3031
import java.util.concurrent.ExecutionException;
3132
import java.util.concurrent.Executor;
3233
import javax.annotation.CheckForNull;
@@ -80,9 +81,9 @@ abstract class AbstractCatchingFuture<
8081

8182
@Override
8283
public final void run() {
83-
ListenableFuture<? extends V> localInputFuture = inputFuture;
84-
Class<X> localExceptionType = exceptionType;
85-
F localFallback = fallback;
84+
@RetainedLocalRef ListenableFuture<? extends V> localInputFuture = inputFuture;
85+
@RetainedLocalRef Class<X> localExceptionType = exceptionType;
86+
@RetainedLocalRef F localFallback = fallback;
8687
if (localInputFuture == null | localExceptionType == null | localFallback == null
8788
// This check, unlike all the others, is a volatile read
8889
|| isCancelled()) {
@@ -152,9 +153,9 @@ public final void run() {
152153
@Override
153154
@CheckForNull
154155
protected String pendingToString() {
155-
ListenableFuture<? extends V> localInputFuture = inputFuture;
156-
Class<X> localExceptionType = exceptionType;
157-
F localFallback = fallback;
156+
@RetainedLocalRef ListenableFuture<? extends V> localInputFuture = inputFuture;
157+
@RetainedLocalRef Class<X> localExceptionType = exceptionType;
158+
@RetainedLocalRef F localFallback = fallback;
158159
String superString = super.pendingToString();
159160
String resultString = "";
160161
if (localInputFuture != null) {
@@ -184,7 +185,8 @@ protected String pendingToString() {
184185

185186
@Override
186187
protected final void afterDone() {
187-
maybePropagateCancellationTo(inputFuture);
188+
@RetainedLocalRef ListenableFuture<? extends V> localInputFuture = inputFuture;
189+
maybePropagateCancellationTo(localInputFuture);
188190
this.inputFuture = null;
189191
this.exceptionType = null;
190192
this.fallback = null;

guava/src/com/google/common/util/concurrent/AbstractTransformFuture.java

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import com.google.common.base.Function;
2424
import com.google.errorprone.annotations.ForOverride;
2525
import com.google.errorprone.annotations.concurrent.LazyInit;
26+
import com.google.j2objc.annotations.RetainedLocalRef;
2627
import java.util.concurrent.CancellationException;
2728
import java.util.concurrent.ExecutionException;
2829
import java.util.concurrent.Executor;
@@ -73,8 +74,8 @@ abstract class AbstractTransformFuture<
7374
@Override
7475
@SuppressWarnings("CatchingUnchecked") // sneaky checked exception
7576
public final void run() {
76-
ListenableFuture<? extends I> localInputFuture = inputFuture;
77-
F localFunction = function;
77+
@RetainedLocalRef ListenableFuture<? extends I> localInputFuture = inputFuture;
78+
@RetainedLocalRef F localFunction = function;
7879
if (isCancelled() | localInputFuture == null | localFunction == null) {
7980
return;
8081
}
@@ -186,16 +187,17 @@ public final void run() {
186187

187188
@Override
188189
protected final void afterDone() {
189-
maybePropagateCancellationTo(inputFuture);
190+
@RetainedLocalRef ListenableFuture<? extends I> localInputFuture = inputFuture;
191+
maybePropagateCancellationTo(localInputFuture);
190192
this.inputFuture = null;
191193
this.function = null;
192194
}
193195

194196
@Override
195197
@CheckForNull
196198
protected String pendingToString() {
197-
ListenableFuture<? extends I> localInputFuture = inputFuture;
198-
F localFunction = function;
199+
@RetainedLocalRef ListenableFuture<? extends I> localInputFuture = inputFuture;
200+
@RetainedLocalRef F localFunction = function;
199201
String superString = super.pendingToString();
200202
String resultString = "";
201203
if (localInputFuture != null) {

0 commit comments

Comments
 (0)