Search for a command to run...
Nice article! Good explanations!
Why not to make the imageView in the callback as a weak reference?
Because canceling the request means we can save network related resources
Thanks for your reply. I agree with your point that it is best practice to cancel the request once the ImageView is ready to be destroyed to save the network/cpu resource.
My concern here is
onDetachedFromWindow is the callback on View instead of Activity/Fragment. So it might be an issue when the ImageView is inside one of the ViewPager. Refer to "Android view - onAttachedToWindow and onDetachedFromWindow - when are they called in the activity lifecycle" in StackOverFlow.
Besides, I tried to understand why the leak happens here. The root cause seems to me that the ImageView instance gets referenced by the callback which is encapsulated into ImageViewAction. For example, if we did show a snackBar inside the callback instead of imageView.setBackgroundColor(), it would not be a memory leak because imageView is garbage collected properly and the callback too I think.
In my mind, the root cause is exactly the WeakHashMap warning you mentioned. But it is the best solution to cancel the request when Activity/Fragment arrives in the destroyed state.
Exactly one year ago, after 12 years in San Francisco, I moved back to France. Life since has been a whirlwind: selling the condo, moving a container overseas, rediscovering French culture (I had to r
The SQLDelight documentation provides this example: val players: Flow<List<HockeyPlayer>> = playerQueries.selectAll() .asFlow() .mapToList(Dispatchers.IO) This looks reasonable, right? In the Square Point Of Sale application, we recently ...
A failed experiment with dominator trees
In this article I run the new LeakCanary toolkit against the Slack Android app. Read on to learn a bunch! A new LeakCanary toolkit In two months, I will give a talk at Droidcon SF: Cutting Edges: universal heap trimming with LeakCanary 3 At Square, ...
Today I was mob programming with Square's Mobile & Performance Reliability team and we toyed with an interesting idea. Our codebase has classes that represent screens a user can navigate to. These classes are defined in modules, and these modules hav...
👋 Hi, this is P.Y., I work as an Android Engineer at Block. Every month I organize an internal Ensemble Leak Hunting session where we look at the top leaks reported by LeakCanary to learn how to read and investigate leak traces. This is a write-up of our latest investigation which surfaced a common mistake when using Picasso.
Before we start, you might want to get a quick refresher on how to read leak traces. In this article, we'll find the root cause of the leak, discuss how to fix it, and what could change in Picasso to help avoid this common mistake.
Here's the leak trace:
┬───
│ GC Root: System class
│
├─ com.example.PicassoHolder class
│ Leaking: NO (a class is never leaking)
│ ↓ static PicassoHolder.singleton
├─ com.squareup.picasso3.Picasso instance
│ ↓ Picasso.targetToAction
│ ~~~~~~~~~~~~~~
├─ java.util.WeakHashMap instance
│ ↓ WeakHashMap.table
│ ~~~~~
├─ java.util.WeakHashMap$Entry[] array
│ ↓ WeakHashMap$Entry[0]
│ ~~~
├─ java.util.WeakHashMap$Entry instance
│ ↓ WeakHashMap$Entry.value
│ ~~~~~
├─ com.squareup.picasso3.ImageViewAction instance
│ ↓ ImageViewAction.callback
│ ~~~~~~~~
├─ com.example.ProfileLayout\(loadImage\)1 instance
│ Anonymous class implementing com.squareup.picasso3.Callback
│ ↓ ProfileController\(loadImage\)1.this$0
│ ~~~~~~
╰→ com.example.ProfileLayout instance
Leaking: YES (ObjectWatcher was watching this because ProfileLayout received View#onDetachedFromWindow() callback)
┬───
│ GC Root: System class
│
├─ com.example.PicassoHolder class
│ Leaking: NO (a class is never leaking)
│ ↓ static PicassoHolder.singleton
├─ com.squareup.picasso3.Picasso instance
...
At the top of the leak trace, a Picasso instance. We know that Picasso instances are long-lived singletons, meant to stay around as UI comes and goes. So we know this is legit and we can move the investigation further down in the leak trace.
Picasso.targetToAction...
├─ com.squareup.picasso3.Picasso instance
│ ↓ Picasso.targetToAction
│ ~~~~~~~~~~~~~~
├─ java.util.WeakHashMap instance
│ ↓ WeakHashMap.table
│ ~~~~~
├─ java.util.WeakHashMap$Entry[] array
│ ↓ WeakHashMap$Entry[0]
│ ~~~
├─ java.util.WeakHashMap$Entry instance
│ ↓ WeakHashMap$Entry.value
│ ~~~~~
├─ com.squareup.picasso3.ImageViewAction instance
...
Picasso.targetToAction is a WeakHashMap of target keys (i.e. views) to action values (i.e. what to load on those views). WeakHashMap has weak keys and strong values, i.e. keys are held by weak references, and when a weak reference to a key clears, its entry is removed. So Picasso.targetToAction holds weak references to the target views & strong references to the corresponding actions.
ImageViewAction.callback...
├─ com.squareup.picasso3.ImageViewAction instance
│ ↓ ImageViewAction.callback
│ ~~~~~~~~
├─ com.example.ProfileLayout\(loadImage\)1 instance
│ Anonymous class implementing com.squareup.picasso3.Callback
│ ↓ ProfileController\(loadImage\)1.this$0
│ ~~~~~~
╰→ com.example.ProfileLayout instance
Leaking: YES (ObjectWatcher was watching this because ProfileLayout received View#onDetachedFromWindow() callback)
ImageViewAction.callback is a Picasso action that holds a custom callback created in ProfileLayout.loadImage(). We can see that this custom Picasso callback is an anonymous class that has a reference to its outer class, ProfileLayout. We know ProfileLayout has been detached for at least 5 seconds as that's LeakCanary's minimum trigger duration. The ProfileLayout.loadImage() code shows that the callback sets a custom background color when the image fails to load:
fun loadImage() {
picasso.load(imageUri)
.into(
imageView,
object : Callback {
override fun onSuccess() = Unit
override fun onError(t: Throwable) {
// Set dark background if image fails to load
imageView.setBackgroundColor(backgroundColor)
}
}
)
}
Looking at the Picasso source code we can see that Picasso.targetToAction entries are removed when a request completes, so we can conclude that the image loading request was still in flight and that ProfileLayout was detached and prevented from being garbage collected by that request in flight.
It's a common mistake when using Picasso: image-loading requests should be canceled if the UI goes away before the request completes, otherwise, they'll keep going and consume resources until success or failure, which could take a long time on a slow network.
We forgot to cancel the request! Let's fix that:
override fun onDetachedFromWindow() {
super.onDetachedFromWindow()
picasso.cancelRequest(imageView)
}
This fixes the leak, yay! Alternatively, we could also use a tag.
Forgetting to cancel Picasso requests when the UI goes away is a fairly common mistake. How comes leaks don't show up more often?
First, Picasso automatically cancels any previous request when loading a new image on the same image view. This helps with adapter views: when a list item view gets recycled and bound to a new row, a new request is started for that view and the previous one is automatically canceled.
Second, even if we forget to cancel a Picasso request, that will only trigger a leak when using a custom callback with a strong reference to the detached ImageView. How comes?
WeakHashMapPicasso.targetToAction is a WeakHashMap that holds weak references to the target views & strong references to the corresponding actions.
ImageViewAction itself (source) holds a weak reference to its target ImageView, and a strong reference to its optional Callback. So when the custom callback isn't set, ImageViewAction doesn't hold any strong reference to the view:
internal class ImageViewAction(
picasso: Picasso,
target: ImageView,
data: Request,
var callback: Callback?
) : Action(picasso, data) {
private val targetReference = WeakReference(target)
As you can see, so far there are no strong references to any view. This means that the default usage of Picasso will not introduce leaks, even if we forget to cancel requests when UI goes away.
Now let's add a custom callback with a strong reference to a view:
Note that if we replaced the callback strong reference with a weak reference, there would be no strong reference holding the callback in memory and it would be garbage collected before the request completes, even when the view is still attached. That's not what we want. So we do need the callback strong reference, which unfortunately means we have a strong reference path to ProfileLayout and its associated view hierarchy, which causes a leak.
This is exactly what the WeakHashMap documentation warns us about:
The value objects in a WeakHashMap are held by ordinary strong references. Thus care should be taken to ensure that value objects do not strongly refer to their own keys, either directly or indirectly, since that will prevent the keys from being discarded.
View.setTag(int, Object)Ideally Picasso would have a consistent memory behavior whether or not callback is set. One way to do that is to store the ImageViewAction directly in the ImageView, as a view tag. That way, Picasso.targetToAction would only hold a weak reference to the ImageView and as soon as the view is detached it becomes unreachable:
Alternatively, Picasso could set a detach listener on the ImageView and auto cancel the requests on detach. This would be best for saving resources but might create surprises if e.g. the app is preloading an image into a detached view.
That's all for now, hope you enjoyed reading this!
Header image generated by DALL-E, prompt: "image inspired by the style of Pablo Picasso, depicting a leaky faucet with bold, abstract shapes and bold colors".
