Use single lock for all error/thumbnail requests in a single request …

…chain.

The single lock avoids deadlock that could previously have been caused by acquiring the lock for one individual request and then the lock for another individual request and vice versa.

I've also cleaned up some unecessary methods in Request and decreased the reliance on Request states in the various coordinators.

PiperOrigin-RevId: 261714443

instrumentation/src/androidTest/java/com/bumptech/glide/test/ConcurrencyHelper.java

@@ -16,6 +16,7 @@
 import com.bumptech.glide.request.target.SizeReadyCallback;
 import com.bumptech.glide.request.target.Target;
 import com.bumptech.glide.request.transition.Transition;
+import com.bumptech.glide.util.Executors;
 import com.bumptech.glide.util.Preconditions;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CountDownLatch;
@@ -197,9 +198,7 @@ public void onDestroy() {
                   public void onResourceReady(
                       @NonNull T resource, @Nullable Transition<? super T> transition) {
                     target.onResourceReady(resource, transition);
-                    if (!Preconditions.checkNotNull(getRequest()).isRunning()) {
-                      latch.countDown();
-                    }
+                    checkRequestAndMaybeReleaseLatch();
                   }
 
                   @Override
@@ -215,9 +214,7 @@ public void onLoadStarted(@Nullable Drawable placeholder) {
                   @Override
                   public void onLoadFailed(@Nullable Drawable errorDrawable) {
                     target.onLoadFailed(errorDrawable);
-                    if (!Preconditions.checkNotNull(getRequest()).isRunning()) {
-                      latch.countDown();
-                    }
+                    checkRequestAndMaybeReleaseLatch();
                   }
 
                   @Override
@@ -240,6 +237,22 @@ public void setRequest(@Nullable Request request) {
                   public Request getRequest() {
                     return target.getRequest();
                   }
+
+                  // We can't guarantee the ordering of when this callback is called and when the
+                  // request's state is updated, so it's safer to post the check back to the UI
+                  // thread.
+                  private void checkRequestAndMaybeReleaseLatch() {
+                    Executors.mainThreadExecutor()
+                        .execute(
+                            new Runnable() {
+                              @Override
+                              public void run() {
+                                if (!Preconditions.checkNotNull(getRequest()).isRunning()) {
+                                  latch.countDown();
+                                }
+                              }
+                            });
+                  }
                 });
             return target;
           }

library/src/main/java/com/bumptech/glide/RequestBuilder.java

