Skip to content

Commit

Permalink
Avoid blocking Futures forever on unexpected load failures.
Browse files Browse the repository at this point in the history
  • Loading branch information
sjudd committed Oct 27, 2017
1 parent d447e58 commit 74fcad1
Show file tree
Hide file tree
Showing 9 changed files with 75 additions and 68 deletions.
11 changes: 6 additions & 5 deletions library/src/main/java/com/bumptech/glide/Registry.java
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ public class Registry {
private final ModelToResourceClassCache modelToResourceClassCache =
new ModelToResourceClassCache();
private final LoadPathCache loadPathCache = new LoadPathCache();
private final Pool<List<Exception>> exceptionListPool = FactoryPools.threadSafeList();
private final Pool<List<Throwable>> throwableListPool = FactoryPools.threadSafeList();

public Registry() {
this.modelLoaderRegistry = new ModelLoaderRegistry(exceptionListPool);
this.modelLoaderRegistry = new ModelLoaderRegistry(throwableListPool);
this.encoderRegistry = new EncoderRegistry();
this.decoderRegistry = new ResourceDecoderRegistry();
this.resourceEncoderRegistry = new ResourceEncoderRegistry();
Expand Down Expand Up @@ -454,8 +454,9 @@ public <Data, TResource, Transcode> LoadPath<Data, TResource, Transcode> getLoad
if (decodePaths.isEmpty()) {
result = null;
} else {
result = new LoadPath<>(dataClass, resourceClass, transcodeClass, decodePaths,
exceptionListPool);
result =
new LoadPath<>(
dataClass, resourceClass, transcodeClass, decodePaths, throwableListPool);
}
loadPathCache.put(dataClass, resourceClass, transcodeClass, result);
}
Expand All @@ -480,7 +481,7 @@ private <Data, TResource, Transcode> List<DecodePath<Data, TResource, Transcode>
ResourceTranscoder<TResource, Transcode> transcoder =
transcoderRegistry.get(registeredResourceClass, registeredTranscodeClass);
decodePaths.add(new DecodePath<>(dataClass, registeredResourceClass,
registeredTranscodeClass, decoders, transcoder, exceptionListPool));
registeredTranscodeClass, decoders, transcoder, throwableListPool));
}
}
return decodePaths;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class DecodeJob<R> implements DataFetcherGenerator.FetcherReadyCallback,
private static final String TAG = "DecodeJob";

@Synthetic final DecodeHelper<R> decodeHelper = new DecodeHelper<>();
private final List<Exception> exceptions = new ArrayList<>();
private final List<Throwable> throwables = new ArrayList<>();
private final StateVerifier stateVerifier = StateVerifier.newInstance();
private final DiskCacheProvider diskCacheProvider;
private final Pools.Pool<DecodeJob<?>> pool;
Expand Down Expand Up @@ -187,7 +187,7 @@ private void releaseInternal() {
currentFetcher = null;
startFetchTime = 0L;
isCancelled = false;
exceptions.clear();
throwables.clear();
pool.release(this);
}

