Follow up of: https://github.com/bumptech/glide/pull/2261

[Tolriq]

@sjudd Ok so there's one last side effect remaining after #2245

Opening a new issue as it's a special case, that will maybe needs something on my side to workaround.

As explained on previous issues, I do have a special model that handles some connexion states internaly.
And those states as they do not change the actual picture data, are not handled in the model hashcode/equals so that memory cache can properly be used.

The problem is that the restart of the query after your patch still use the previous model and not the new one with the updated internal flags so does not work.

1. The restart can maybe be updated to use the new model, even if for Glide it's the same as model could have internal states.
2. Adding a way to indicate a cacheKey for the model. To allow different hashcode/equals to indicate when the model is actually different, but still allow memory cache to use the same data as the picture is the same
3. Something else?

[sjudd]

I'm not sure I can support this. Passing in a new model object is plausible, but it's hard to justify.

Could you track connection state another way? Maybe by setting a tag on the View? Or do you need this connection state in the request somewhere (like the ModelLoader).

[Tolriq]

@sjudd well adapter should not know about internal states and model should not modify the views.

Anyway point 1 was just a quick and easy (hackish I agree) workaround to solve the issue.

I tried to point out something also in previous issues, but obviously failed :) With all due respect I think that there's a big design flaw in Glide 4 to use model hashCode/equals to handle cache / equity from Glide point of view.

hashCode / equals are basic java stuff designed to means something precise. The equity of the model. The problem is that you also use that as equity / cache key for the resulting image.

Let's take a way simpler example than my advanced use case.

A user needs to use a custom model / loader to load images from something not supported natively by Glide.

He already have a simple model

class Beach {
 String image;
 String name;
}

To use this model efficiently in Glide (use of cache) it needs to implement hashCode / equals to only take care of image as different beach could share a same image.

But that guy store those in a list and got push of that data. So to avoid duplicates he uses, list.contains.
But then big problem, different models that point to same image will return equality, so will return true when it should not.

Solution: fix hashCode/equals to do what they are meant to, and use all the fields.

But then, Glide is broken and no more use properly it's cache.

Having hashCode/equals to handle to very different purpose put the users in an impossible choice, have their app working correctly or have Glide cache working.

[sjudd]

Did you consider creating a second class that contains just the data you want to be used in Glide? Then you can have two different objects with two different equals/hashcode implementations.

[Tolriq]

@sjudd This is actually the case for my needs and implementation :)

I was just giving another very simple use case that is now quite complicated to handle with Glide 4 compared to Glide 3.

My need is to have different models that contains meta data, internal state, whatever, but still be able to indicate to Glide that the end result image is the same.

In glide 3 this was possible. In Glide 4 there's the Key interface but not only it's only for diskCache but still does take the model as another Key.

For my use case since the object is unique to Glide I did hack the hashCode / equals to be the image Key and not real ones. But this triggers this issue that I can't force a reload from my fetcher with the correct data on second calls.

I'm kinda stuck with no possible solution except maybe rewriting 100% of the app image handling. To be honest I prefer maintaining a fork with a little hack, than rewriting everything with risks for a 4.73 rating millions users app ;) (But I'd really like to not have to)

Maybe another solution would be to have an optional callback configured somehow, that would be called to decide if the query should actually be restarted and passing to the callback the previous model and the new one? (But certainly complicated to handle via generics without breaking the new nice API).

[Tolriq]

Some details about part of why complex use case could help.

My app can connect to multiple media centers and will get the images from the media centers so the actual real download url will be different.
But the actual end image can be the same stored on both media centers so no need to download / cache them twice.
Since multiple media centers can be used at the same time from different context the model neess to handle:
The actual media center and download path.
The cache key.
The status of the media center at request time. To avoid network tries for nothing.(not related to actual phone network status)

All this needs to be handled in the model as it's the proper place for that.
And as such needs a way to handle both equals / hashCode for same images and a way to restart the query if status or media center change.

