Double-checked locking *may* be broken in RequestManagerRetriever.get()

[TWiStErRob]

See http://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java
particularly the third code block and the explanation after, which is the same as
https://github.com/bumptech/glide/blob/master/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java#L37

I'm not sure if this affects Android, worth looking. HelperHolder looks like a clean solution for all environments.

[sjudd]

After reading that I'm pretty sure you're right. It should be rare, there probably isn't a lot of concurrency in most android apps since few will start loads on anything other than the main thread, but we should fix it. I agree the holder looks reasonable.

[sjudd]

I'm actually a bit off here, this is almost ok on Java 6+ http://en.wikipedia.org/wiki/Singleton_pattern#Lazy_initialization. In this case I'm missing volatile, but otherwise the pattern is fine.

[illarionov]

Looks like the developers of android consider the "double check locking" pattern is fine with volatile (SMP primer)

[sjudd]

Also missing volatile here: https://github.com/bumptech/glide/blob/8bb5e18b09fa9246836fede0db6f86145df00cd6/integration/okhttp/src/main/java/com/bumptech/glide/integration/okhttp/OkHttpUrlLoader.java

[TWiStErRob]

Actually, I read into the next section: http://en.wikipedia.org/wiki/Singleton_pattern#Eager_initialization and my brain took a turn: I think you don't even have to be lazy here (is it premature maybe to use lazy singleton?), reasons being:

• RequestManagerRetriever only has one public static method: get() which will definitely return an instance, hence the first time you load the class, you can be 99.99% sure that the next step after class-load will be a get() call, and then probably the instance level gets will be called.
• Based on the previous one, you don't even need a holder, because if RequestManagerRetriever is loaded, get will be called.
• RequestManagerRetriever is lightweight, only two HashMaps and a Handler (which doesn't instantiate anything, just gets existing references);
• also classes that may be loaded with loading RequestManagerRetriever probably doesn't include much which is not loaded already
• not using double-checked locking, but eager init is actually faster if all the above is accepted (no synchornized)

The same is not true for RequestManager which needs arguments from get, but based on @illarionov, that's cool because it has volatile.

[sjudd]

Hmm good point, lazy initialization is probably overkill, particularly since it will definitely be triggered by any Glide.load call.