Catch OutOfMemoryError in decode for v3

[ihenchi]

No description provided.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[ihenchi]

All commit authors have now been verified and signed the CLA.

[googlebot]

CLAs look good, thanks!

[TWiStErRob]

Hi, thanks.
What's the idea behind this?
How is this going to solve OOMs?

[dmapr]

It is not going to solve OOMs, but it will ensure that you will get a callback (onException) in your listener when an OOM occurs. Right now if an OOM occurs then you don't get any callback (neither onException nor onResourceReady). In our particular case it happens when loading large animated GIFs and it allows us to fallback to single frame loading.

[TWiStErRob]

I don't feel comfortable with that "fake" RuntimeException, but there isn't much we can do about the Error vs Exception hierarchy clash in v3. Let's see what @sjudd says.

Btw, are you aware of #859?

[dmapr]

Thanks! We weren't aware of it; somehow it hasn't popped up when we were searching for the existing issues. @ihenchi, let's take a look at the approach suggested in the #859 and see if we can get that to work. We also gotta add a test case for OOM in the EngineRunnableTest to bring the coverage back where it belongs.

[sjudd]

Sorry for the delay. I'm torn here. I don't generally like the idea of catching OOMs, it's usually a sign that something else is wrong and catching the OOMs just delays the inevitable. Super large GIFs or other assets that we can't downsample effectively make this seem more reasonable...

I guess it's reasonable to at least expose the error so you can respond to it.

Thanks for putting this up!

[TWiStErRob]

@ihenchi @dmapr where did #859 lead you to?

I'm starting to agree with this PR, but I think it needs to be amended a little:

• deliver the OOM (wrapped in an exception) to listener.onException,

• I would say a ErrorWrappingGlideException or similar would probably a good way to distinguish
  (internally and in listener too, as documented behavior for OOM)

• if there's no listener or the listener doesn't return true (=handled), trigger default behavior via:

  throw (OutOfMemoryError)ex.getCause()

• let the thread pool handle it with its UncaughtThrowableStrategy as before.

@sjudd currently default strategy is LOG anyway, so Glide catches the OOM, but there's no way to handle it, only if the developer sees it in the log during development can act on it. Also the current behavior is to just display the placeholder in perpetuity, because the thread handling the request that would've called target.onLoadFailed/listener.onException is "dead" (stopped prematurely). This also means that no other Target method is called either (e.g. onLoadCleared). With this PR either a "retry with different load" or "show .error()" action can be implemented.

[TWiStErRob]

Note: a transformation, or transcoder can also throw this error.

[dmapr]

@TWiStErRob, at a glance it seemed like we would need to implement the #859 for multiple decoders (Bitmap, GIF, etc.) so it was less attractive than doing it in the EngineRunnable.

[TWiStErRob]

This would probably allow fixing #491.

[sjudd]

Ok I'm ok with this if you create a new type of Exception, rather than just a runtime exception. A specific subclass will make it easier to catch this particular case. Otherwise lgtm.

[dmapr]

Sorry for the delay, I was on vacation and @ihenchi had his hands full with the day job. We'll try to put this together to satisfy your requirements.

[TWiStErRob]

No worries, better taking the time than getting it wrong.

[coveralls]

Coverage increased (+0.09%) to 83.247% when pulling a89dda8 on jivesoftware:3.0 into 82209f7 on bumptech:3.0.

[TWiStErRob]

I think if it's error wrapping it should expect an Error in ctor.

Also along the same lines maybe override getCause() with a covariant return type:

@Override public Error getCause () ...

[TWiStErRob]

@ihenchi I was wondering why you ignored my 3rd point in #1057 (comment), but then I investigated and I saw that it's not really possible, because that would crash the host application, because the exception is moved from the fetcher thread to the UI thread by the time we ask the listener if it's handled. @sjudd Do you think it is safe to ignore that point?

[dmapr]

Yes, sorry, forgot to reply to the third point. Yes, I don't think we can guarantee that the unprocessed exception can be delivered to the UncaughtThrowableStrategy once the EngineRunnable completes.

[TWiStErRob]

There would be way, but it's hacky: somehow manually post a new Runnable to the thread pool which just re-throws the error.

[dmapr]

I believe such an approach would increase the complexity beyond our comfort level. Let's wait to see what @sjudd says before we start down that path.

[TWiStErRob]

Agreed, I'm not comfortable with it either, just an option.

[TWiStErRob]

@ihenchi @dmapr can you please replace the exception with this (changes: package, ctor, getter):

package com.bumptech.glide.load.engine;

/**
 * An exception class used for wrapping and distinguishing errors such as {@link OutOfMemoryError}.
 */
public class ErrorWrappingGlideException extends Exception {
    public ErrorWrappingGlideException(Error error) {
        super(error);
        if (error == null) {
            throw new NullPointerException("The causing error must not be null");
        }
    }

    @Override
    public Error getCause() {
        // cast is safe because constructor ensures there must be a cause, and it must be an Error
        return (Error) super.getCause();
    }
}

and then squash all the commits into one.

@TWiStErRob I've noted above that transcoders and transformations can also throw, but they're both deep within decode() so they're all caught.

@sjudd please acknowledge that it is safe to ignore my third point above, I think that's a breaking change as it invalidates the existence of the central UncaughtThrowableStrategy for OOM and would force users to attach listeners to each load to even see OOMs in logs. Catching the OOM will delegate it outside the executor's reach, and we only know on the UI thread in GenericRequest whether we actually have a listener or not to decide what to do. Throwing at that point would always crash the app, instead of the current perpetual placeholder behavior.

[sjudd]

UncaughtThrowableStrategy isn't really an API, it's a way to expose errors that occur, primarily for developers. I think what we're saying with this PR is that an OOM shouldn't really be an error because there are times where OOMs are unavoidable.

[TWiStErRob]

Ah, true that.

would force users to attach listeners to each load to even see OOMs in logs.

Actually, this is currently true for all exceptions, if you want the stack trace. Since the exceptions are logged in log.tag.EngineRunnable anyway, the OOMs would be logged too.

So the change that will occur when merging this is that instead of perpetual placeholder OOMs would be showing the error drawable like any other exception, and the lifecycle methods will be called as expected. Degrading OOM from unexpected error to expected error.

@ihenchi @dmapr I think we're all on the same page. Please do the actions in #1057 (comment) and we're good to go.

[coveralls]

Coverage decreased (-0.009%) to 83.221% when pulling ef3b05d on jivesoftware:3.0 into 3881e22 on bumptech:3.0.

[coveralls]

Coverage increased (+0.03%) to 83.255% when pulling 9eef8a9 on jivesoftware:3.0 into 3881e22 on bumptech:3.0.

[dmapr]

@TWiStErRob @sjudd — updated as requested.

[sjudd]

Thanks! I appreciate your patience here.

[dmapr]

Thanks! Do you have an ETA for 3.8.0?

[TWiStErRob]

Not sure, but you can use it right away: https://github.com/bumptech/glide/wiki/Snapshots