Skip to content

Commit

Permalink
Fix LottieTask leak (#2465)
Browse files Browse the repository at this point in the history
I investigated leaks with `RememberLottieComposition` and `LottieAnimationView`, initially thinking that we might need to stop listening to LottieTask when a view or composition gets detached / removed.

However, after looking at a heap dump, I realized that the LottieTask was actually finished. It had delivered its result so it didn't have any reason to stay in memory.

This moved my suspicion to SynchronousQueue as Android has had issues with queues & worker threads on several occasions (see code for details).

Also updated the cached thread pool to create threads named "lottie".

Leaks:

```
┬───
│ GC Root: Thread object
│
├─ java.lang.Thread instance
│    Leaking: UNKNOWN
│    Retaining 6.2 MB in 181913 objects
│    Thread name: 'pool-88-thread-2'
│    ↓ Thread<Java Local>
│            ~~~~~~~~~~~~
├─ java.util.concurrent.SynchronousQueue$TransferStack$SNode instance
│    Leaking: UNKNOWN
│    Retaining 6.2 MB in 181909 objects
│    ↓ SynchronousQueue$TransferStack$SNode.match
│                                           ~~~~~
├─ java.util.concurrent.SynchronousQueue$TransferStack$SNode instance
│    Leaking: UNKNOWN
│    Retaining 6.2 MB in 181908 objects
│    ↓ SynchronousQueue$TransferStack$SNode.item
│                                           ~~~~
├─ com.airbnb.lottie.LottieTask$LottieFutureTask instance
│    Leaking: UNKNOWN
│    Retaining 6.2 MB in 181907 objects
│    ↓ LottieTask$LottieFutureTask.this$0
│                                  ~~~~~~
├─ com.airbnb.lottie.LottieTask instance
│    Leaking: UNKNOWN
│    Retaining 6.2 MB in 181905 objects
│    ↓ LottieTask.failureListeners
│                 ~~~~~~~~~~~~~~~~
├─ java.util.LinkedHashSet instance
│    Leaking: UNKNOWN
│    Retaining 113 B in 5 objects
│    ↓ LinkedHashSet[element()]
│                   ~~~~~~~~~~~
├─ com.airbnb.lottie.compose.RememberLottieCompositionKt$await$2$2 instance
│    Leaking: UNKNOWN
│    Retaining 12 B in 1 objects
│    Anonymous class implementing com.airbnb.lottie.LottieListener
│    ↓ RememberLottieCompositionKt$await$2$2.$cont
│                                            ~~~~~
├─ kotlinx.coroutines.CancellableContinuationImpl instance
│    Leaking: UNKNOWN
│    Retaining 6.2 MB in 181892 objects
│    ↓ CancellableContinuationImpl.delegate
│                                  ~~~~~~~~
├─ androidx.compose.ui.test.FrameDeferringContinuationInterceptor$FrameDeferredContinuation instance
│    Leaking: UNKNOWN
│    Retaining 6.2 MB in 181888 objects
│    ↓ FrameDeferringContinuationInterceptor$FrameDeferredContinuation.continuation
│                                                                      ~~~~~~~~~~~~
├─ androidx.compose.ui.test.ApplyingContinuationInterceptor$SendApplyContinuation instance
│    Leaking: UNKNOWN
│    Retaining 6.2 MB in 181887 objects
│    ↓ ApplyingContinuationInterceptor$SendApplyContinuation.continuation
│                                                            ~~~~~~~~~~~~
├─ com.airbnb.lottie.compose.RememberLottieCompositionKt$lottieComposition$1 instance
│    Leaking: UNKNOWN
│    Retaining 6.2 MB in 181886 objects
│    Anonymous subclass of kotlin.coroutines.jvm.internal.ContinuationImpl
│    ↓ BaseContinuationImpl.completion
│                           ~~~~~~~~~~
├─ com.airbnb.lottie.compose.RememberLottieCompositionKt$rememberLottieComposition$3 instance
│    Leaking: UNKNOWN
│    Retaining 6.2 MB in 181885 objects
│    Anonymous subclass of kotlin.coroutines.jvm.internal.SuspendLambda
│    $context instance of com.squareup.ui.market.core.theme.MarketContextWrapper, wrapping activity com.squareup.ui.main.MainActivity with mDestroyed = true
│    ↓ RememberLottieCompositionKt$rememberLottieComposition$3.$context
│                                                              ~~~~~~~~
╰→ com.squareup.ui.market.core.theme.MarketContextWrapper instance
​     Leaking: YES (MarketContextWrapper wraps an Activity with Activity.mDestroyed true)
​     Retaining 6.2 MB in 181845 objects
​     mBase instance of com.squareup.ui.market.core.theme.MarketContextWrapper, wrapping activity com.squareup.ui.main.MainActivity with mDestroyed = true
```

```
┬───
│ GC Root: Thread object
│
├─ java.lang.Thread instance
│    Leaking: UNKNOWN
│    Retaining 8.1 MB in 257497 objects
│    Thread name: 'pool-38-thread-1'
│    ↓ Thread<Java Local>
│            ~~~~~~~~~~~~
├─ java.util.concurrent.SynchronousQueue$TransferStack$SNode instance
│    Leaking: UNKNOWN
│    Retaining 8.1 MB in 257493 objects
│    ↓ SynchronousQueue$TransferStack$SNode.match
│                                           ~~~~~
├─ java.util.concurrent.SynchronousQueue$TransferStack$SNode instance
│    Leaking: UNKNOWN
│    Retaining 8.1 MB in 257492 objects
│    ↓ SynchronousQueue$TransferStack$SNode.item
│                                           ~~~~
├─ com.airbnb.lottie.LottieTask$LottieFutureTask instance
│    Leaking: UNKNOWN
│    Retaining 8.1 MB in 257491 objects
│    ↓ LottieTask$LottieFutureTask.this$0
│                                  ~~~~~~
├─ com.airbnb.lottie.LottieTask instance
│    Leaking: UNKNOWN
│    Retaining 8.1 MB in 257489 objects
│    ↓ LottieTask.failureListeners
│                 ~~~~~~~~~~~~~~~~
├─ java.util.LinkedHashSet instance
│    Leaking: UNKNOWN
│    Retaining 101 B in 4 objects
│    ↓ LinkedHashSet[element()]
│                   ~~~~~~~~~~~
├─ com.airbnb.lottie.LottieAnimationView$1 instance
│    Leaking: UNKNOWN
│    Retaining 12 B in 1 objects
│    Anonymous class implementing com.airbnb.lottie.LottieListener
│    ↓ LottieAnimationView$1.this$0
│                            ~~~~~~
╰→ com.airbnb.lottie.LottieAnimationView instance
​     Leaking: YES (View.mContext references a destroyed activity)
​     Retaining 8.1 MB in 257476 objects
​     View not part of a window view hierarchy
​     View.mAttachInfo is null (view detached)
​     View.mWindowAttachCount = 1
​     mContext instance of flow.path.FlowPathContextWrapper, wrapping activity com.squareup.ui.main.MainActivity with mDestroyed = true
```
  • Loading branch information
pyricau committed Feb 18, 2024
1 parent 504bbdc commit fe412fa
Showing 1 changed file with 36 additions and 12 deletions.
48 changes: 36 additions & 12 deletions lottie/src/main/java/com/airbnb/lottie/LottieTask.java
Expand Up @@ -7,6 +7,7 @@
import androidx.annotation.RestrictTo;

import com.airbnb.lottie.utils.Logger;
import com.airbnb.lottie.utils.LottieThreadFactory;

import java.util.ArrayList;
import java.util.LinkedHashSet;
Expand Down Expand Up @@ -35,7 +36,7 @@ public class LottieTask<T> {
* You may change this to run deserialization synchronously for testing.
*/
@SuppressWarnings("WeakerAccess")
public static Executor EXECUTOR = Executors.newCachedThreadPool();
public static Executor EXECUTOR = Executors.newCachedThreadPool(new LottieThreadFactory());

/* Preserve add order. */
private final Set<LottieListener<T>> successListeners = new LinkedHashSet<>(1);
Expand Down Expand Up @@ -64,7 +65,7 @@ public LottieTask(T result) {
setResult(new LottieResult<>(e));
}
} else {
EXECUTOR.execute(new LottieFutureTask(runnable));
EXECUTOR.execute(new LottieFutureTask<T>(this, runnable));
}
}

