Assert that into() and clear() are not called in callbacks.

Fixes #2413.
bumptech · Sep 22, 2017 · 6fb87b3 · 6fb87b3
1 parent 1b4a99a
commit 6fb87b3
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 9 deletions.
diff --git a/library/src/main/java/com/bumptech/glide/request/SingleRequest.java b/library/src/main/java/com/bumptech/glide/request/SingleRequest.java
@@ -45,6 +45,7 @@ public SingleRequest<?> create() {
           return new SingleRequest<Object>();
         }
       });
+  private boolean isCallingCallbacks;
 
   private enum Status {
     /**
@@ -182,6 +183,7 @@ public StateVerifier getVerifier() {
 
   @Override
   public void recycle() {
+    assertNotCallingCallbacks();
     glideContext = null;
     model = null;
     transcodeClass = null;
@@ -203,6 +205,7 @@ public void recycle() {
 
   @Override
   public void begin() {
+    assertNotCallingCallbacks();
     stateVerifier.throwIfRecycled();
     startTime = LogTime.getLogTime();
     if (model == null) {
@@ -260,6 +263,7 @@ && canNotifyStatusChanged()) {
    * @see #clear()
    */
   void cancel() {
+    assertNotCallingCallbacks();
     stateVerifier.throwIfRecycled();
     target.removeCallback(this);
     status = Status.CANCELLED;
@@ -269,6 +273,15 @@ void cancel() {
     }
   }
 
+  // Avoids difficult to understand errors like #2413.
+  private void assertNotCallingCallbacks() {
+    if (isCallingCallbacks) {
+      throw new IllegalStateException("You can't start or clear loads in RequestListener or"
+          + " Target callbacks. If you must do so, consider posting your into() or clear() calls"
+          + " to the main thread using a Handler instead.");
+    }
+  }
+
   /**
    * Cancels the current load if it is in progress, clears any resources held onto by the request
    * and replaces the loaded resource if the load completed with the placeholder.
@@ -280,6 +293,7 @@ void cancel() {
   @Override
   public void clear() {
     Util.assertMainThread();
+    assertNotCallingCallbacks();
     if (status == Status.CLEARED) {
       return;
     }
@@ -535,11 +549,16 @@ private void onResourceReady(Resource<R> resource, R result, DataSource dataSour
           + LogTime.getElapsedMillis(startTime) + " ms");
     }
 
-    if (requestListener == null
-        || !requestListener.onResourceReady(result, model, target, dataSource, isFirstResource)) {
-      Transition<? super R> animation =
-          animationFactory.build(dataSource, isFirstResource);
-      target.onResourceReady(result, animation);
+    isCallingCallbacks = true;
+    try {
+      if (requestListener == null
+          || !requestListener.onResourceReady(result, model, target, dataSource, isFirstResource)) {
+        Transition<? super R> animation =
+            animationFactory.build(dataSource, isFirstResource);
+        target.onResourceReady(result, animation);
+      }
+    } finally {
+      isCallingCallbacks = false;
     }
 
     notifyLoadSuccess();
@@ -565,10 +584,16 @@ private void onLoadFailed(GlideException e, int maxLogLevel) {
 
     loadStatus = null;
     status = Status.FAILED;
-    //TODO: what if this is a thumbnail request?
-    if (requestListener == null
-        || !requestListener.onLoadFailed(e, model, target, isFirstReadyResource())) {
-      setErrorPlaceholder();
+
+    isCallingCallbacks = true;
+    try {
+      //TODO: what if this is a thumbnail request?
+      if (requestListener == null
+          || !requestListener.onLoadFailed(e, model, target, isFirstReadyResource())) {
+        setErrorPlaceholder();
+      }
+    } finally {
+      isCallingCallbacks = false;
     }
   }
 

diff --git a/library/src/main/java/com/bumptech/glide/request/target/PreloadTarget.java b/library/src/main/java/com/bumptech/glide/request/target/PreloadTarget.java
@@ -1,5 +1,9 @@
 package com.bumptech.glide.request.target;
 
+import android.os.Handler;
+import android.os.Handler.Callback;
+import android.os.Looper;
+import android.os.Message;
 import com.bumptech.glide.RequestManager;
 import com.bumptech.glide.request.transition.Transition;
 
@@ -10,6 +14,17 @@
  * @param <Z> The type of resource that will be loaded into memory.
  */
 public final class PreloadTarget<Z> extends SimpleTarget<Z> {
+  private static final int MESSAGE_CLEAR = 1;
+  private static final Handler HANDLER = new Handler(Looper.getMainLooper(), new Callback() {
+    @Override
+    public boolean handleMessage(Message message) {
+      if (message.what == MESSAGE_CLEAR) {
+        ((PreloadTarget<?>) message.obj).clear();
+        return true;
+      }
+      return false;
+    }
+  });
 
   private final RequestManager requestManager;
 
@@ -31,6 +46,10 @@ private PreloadTarget(RequestManager requestManager, int width, int height) {
 
   @Override
   public void onResourceReady(Z resource, Transition<? super Z> transition) {
+    HANDLER.obtainMessage(MESSAGE_CLEAR, this).sendToTarget();
+  }
+
+  private void clear() {
     requestManager.clear(this);
   }
 }