Maybe it's more clear why there's currently no solution :( Except calling clear of course but then flickering so not really a solution.

[Tolriq]

@sjudd last other small basic use case to show the issue that more looks like my need. Hoping we find an agreement that something might be needed.

Take an application X that can connect to two different servers and display images from those. The app offer a button to switch between servers without stopping the activity so triggering just a refresh of the view.

Now serverA have an image http://servera/image1.png and serverB have an image http://serverb/image1.png.

Both images are the exact same and the app knows it. So to handle this correctly and use Glide cache without duplicates, the app implements a model.

class Image {
String url;
String cacheKey;

hashCode() {
return cacheKey.hashCode();
}

equals() {
returns cacheKey=cacheKey;
}

And all seems to works perfectly, correct cache is used all is perfect.

Then a real user start the app that is pre configured to connect to serverA. The app start, glide load the serverA image url, but since the serverA is down the image is not shown and the error is triggered. All normal.
He then click the button to switch to serverB. The app refresh the view, trigger glide with a new model that says image is at http://serverB. But since it's equals to previous query, glide does not trigger a call to the new server, it reuse the previous model. Connects to serverA and returns an error, when it could have loaded the image.

Maybe this sample explain more, why hashCode / equals poses a problem. I'm not English native but I think it's more or less clear :)
If you really want users to use a dedicated model just for Glide and to have special hashCode/equals to indicate image equity, we need to find something to indicate when image is the same but model is different and new model should be used for the restart.

[sjudd]

The only two differences between v3 and v4 are:

1. We avoid restarting loads if the parameters haven't changed, which you can trivially avoid by clearing the Target manually to exactly duplicate the behavior in Glide v3.
2. We try to avoid starting loads into ImageViews that are waiting for layout.

There's nothing about equals or hashcode that I can think of that changes #1. If we had the same behavior but used the string id from Glide v3 instead of equals/hashcode, you'd still have exactly the same problem. Or am I missing something?

I think your actual issue here is with the flashing that you see if you copy Glide v3's behavior and clear the Target before each load?

[Tolriq]

@sjudd If you use the string id and not the hash code this change all :)

2 different models can share the same image result and cache but still be different models.

This means the issue would not happens, as the model are different you'd clear the query when needed and do a full restart. Yet still use the memory cache.

Yes the end issue is the flickering if we always clear. But what I try to demonstrate is that there's so many cases where clear could be needed that totally remove the utility of Glide 4 optimisations.

