Coil: Coil leaks Activity Context when an Activity with an ongoing request is destroyed

[Stonos]

Describe the bug
Coil causes a memory leak when finish() is called on an Activity that has an ongoing request.

To Reproduce
Download & run the attached project:
CoilMemoryLeak.zip

ActivityA tries to load an "image" from a URL that takes 60 seconds to respond. If you click the "Go to Activity B" button during that time, then LeakCanary should detect retained objects (check the notification). This does not happen if the request has finished before going to ActivityB.

Expected behavior
Any references to the Activity should always be cleared when it is destroyed.

Logs/Screenshots

2019-08-12 14:51:11.619 8859-9256/com.example.coilmemoryleak D/LeakCanary: HeapAnalysisSuccess(heapDumpFile=/data/user/0/com.example.coilmemoryleak/files/leakcanary/2019-08-12_14-50-53_530.hprof, createdAtTimeMillis=1565610671616, analysisDurationMillis=14783, applicationLeaks=[ApplicationLeak(className=com.example.coilmemoryleak.ActivityA, leakTrace=
    ┬
    ├─ android.os.HandlerThread
    │    Leaking: NO (PathClassLoader↓ is not leaking)
    │    Thread name: 'LeakCanary-Heap-Dump'
    │    GC Root: Thread object
    │    ↓ thread HandlerThread.contextClassLoader
    ├─ dalvik.system.PathClassLoader
    │    Leaking: NO (Object[]↓ is not leaking and A ClassLoader is never leaking)
    │    ↓ PathClassLoader.runtimeInternalObjects
    ├─ java.lang.Object[]
    │    Leaking: NO (Coil↓ is not leaking)
    │    ↓ array Object[].[870]
    ├─ coil.Coil
    │    Leaking: NO (a class is never leaking)
    │    ↓ static Coil.imageLoader
    │                  ~~~~~~~~~~~
    ├─ coil.RealImageLoader
    │    Leaking: UNKNOWN
    │    ↓ RealImageLoader.loaderScope
    │                      ~~~~~~~~~~~
    ├─ kotlinx.coroutines.internal.ContextScope
    │    Leaking: UNKNOWN
    │    ↓ ContextScope.coroutineContext
    │                   ~~~~~~~~~~~~~~~~
    ├─ kotlin.coroutines.CombinedContext
    │    Leaking: UNKNOWN
    │    ↓ CombinedContext.left
    │                      ~~~~
    ├─ kotlinx.coroutines.SupervisorJobImpl
    │    Leaking: UNKNOWN
    │    ↓ SupervisorJobImpl._state
    │                        ~~~~~~
    ├─ kotlinx.coroutines.NodeList
    │    Leaking: UNKNOWN
    │    ↓ NodeList._next
    │               ~~~~~
    ├─ kotlinx.coroutines.ChildHandleNode
    │    Leaking: UNKNOWN
    │    ↓ ChildHandleNode.childJob
    │                      ~~~~~~~~
    ├─ kotlinx.coroutines.StandaloneCoroutine
    │    Leaking: UNKNOWN
    │    ↓ StandaloneCoroutine._state
    │                          ~~~~~~
    ├─ kotlinx.coroutines.ChildHandleNode
    │    Leaking: UNKNOWN
    │    ↓ ChildHandleNode.childJob
    │                      ~~~~~~~~
    ├─ kotlinx.coroutines.UndispatchedCoroutine
    │    Leaking: UNKNOWN
    │    ↓ UndispatchedCoroutine.uCont
    │                            ~~~~~
    ├─ coil.RealImageLoader$load$job$1
    │    Leaking: UNKNOWN
    │    Anonymous subclass of kotlin.coroutines.jvm.internal.SuspendLambda
    │    ↓ RealImageLoader$load$job$1.$request
    │                                 ~~~~~~~~
    ├─ coil.request.LoadRequest
    │    Leaking: UNKNOWN
    │    ↓ LoadRequest.context
    │                  ~~~~~~~
    ╰→ com.example.coilmemoryleak.ActivityA
    ​     Leaking: YES (Activity#mDestroyed is true and ObjectWatcher was watching this)
    ​     key = 0893b148-6f03-4932-ad48-e07e204abb86
    ​     watchDurationMillis = 336257
    ​     retainedDurationMillis = 331257
    , retainedHeapByteSize=69591)], libraryLeaks=[])

Library version
0.6.0

[afollestad]

Yeah the jobs should be scoped. You should listen to the attach state of views that are being loaded into - when they are detached you should cancel the job.

[colinrtwhite]

Thanks for the sample project! Coil sets up a LifecycleObserver for any load requests that should kill any in-flight requests when the associated View is detached or the closest Lifecycle's onDestroy is called. I'll take a look into this later tonight (PST).

get requests are scoped to the containing CoroutineScope, which is not automatically tied to a lifecycle.

[afollestad]

It could be - ideally it'd be automatic so boiler plate is unnecessary. E.g. the load extension method, it knows what view it's loading for and can set an attach state listener to that view which cancels the jobs for that view.

[colinrtwhite]

Coil also listens for detach events on any ViewTargets and restarts the request when the View is reattached - similar to Glide. This is handled automatically.

[afollestad]

Gotcha, haven't looked into everything too much yet so apologies for the ignorance lol.

Excited for this project though, congrats on the release!

[colinrtwhite]

No worries! Thanks for trying the library out.

[colinrtwhite]

Was able reproduce this with the sample application. I've verified that Coil is calling the OkHttp Call.cancel API as soon as the lifecycle is destroyed, however the Request object is being held onto longer than needed. Going to keep looking tomorrow.

[colinrtwhite]

For those following this ticket, I've been prioritizing other issues since the fix for this isn't straightforward. Coil's OkHttpClient is holding onto the request object slightly longer than the containing lifecycle, but it will ultimately be GC'd.

Going to circle back on this when some of the other issues have been fixed, but if anyone from the community has a clean way to fix this, feel free to submit a PR!

[GuilhE]

Any news regarding this issue?
Still exists in version 0.9.5, I'll see if I can manage a workaround or fix.

EDIT: noticed that 0.10.0 is out. I've tried to replicate the leak and I think it's fixed. I had one but it was collected right after.

[andre-artus]

Is this corrected now?

[colinrtwhite]

Closing as fixed as the reproduction project no longer detects a leak with 0.12.0. I believe this was fixed by supporting thread interruption in 0.12.0, which allows for faster request cancellation.