How to debug cancellation of load requests

[clhols]

I am migrating from Picasso to Coil. It went quite easy as the APIs are very similar.

I have a custom Fetcher for loading internal schemes.

I load a list of items in a recycler view, each having a logo loaded with Coil by the custom fetcher.

When I scroll up and down the recycler view list, I see that sometimes a wrong logo is displayed on some of the items. It seems as if the load request, which wasn't done, jumped to one of the new items coming into view. I have to scroll up and down the list a few times to trigger it.

My guess it that the inflight request connected to the image view isn't cancelled or maybe a bitmap is reused?

I am pretty sure it isn't a problem with the recycler view recycling of views as Picasso handled it just fine. Also the test on the items is correct. Only the logo is wrong.

Can you guide me on how to debug this?

[colinrtwhite]

Hmm it does sound like your requests aren't being cancelled. Are you using target(imageView) or imageView.load? Also here's how to enable debug logging.

[clhols]

By you asking if I used target(imageView) I realise what the problem is. I must have turned off my brain when implementing it. I am using target(onStart, onSuccess, onError) and now realise that the loader can't know when the imageView is detached, because it doesn't have a reference to it.

I am using that target call because imageView.load was clearing a manually added placeholder if loading fails. I though that imageView.load wouldn't touch the image view if I didn't add an error drawable and loading would failed. But it does. Is there a way to handle the cancellation automatically and keep using target(onStart, onSuccess, onError) ?

[clhols]

To my surprise switching to imageView.load did not resolve the issue. Can a custom Fetcher affect the cancellation in some way?

[colinrtwhite]

@clhols To support automatic cancellation when the view is detached, you could pass a custom ViewTarget. Also to keep the same placeholder when the request fails you could set error(R.id.placeholder).

[clhols]

I could pass a custom ViewTarget that changes the error behavior. But it still doesn't explain why the cancelation fails.

[colinrtwhite]

If you enable debug logging you can see if the requests are being cancelled. If you don't set a ViewTarget on your request, requests will only be cancelled if your Lifecycle is destroyed, or RequestDisposable.dispose is called.

[clhols]

The debug logging shows that requests to the custom fetcher don't get cancelled. Only normal HTTP requests are cancelled.

[clhols]

I see that requests that are successful can get cancelled, but requests that fail and throw an exception in the fetcher don't get cancelled and loads the error placeholder.

[clhols]

I think I found the cause.

If customFetcher.fetch(...) doesn't check cancellation before throwing the exception of a loading failure, then the throwable in deferred.invokeOnCompletion { ... } in RealImageLoader.executeInternal(...) will not be a CancellationException and it will handle it as an error scenario. I think it should check if the job has been cancelled instead of relying on the custom fetcher to behave properly. It could also throw an exception telling that the fetcher didn't handle the cancellation as it should.

I my case I had a suspendCoroutine for handling callbacks from service requests. But it doesn't handle cancellation so I had to change it to a suspendCancellableCoroutine.

[clhols]

@colinrtwhite What is your take on this? Am I wrong? I also see issues with images loaded from the built in HttpFetcher. Even when they are in the memory cache, they can jump onto the wrong image views.

[colinrtwhite]

Yep, you'll need to use suspendCancellableCoroutine to support cancellation as coroutine cancellation is cooperative. If possible you should set continuation.invokeOnCancellation as well (HttpFetcher uses it internally).

As for images jumping to the wrong image view, I'm not sure I can figure out what's happening with the current info. Do you have a sample project that reproduces the issue I could take a look at?

[clhols]

I will see if I can create a sample project that reproduces it.

What about my idea of having RealImageLoader check if the job was cancelled instead of relying on the fetcher to do proper cancellation?

[clhols]

@colinrtwhite I decided to debug it instead as I though it would be easier than creating the sample project. Here is what I found: On some items in the recycler view I call setImageResource(...) on the imageView. When the item is recycled and Coil is requested to load, it sets a ViewTargetRequestManager using setTag on the image view. That item now scrolls off screen and the request is cancelled. Then it scrolls back onto the screen, the onBindViewHolder in the recycler view calls setImageResource(...), but the ViewTargetRequestManager is still there in the imageViews tag, so its onViewAttachedToWindow is called and it calls request.restart() and it loads the wrong image. So somehow the setImageResource(...) needs to remove the ViewTargetRequestManager.

[colinrtwhite]

@clhols RealImageLoader already checks for cancellation after the fetcher has returned here.

Ah yep you shouldn't mix setImageResource/setImageDrawable and load. It's not possible to hook into setImageResource without creating a custom ImageView. I believe Glide has the same issue. You can clear the Coil request attached to the view with clear().

[clhols]

I believe that it doesn't reach that code if the fetcher throws an exception. Otherwise my initial implementation would have worked.

What is the use case for that restart of a cancelled load? Is it just an optimization?

[colinrtwhite]

It's needed for proper recycling behaviour with Fragments/RecyclerViews. Without it, Fragments that are detached and moved to the backstack and RecyclerView views that are in the View pool would hold active lifecycle observers and wouldn't recycle their images. This would add a lot more memory pressure to the application.

[clhols]

Do you know how Picasso handles the same issue? Because it didn't have issues with mixing setImageResource and load.

[colinrtwhite]

I believe Picasso doesn't clear the view in onViewDetachedFromWindow and doesn't restart requests (though I'm not 100% sure). This will result in higher memory pressure when scrolling in lists + using Fragments. Picasso can also avoid this because they don't integrate with androidx lifecycles and won't leak a lifecycle observer if they don't clear when the view is detached.

[rakshitsoni02]

@clhols how did you handle the case when scrolling the recycler view loads the wrong image?

I tried clear() before making a new request but I think that doesn't make any difference

[clhols]

@rakshitsoni02 I think we just made it load all images with Coil. If you do that and still see wrong images, then it could be an issue in your view holder or maybe in onBind.