@@ -849,6 +849,7 @@ private Request buildRequest(
       BaseRequestOptions<?> requestOptions,
       Executor callbackExecutor) {
     return buildRequestRecursive(
+        /*requestLock=*/ new Object(),
         target,
         targetListener,
         /*parentCoordinator=*/ null,
@@ -861,6 +862,7 @@ private Request buildRequest(
   }
 
   private Request buildRequestRecursive(
+      Object requestLock,
       Target<TranscodeType> target,
       @Nullable RequestListener<TranscodeType> targetListener,
       @Nullable RequestCoordinator parentCoordinator,
@@ -874,12 +876,13 @@ private Request buildRequestRecursive(
     // Build the ErrorRequestCoordinator first if necessary so we can update parentCoordinator.
     ErrorRequestCoordinator errorRequestCoordinator = null;
     if (errorBuilder != null) {
-      errorRequestCoordinator = new ErrorRequestCoordinator(parentCoordinator);
+      errorRequestCoordinator = new ErrorRequestCoordinator(requestLock, parentCoordinator);
       parentCoordinator = errorRequestCoordinator;
     }
 
     Request mainRequest =
         buildThumbnailRequestRecursive(
+            requestLock,
             target,
             targetListener,
             parentCoordinator,
@@ -903,6 +906,7 @@ private Request buildRequestRecursive(
 
     Request errorRequest =
         errorBuilder.buildRequestRecursive(
+            requestLock,
             target,
             targetListener,
             errorRequestCoordinator,
@@ -917,6 +921,7 @@ private Request buildRequestRecursive(
   }
 
   private Request buildThumbnailRequestRecursive(
+      Object requestLock,
       Target<TranscodeType> target,
       RequestListener<TranscodeType> targetListener,
       @Nullable RequestCoordinator parentCoordinator,
@@ -956,9 +961,11 @@ private Request buildThumbnailRequestRecursive(
         thumbOverrideHeight = requestOptions.getOverrideHeight();
       }
 
-      ThumbnailRequestCoordinator coordinator = new ThumbnailRequestCoordinator(parentCoordinator);
+      ThumbnailRequestCoordinator coordinator =
+          new ThumbnailRequestCoordinator(requestLock, parentCoordinator);
       Request fullRequest =
           obtainRequest(
+              requestLock,
               target,
               targetListener,
               requestOptions,
@@ -972,6 +979,7 @@ private Request buildThumbnailRequestRecursive(
       // Recursively generate thumbnail requests.
       Request thumbRequest =
           thumbnailBuilder.buildRequestRecursive(
+              requestLock,
               target,
               targetListener,
               coordinator,
@@ -986,9 +994,11 @@ private Request buildThumbnailRequestRecursive(
       return coordinator;
     } else if (thumbSizeMultiplier != null) {
       // Base case: thumbnail multiplier generates a thumbnail request, but cannot recurse.
-      ThumbnailRequestCoordinator coordinator = new ThumbnailRequestCoordinator(parentCoordinator);
+      ThumbnailRequestCoordinator coordinator =
+          new ThumbnailRequestCoordinator(requestLock, parentCoordinator);
       Request fullRequest =
           obtainRequest(
+              requestLock,
               target,
               targetListener,
               requestOptions,
@@ -1003,6 +1013,7 @@ private Request buildThumbnailRequestRecursive(
 
       Request thumbnailRequest =
           obtainRequest(
+              requestLock,
               target,
               targetListener,
               thumbnailOptions,
@@ -1018,6 +1029,7 @@ private Request buildThumbnailRequestRecursive(
     } else {
       // Base case: no thumbnail.
       return obtainRequest(
+          requestLock,
           target,
           targetListener,
           requestOptions,
@@ -1031,6 +1043,7 @@ private Request buildThumbnailRequestRecursive(
   }
 
   private Request obtainRequest(
+      Object requestLock,
       Target<TranscodeType> target,
       RequestListener<TranscodeType> targetListener,
       BaseRequestOptions<?> requestOptions,
@@ -1043,6 +1056,7 @@ private Request obtainRequest(
     return SingleRequest.obtain(
         context,
         glideContext,
+        requestLock,
         model,
         transcodeClass,
         requestOptions,

library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java

@@ -1,5 +1,6 @@
 package com.bumptech.glide.request;
 
+import androidx.annotation.GuardedBy;
 import androidx.annotation.Nullable;
 
 /**
@@ -8,11 +9,20 @@
  */
 public final class ErrorRequestCoordinator implements RequestCoordinator, Request {
 
+  private final Object requestLock;
   @Nullable private final RequestCoordinator parent;
-  private Request primary;
-  private Request error;
 
-  public ErrorRequestCoordinator(@Nullable RequestCoordinator parent) {
+  private volatile Request primary;
+  private volatile Request error;
+
+  @GuardedBy("requestLock")
+  private RequestState primaryState = RequestState.CLEARED;
+
+  @GuardedBy("requestLock")
+  private RequestState errorState = RequestState.CLEARED;
+
+  public ErrorRequestCoordinator(Object requestLock, @Nullable RequestCoordinator parent) {
+    this.requestLock = requestLock;
     this.parent = parent;
   }
 
@@ -23,56 +33,69 @@ public void setRequests(Request primary, Request error) {
 
   @Override
   public void begin() {
-    if (!primary.isRunning()) {
-      primary.begin();
+    synchronized (requestLock) {
+      if (primaryState != RequestState.RUNNING) {
+        primaryState = RequestState.RUNNING;
+        primary.begin();
+      }
     }
   }
 
   @Override
   public void clear() {
-    primary.clear();
-    // Don't check primary.isFailed() here because it will have been reset by the clear call
-    // immediately before this.
-    if (error.isRunning()) {
-      error.clear();
+    synchronized (requestLock) {
+      primaryState = RequestState.CLEARED;
+      primary.clear();
+      // Don't check primary's failed state here because it will have been reset by the clear call
+      // immediately before this.
+      if (errorState != RequestState.CLEARED) {
+        errorState = RequestState.CLEARED;
+        error.clear();
+      }
     }
   }
 
   @Override
   public void pause() {
-    primary.pause();
-    error.pause();
+    synchronized (requestLock) {
+      if (primaryState == RequestState.RUNNING) {
+        primaryState = RequestState.PAUSED;
+        primary.pause();
+      }
+      if (errorState == RequestState.RUNNING) {
+        errorState = RequestState.PAUSED;
+        error.pause();
+      }
+    }
   }
 
   @Override
   public boolean isRunning() {
-    return primary.isFailed() ? error.isRunning() : primary.isRunning();
+    synchronized (requestLock) {
+      return primaryState == RequestState.RUNNING || errorState == RequestState.RUNNING;
+    }
   }
 
   @Override
   public boolean isComplete() {
-    return primary.isFailed() ? error.isComplete() : primary.isComplete();
-  }
-
-  @Override
-  public boolean isResourceSet() {
-    return primary.isFailed() ? error.isResourceSet() : primary.isResourceSet();
+    synchronized (requestLock) {
+      return primaryState == RequestState.SUCCESS || errorState == RequestState.SUCCESS;
+    }
   }
 
   @Override
   public boolean isCleared() {
-    return primary.isFailed() ? error.isCleared() : primary.isCleared();
-  }
-
-  @Override
-  public boolean isFailed() {
-    return primary.isFailed() && error.isFailed();
+    synchronized (requestLock) {
+      return primaryState == RequestState.CLEARED && errorState == RequestState.CLEARED;
+    }
   }
 
   @Override
   public void recycle() {
-    primary.recycle();
-    error.recycle();
+    synchronized (requestLock) {
+      primary.recycle();
+      error.recycle();
+    }
   }
 
   @Override
@@ -86,62 +109,89 @@ public boolean isEquivalentTo(Request o) {
 
   @Override
   public boolean canSetImage(Request request) {
-    return parentCanSetImage() && isValidRequest(request);
+    synchronized (requestLock) {
+      return parentCanSetImage() && isValidRequest(request);
+    }
   }
 
+  @GuardedBy("requestLock")
   private boolean parentCanSetImage() {
     return parent == null || parent.canSetImage(this);
   }
 
   @Override
   public boolean canNotifyStatusChanged(Request request) {
-    return parentCanNotifyStatusChanged() && isValidRequest(request);
+    synchronized (requestLock) {
+      return parentCanNotifyStatusChanged() && isValidRequest(request);
+    }
   }
 
   @Override
   public boolean canNotifyCleared(Request request) {
-    return parentCanNotifyCleared() && isValidRequest(request);
+    synchronized (requestLock) {
+      return parentCanNotifyCleared() && isValidRequest(request);
+    }
   }
 
+  @GuardedBy("requestLock")
   private boolean parentCanNotifyCleared() {
     return parent == null || parent.canNotifyCleared(this);
   }
 
+  @GuardedBy("requestLock")
   private boolean parentCanNotifyStatusChanged() {
     return parent == null || parent.canNotifyStatusChanged(this);
   }
 
+  @GuardedBy("requestLock")
   private boolean isValidRequest(Request request) {
-    return request.equals(primary) || (primary.isFailed() && request.equals(error));
+    return request.equals(primary)
+        || (primaryState == RequestState.FAILED && request.equals(error));
   }
 
   @Override
   public boolean isAnyResourceSet() {
-    return parentIsAnyResourceSet() || isResourceSet();
+    synchronized (requestLock) {
+      return parentIsAnyResourceSet() || isComplete();
+    }
   }
 
+  @GuardedBy("requestLock")
   private boolean parentIsAnyResourceSet() {
     return parent != null && parent.isAnyResourceSet();
   }
 
   @Override
   public void onRequestSuccess(Request request) {
-    if (parent != null) {
-      parent.onRequestSuccess(this);
+    synchronized (requestLock) {
+      if (request.equals(primary)) {
+        primaryState = RequestState.SUCCESS;
+      } else if (request.equals(error)) {
+        errorState = RequestState.SUCCESS;
+      }
+      if (parent != null) {
+        parent.onRequestSuccess(this);
+      }
     }
   }
 
   @Override
   public void onRequestFailed(Request request) {
-    if (!request.equals(error)) {
-      if (!error.isRunning()) {
-        error.begin();
+    synchronized (requestLock) {
+      if (!request.equals(error)) {
+        primaryState = RequestState.FAILED;
+        if (errorState != RequestState.RUNNING) {
+          errorState = RequestState.RUNNING;
+          error.begin();
+        }
+        return;
       }
-      return;
-    }
 
-    if (parent != null) {
-      parent.onRequestFailed(this);
+      errorState = RequestState.FAILED;
+
+      if (parent != null) {
+        parent.onRequestFailed(this);
+      }
     }
   }
 }