Expand Down Expand Up @@ -227,19 +227,25 @@ public void run() {
return;
}
runWrapped();
} catch (RuntimeException e) {
} catch (Throwable t) {
// Catch Throwable and not Exception to handle OOMs. Throwables are swallowed by our
// usage of .submit() in GlideExecutor so we're not silently hiding crashes by doing this. We
// are however ensuring that our callbacks are always notified when a load fails. Without this
// notification, uncaught throwables never notify the corresponding callbacks, which can cause
// loads to silently hang forever, a case that's especially bad for users using Futures on
// background threads.
if (Log.isLoggable(TAG, Log.DEBUG)) {
Log.d(TAG, "DecodeJob threw unexpectedly"
+ ", isCancelled: " + isCancelled
+ ", stage: " + stage, e);
+ ", stage: " + stage, t);
}
// When we're encoding we've already notified our callback and it isn't safe to do so again.
if (stage != Stage.ENCODE) {
exceptions.add(e);
throwables.add(t);
notifyFailed();
}
if (!isCancelled) {
throw e;
throw t;
}
} finally {
// Keeping track of the fetcher here and calling cleanup is excessively paranoid, we call
Expand Down Expand Up @@ -309,7 +315,7 @@ private void runGenerators() {

private void notifyFailed() {
setNotifiedOrThrow();
GlideException e = new GlideException("Failed to load resource", new ArrayList<>(exceptions));
GlideException e = new GlideException("Failed to load resource", new ArrayList<>(throwables));
callback.onLoadFailed(e);
onLoadFailed();
}
Expand Down Expand Up @@ -379,7 +385,7 @@ public void onDataFetcherFailed(Key attemptedKey, Exception e, DataFetcher<?> fe
fetcher.cleanup();
GlideException exception = new GlideException("Fetching data failed", e);
exception.setLoggingDetails(attemptedKey, dataSource, fetcher.getDataClass());
exceptions.add(exception);
throwables.add(exception);
if (Thread.currentThread() != currentThread) {
runReason = RunReason.SWITCH_TO_SOURCE_SERVICE;
callback.reschedule(this);
Expand All @@ -400,7 +406,7 @@ private void decodeFromRetrievedData() {
resource = decodeFromData(currentFetcher, currentData, currentDataSource);
} catch (GlideException e) {
e.setLoggingDetails(currentAttemptingKey, currentDataSource);
exceptions.add(e);
throwables.add(e);
}
if (resource != null) {
notifyEncodeAndRelease(resource, currentDataSource);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,13 @@ public class DecodePath<DataType, ResourceType, Transcode> {
private final Class<DataType> dataClass;
private final List<? extends ResourceDecoder<DataType, ResourceType>> decoders;
private final ResourceTranscoder<ResourceType, Transcode> transcoder;
private final Pool<List<Exception>> listPool;
private final Pool<List<Throwable>> listPool;
private final String failureMessage;

public DecodePath(Class<DataType> dataClass, Class<ResourceType> resourceClass,
Class<Transcode> transcodeClass,
List<? extends ResourceDecoder<DataType, ResourceType>> decoders,
ResourceTranscoder<ResourceType, Transcode> transcoder, Pool<List<Exception>> listPool) {
ResourceTranscoder<ResourceType, Transcode> transcoder, Pool<List<Throwable>> listPool) {
this.dataClass = dataClass;
this.decoders = decoders;
this.transcoder = transcoder;
Expand All @@ -47,7 +47,7 @@ public Resource<Transcode> decode(DataRewinder<DataType> rewinder, int width, in

private Resource<ResourceType> decodeResource(DataRewinder<DataType> rewinder, int width,
int height, Options options) throws GlideException {
List<Exception> exceptions = listPool.acquire();
List<Throwable> exceptions = listPool.acquire();
try {
return decodeResourceWithList(rewinder, width, height, options, exceptions);
} finally {
Expand All @@ -56,7 +56,7 @@ private Resource<ResourceType> decodeResource(DataRewinder<DataType> rewinder, i
}

private Resource<ResourceType> decodeResourceWithList(DataRewinder<DataType> rewinder, int width,
int height, Options options, List<Exception> exceptions) throws GlideException {
int height, Options options, List<Throwable> exceptions) throws GlideException {
Resource<ResourceType> result = null;
for (int i = 0, size = decoders.size(); i < size; i++) {
ResourceDecoder<DataType, ResourceType> decoder = decoders.get(i);
Expand All @@ -68,7 +68,7 @@ private Resource<ResourceType> decodeResourceWithList(DataRewinder<DataType> rew
}
// Some decoders throw unexpectedly. If they do, we shouldn't fail the entire load path, but
// instead log and continue. See #2406 for an example.
} catch (IOException | RuntimeException e) {
} catch (IOException | RuntimeException | OutOfMemoryError e) {
if (Log.isLoggable(TAG, Log.VERBOSE)) {
Log.v(TAG, "Failed to decode data for " + decoder, e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,20 @@
public final class GlideException extends Exception {
private static final StackTraceElement[] EMPTY_ELEMENTS = new StackTraceElement[0];

private final List<Exception> causes;
private final List<Throwable> causes;
private Key key;
private DataSource dataSource;
private Class<?> dataClass;

public GlideException(String message) {
this(message, Collections.<Exception>emptyList());
this(message, Collections.<Throwable>emptyList());
}

public GlideException(String detailMessage, Exception cause) {
public GlideException(String detailMessage, Throwable cause) {
this(detailMessage, Collections.singletonList(cause));
}

public GlideException(String detailMessage, List<Exception> causes) {
public GlideException(String detailMessage, List<Throwable> causes) {
super(detailMessage);
setStackTrace(EMPTY_ELEMENTS);
this.causes = causes;
Expand Down Expand Up @@ -62,7 +62,7 @@ public Throwable fillInStackTrace() {
*
* @see #getRootCauses()
*/
public List<Exception> getCauses() {
public List<Throwable> getCauses() {
return causes;
}

Expand All @@ -74,8 +74,8 @@ public List<Exception> getCauses() {
* a given model using multiple different pathways, there may be multiple related or unrelated
* reasons for a load to fail.
*/
public List<Exception> getRootCauses() {
List<Exception> rootCauses = new ArrayList<>();
public List<Throwable> getRootCauses() {
List<Throwable> rootCauses = new ArrayList<>();
addRootCauses(this, rootCauses);
return rootCauses;
}
Expand All @@ -88,20 +88,20 @@ public List<Exception> getRootCauses() {
* complete stack traces.
*/
public void logRootCauses(String tag) {
List<Exception> causes = getRootCauses();
List<Throwable> causes = getRootCauses();
for (int i = 0, size = causes.size(); i < size; i++) {
Log.i(tag, "Root cause (" + (i + 1) + " of " + size + ")", causes.get(i));
}
}

private void addRootCauses(Exception exception, List<Exception> rootCauses) {
if (exception instanceof GlideException) {
GlideException glideException = (GlideException) exception;
for (Exception e : glideException.getCauses()) {
addRootCauses(e, rootCauses);
private void addRootCauses(Throwable throwable, List<Throwable> rootCauses) {
if (throwable instanceof GlideException) {
GlideException glideException = (GlideException) throwable;
for (Throwable t : glideException.getCauses()) {
addRootCauses(t, rootCauses);
}
} else {
rootCauses.add(exception);
rootCauses.add(throwable);
}
}

Expand Down Expand Up @@ -136,18 +136,18 @@ public String getMessage() {
// Appendable throws, PrintWriter, PrintStream, and IndentedAppendable do not, so this should
// never happen.
@SuppressWarnings("PMD.PreserveStackTrace")
private static void appendExceptionMessage(Exception e, Appendable appendable) {
private static void appendExceptionMessage(Throwable t, Appendable appendable) {
try {
appendable.append(e.getClass().toString()).append(": ").append(e.getMessage()).append('\n');
appendable.append(t.getClass().toString()).append(": ").append(t.getMessage()).append('\n');
} catch (IOException e1) {
throw new RuntimeException(e);
throw new RuntimeException(t);
}
}

// Appendable throws, PrintWriter, PrintStream, and IndentedAppendable do not, so this should
// never happen.
@SuppressWarnings("PMD.PreserveStackTrace")
private static void appendCauses(List<Exception> causes, Appendable appendable) {
private static void appendCauses(List<Throwable> causes, Appendable appendable) {
try {
appendCausesWrapped(causes, appendable);
} catch (IOException e) {
Expand All @@ -156,7 +156,7 @@ private static void appendCauses(List<Exception> causes, Appendable appendable)
}

@SuppressWarnings("ThrowableResultOfMethodCallIgnored")
private static void appendCausesWrapped(List<Exception> causes, Appendable appendable)
private static void appendCausesWrapped(List<Throwable> causes, Appendable appendable)
throws IOException {
int size = causes.size();
for (int i = 0; i < size; i++) {
Expand All @@ -166,7 +166,7 @@ private static void appendCausesWrapped(List<Exception> causes, Appendable appen
.append(String.valueOf(size))
.append("): ");

Exception cause = causes.get(i);
Throwable cause = causes.get(i);
if (cause instanceof GlideException) {
GlideException glideCause = (GlideException) cause;
glideCause.printStackTrace(appendable);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@
*/
public class LoadPath<Data, ResourceType, Transcode> {
private final Class<Data> dataClass;
private final Pool<List<Exception>> listPool;
private final Pool<List<Throwable>> listPool;
private final List<? extends DecodePath<Data, ResourceType, Transcode>> decodePaths;
private final String failureMessage;

public LoadPath(Class<Data> dataClass, Class<ResourceType> resourceClass,
Class<Transcode> transcodeClass,
List<DecodePath<Data, ResourceType, Transcode>> decodePaths, Pool<List<Exception>> listPool) {
List<DecodePath<Data, ResourceType, Transcode>> decodePaths, Pool<List<Throwable>> listPool) {
this.dataClass = dataClass;
this.listPool = listPool;
this.decodePaths = Preconditions.checkNotEmpty(decodePaths);
Expand All @@ -37,17 +37,17 @@ public LoadPath(Class<Data> dataClass, Class<ResourceType> resourceClass,

public Resource<Transcode> load(DataRewinder<Data> rewinder, Options options, int width,
int height, DecodePath.DecodeCallback<ResourceType> decodeCallback) throws GlideException {
List<Exception> exceptions = listPool.acquire();
List<Throwable> throwables = listPool.acquire();
try {
return loadWithExceptionList(rewinder, options, width, height, decodeCallback, exceptions);
return loadWithExceptionList(rewinder, options, width, height, decodeCallback, throwables);
} finally {
listPool.release(exceptions);
listPool.release(throwables);
}
}

private Resource<Transcode> loadWithExceptionList(DataRewinder<Data> rewinder, Options options,
int width, int height, DecodePath.DecodeCallback<ResourceType> decodeCallback,
List<Exception> exceptions) throws GlideException {
List<Throwable> exceptions) throws GlideException {
int size = decodePaths.size();
Resource<Transcode> result = null;
for (int i = 0; i < size; i++) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ public class ModelLoaderRegistry {
private final MultiModelLoaderFactory multiModelLoaderFactory;
private final ModelLoaderCache cache = new ModelLoaderCache();

public ModelLoaderRegistry(Pool<List<Exception>> exceptionListPool) {
this(new MultiModelLoaderFactory(exceptionListPool));
public ModelLoaderRegistry(Pool<List<Throwable>> throwableListPool) {
this(new MultiModelLoaderFactory(throwableListPool));
}

// Visible for testing.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@
class MultiModelLoader<Model, Data> implements ModelLoader<Model, Data> {

private final List<ModelLoader<Model, Data>> modelLoaders;
private final Pool<List<Exception>> exceptionListPool;
private final Pool<List<Throwable>> exceptionListPool;

MultiModelLoader(List<ModelLoader<Model, Data>> modelLoaders,
Pool<List<Exception>> exceptionListPool) {
Pool<List<Throwable>> exceptionListPool) {
this.modelLoaders = modelLoaders;
this.exceptionListPool = exceptionListPool;
}
Expand Down Expand Up @@ -74,15 +74,15 @@ public String toString() {
static class MultiFetcher<Data> implements DataFetcher<Data>, DataCallback<Data> {

private final List<DataFetcher<Data>> fetchers;
private final Pool<List<Exception>> exceptionListPool;
private final Pool<List<Throwable>> throwableListPool;
private int currentIndex;
private Priority priority;
private DataCallback<? super Data> callback;
@Nullable
private List<Exception> exceptions;
private List<Throwable> exceptions;

MultiFetcher(List<DataFetcher<Data>> fetchers, Pool<List<Exception>> exceptionListPool) {
this.exceptionListPool = exceptionListPool;
MultiFetcher(List<DataFetcher<Data>> fetchers, Pool<List<Throwable>> throwableListPool) {
this.throwableListPool = throwableListPool;
Preconditions.checkNotEmpty(fetchers);
this.fetchers = fetchers;
currentIndex = 0;
Expand All @@ -92,14 +92,14 @@ static class MultiFetcher<Data> implements DataFetcher<Data>, DataCallback<Data>
public void loadData(Priority priority, DataCallback<? super Data> callback) {
this.priority = priority;
this.callback = callback;
exceptions = exceptionListPool.acquire();
exceptions = throwableListPool.acquire();
fetchers.get(currentIndex).loadData(priority, this);
}

@Override
public void cleanup() {
if (exceptions != null) {
exceptionListPool.release(exceptions);
throwableListPool.release(exceptions);
}
exceptions = null;
for (DataFetcher<Data> fetcher : fetchers) {
Expand Down
Loading

0 comments on commit 74fcad1

Please sign in to comment.