The thing is that is the query is the same query then what is done actually is correct. Having devs to always call clear to fix some case issue is counter productive :( As to force users to have a model dedicated to Glide as hashCode/equals needs to be specific for image cache to work.

If the model is actually the same model then current behavior is correct. If the model change but the final image is the same, current behavior is not correct.

Solution would be to have devs to find a way to check by themself if the target already had a load, get previous model if possible then call another compare function then clear then reload.
This sounds like overkill to address something that the framework should be able to manage.

2 different models can share the same image cache. Currently this is not possible.

Edit: Updating to new model on recycle may not be perfect, but this allows things to at least works consistently with the API that force hashCode to be used for something else. I can understand that you find it hackish but then finding another way to properly handle different models sharing image cache is important for Glide 4 to be as effective as Glide 3.

[Tolriq]

@sjudd Since this is mostly an important issue with failed request would you accept a compromise ?

https://github.com/sjudd/glide/blob/master/library/src/main/java/com/bumptech/glide/RequestBuilder.java#L362

Add && !Preconditions.checkNotNull(previous).isFailed() and change comment after. (Maybe also take in account cancelled? (Or do the opposite and reuse previous target only if Completed / running?

This does not introduce fundamental changes to your implementation, yet ensure that failed request are started with the correct updated model in case there's some difference in it to allow the request to have more chance to success ?

[sjudd]

You're nothing if not persistent :).

So I think I'm finally clueing in. You want both the cache key mechanism from Glide v3 and the equals/hashcode mechanism from Glide v4. Where equals/hashcode is used to determine whether or not to restart the request and the cache key is used for the memory cache/request deduplication.

How do you know which model to pass in for a given cache key if you don't know whether the request has failed or not? Would adding a request parameter that allowed you to specify a fallback request that would be run if the main request failed work?

I'm not totally opposed to clearing/restarting if the request is failed, but it seems a little weird since that can already be handled by SingleRequest. How common is it for people to have multiple model objects that have the same cache key? Equals()/hashcode() was supposed to be a more flexible alternative to Glide v3's string cache key because you don't have to find a way to serialize your model to use it in Glide.

[Tolriq]

Well it took me a lot of time to adapt to Glide 4 as I go deeper in my usage than most to optimize every bits. High ratings are hard to get easy to loose :)

And I'm now stuck with no solution to have something that work so yes I insist a little to avoid maintaining a fork.

Anyway about different model and cache key, I don't know if it's that common. But as the little example I gave earlier there's a simple reason to want that. Same cache for different urls. Could be because the app can connect to mutiple servers or frontend for example.
This is my case I have same images that can be accessed from different servers and the user can switch and refresh views. This triggers this issue where I need to have a common cache key but different models to handle the different servers. A callback as you describe would not really help. As failure is normal on some case. Just that a restart with a different model (new server) but same hashCode (image) should actually restart the load on the new server.

I've tested the last proposed approach (on reuse only when completed or running) and this works ok on all cases. Proper cache usage, proper event triggering on load or ignored restart when imahe loaded and proper restart on failure with changed model.

[sjudd]

Could you theoretically store the current server in a singleton instead of in each model and retrieve the current server in the ModelLoader? I guess that would be race prone?

I'm not totally opposed to your idea, I'm just exploring options.

[Tolriq]

Well it's not that simple, my app.connects to multiple servers at the same time to control them. It's not like round robin of internet servers.
An image is really a combination of an url and cache keys.
And I also handle some flag to prevent loading from network but only from cache during transitions (my fetcher cache not Glide's one). (But then should load from network if needed after the transition if needed).

Nothing that can be handled outside of the model to represent the state at the moment of the request.

Sorry to bother you so much.

[sjudd]

I found another issue with my change:

1. Start request A with a RequestListener
2. When request A finishes, in the RequestListener start request B that is identical to request A, except without the RequestListener applied.

Because we're restarting the previous request here: 73a8e01#diff-a38a476f7aa7c143d4cde2716bb2b59dR369 rather than the new request, the two requests I listed above will recurse infinitely. Our new assertion that throws if a request is started in a RequestListener or Target will avoid a stack overflow, but if users just post to a Handler as the assertion suggests, the will still end up looping requests infinitely. Given that they're not setting the RequestListener on the second requests, it's not at all obvious that the requests will loop.

I don't think we want to include the RequestListener in isEquivalentTo because it's common for people to pass in new anonymous inner classes that will never implement equals/hashCode.

It seems like it might be reasonable to just start the new request, instead of restarting the old request? It's somewhat less efficient because we have to build the new SingleRequest object and any thumbnail or error sub requests, but it probably results in better behavior.

@Tolriq would just starting the new request, rather than restarting the old request, unless the old request is actively running fix your issues?

[Tolriq]

Well yes but then it removes most of the optimizations done no?

[sompor038]

sompor038

[sjudd]

Wups @Tolriq sorry I missed your comment. I think it keeps the optimization. The current behavior is that if the requests are equivalent, but the old request is not running, we restart the old request. My proposal is that we do the same thing, but instead of restarting the old request, we start the new request.

The cases where we do or don't start a request don't change. The only thing that will change is which request we start.

[sjudd]

Oh no I"m wrong it does remove some of the optimizations added in SingleRequest.