Improve RequestBuilder.submitAsync() to ensure the coroutine continuation is resumed only once

[cbeyls]

RequestListener.onResourceReady() may also be called to provide the placeholder image when the request is starting or when it is cleared.

Check if the current request is complete (its status set to COMPLETE) to determine if the Glide resource is for a thumbnail/placeholder or the final image, and resume the coroutine only for the final image (or in case of error).
This logic is borrowed from the Glide Flow API.

[connyduck]

This is still not working, the following crash is reported in the Google Play console for version 26.0:

Exception java.lang.IllegalStateException:
  at kotlinx.coroutines.CancellableContinuationImpl.alreadyResumedError (CancellableContinuationImpl.kt:551)
  at kotlinx.coroutines.CancellableContinuationImpl.resumeImpl (CancellableContinuationImpl.kt:516)
  at kotlinx.coroutines.CancellableContinuationImpl.resumeImpl$default (CancellableContinuationImpl.kt:489)
  at kotlinx.coroutines.CancellableContinuationImpl.resumeWith (CancellableContinuationImpl.kt:364)
  at com.keylesspalace.tusky.util.GlideExtensionsKt$submitAsync$2$target$1.onResourceReady (GlideExtensions.kt:40)
  at com.bumptech.glide.request.SingleRequest.onResourceReady (SingleRequest.java:650)
  at com.bumptech.glide.request.SingleRequest.onResourceReady (SingleRequest.java:596)
  at com.bumptech.glide.request.SingleRequest.begin (SingleRequest.java:243)
  at com.bumptech.glide.manager.RequestTracker.resumeRequests (RequestTracker.java:115)
  at com.bumptech.glide.RequestManager.resumeRequests (RequestManager.java:339)
  at com.bumptech.glide.RequestManager.onStart (RequestManager.java:364)
  at com.bumptech.glide.manager.ApplicationLifecycle.addListener (ApplicationLifecycle.java:15)
  at com.bumptech.glide.RequestManager$1.run (RequestManager.java:84)
  at android.os.Handler.handleCallback (Handler.java:938)
  at android.os.Handler.dispatchMessage (Handler.java:99)
  at android.os.Looper.loopOnce (Looper.java:226)
  at android.os.Looper.loop (Looper.java:313)
  at android.app.ActivityThread.main (ActivityThread.java:8663)
  at java.lang.reflect.Method.invoke
  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run (RuntimeInit.java:567)
  at com.android.internal.os.ZygoteInit.main (ZygoteInit.java:1135)

😭

[cbeyls]

Glide's API is a mess.
If the crash still occurs it means that a Request may dispatch a new resource even after being complete which makes no sense. Anyway, I suppose we can just silently ignore the future resources that arrive after the continuation is resumed.

[cbeyls]

Glide's application RequestManager keeps tracking targets and their request using weak references unless they are explicitly removed using RequestManager.clear(target).
So the target could be tracked for a very long time after the request completes because weak references are non-deterministic.

If a request like this one is still tracked and Glide detects the app came back to the foreground, it will send the result again to the target even if the request is complete:

      // If we're restarted after we're complete (usually via something like a notifyDataSetChanged
      // that starts an identical request into the same Target or View), we can simply use the
      // resource and size we retrieved the last time around and skip obtaining a new size, starting
      // a new load etc. This does mean that users who want to restart a load because they expect
      // that the view size has changed will need to explicitly clear the View or Target before
      // starting the new load.
      if (status == Status.COMPLETE) {
        onResourceReady(
            resource, DataSource.MEMORY_CACHE, /* isLoadedFromAlternateCacheKey= */ false);
        return;
      }

So this is the cause of the issue.

There is no clean way to fix this.

The extension method doesn't have access to the RequestManager and even if we add it as extra argument, it would be hacky to call clear(target) manually, and it could potentially deliver another result (the placeholder) to the continuation during the clear which is not helping.

It is not possible to unregister a request listener on a Request after is has been registered, so the target will keep tracking the continuation until the weak reference disappears.

It is possible to set the request to null on the Target (which would clear the request listeners), but the Glide documentation says this API is reserved for Glide usage only and it may cause issues.

What looks the best to me is to create a RequestListener that keeps a reference to the continuation until the result arrives then clears that reference. This way we are sure that the continuation will only be resumed once, and no reference will be kept to the parent coroutine after resuming the continuation.

[connyduck]

Maybe we should just catch the exception?

[cbeyls]

Maybe we should just catch the exception?

We can test continuation.isActive to prevent the exception from happening but I think it's better to clear the continuation reference to make sure it's not leaking.