Expand Down Expand Up @@ -173,22 +174,45 @@ private synchronized void notifyFailureListeners(Throwable e) {
}
}

private class LottieFutureTask extends FutureTask<LottieResult<T>> {
LottieFutureTask(Callable<LottieResult<T>> callable) {
private static class LottieFutureTask<T> extends FutureTask<LottieResult<T>> {

private LottieTask<T> lottieTask;

LottieFutureTask(LottieTask<T> task, Callable<LottieResult<T>> callable) {
super(callable);
lottieTask = task;
}

@Override
protected void done() {
if (isCancelled()) {
// We don't need to notify and listeners if the task is cancelled.
return;
}

try {
setResult(get());
} catch (InterruptedException | ExecutionException e) {
setResult(new LottieResult<>(e));
if (isCancelled()) {
// We don't need to notify and listeners if the task is cancelled.
return;
}

try {
lottieTask.setResult(get());
} catch (InterruptedException | ExecutionException e) {
lottieTask.setResult(new LottieResult<>(e));
}
} finally {
// LottieFutureTask can be held in memory for up to 60 seconds after the task is done, which would
// result in holding on to the associated LottieTask instance and leaking its listeners. To avoid
// that, we clear our the reference to the LottieTask instance.
//
// How is LottieFutureTask held for up to 60 seconds? It's a bug in how the VM cleans up stack
// local variables. When you have a loop that polls a blocking queue and assigns the result
// to a local variable, after looping the local variable will still reference the previous value
// until the queue returns the next result.
//
// Executors.newCachedThreadPool() relies on a SynchronousQueue and creates a cached thread pool
// with a default keep alice of 60 seconds. After a given worker thread runs a task, that thread
// will wait for up to 60 seconds for a new task to come, and while waiting it's also accidentally
// keeping a reference to the previous task.
//
// See commit d577e728e9bccbafc707af3060ea914caa73c14f in AOSP for how that was fixed for Looper.
lottieTask = null;
}
}
}
Expand Down

0 comments on commit fe412fa

Please sign in to comment.