Merge pull request #4866 from sjudd:error_request_coordinator_fixes

PiperOrigin-RevId: 467979387
bumptech · Aug 16, 2022 · 10bffe3 · 10bffe3
2 parents f714343 + 5c232dd
commit 10bffe3
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 18 deletions.
diff --git a/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java b/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java
@@ -102,7 +102,9 @@ public boolean isEquivalentTo(Request o) {
   @Override
   public boolean canSetImage(Request request) {
     synchronized (requestLock) {
-      return parentCanSetImage() && isValidRequest(request);
+      // Only one of primary or error runs at a time, so if we've reached this point and nothing
+      // else is broken, we should have nothing else to enforce.
+      return parentCanSetImage();
     }
   }
 
@@ -114,14 +116,14 @@ private boolean parentCanSetImage() {
   @Override
   public boolean canNotifyStatusChanged(Request request) {
     synchronized (requestLock) {
-      return parentCanNotifyStatusChanged() && isValidRequest(request);
+      return parentCanNotifyStatusChanged() && isValidRequestForStatusChanged(request);
     }
   }
 
   @Override
   public boolean canNotifyCleared(Request request) {
     synchronized (requestLock) {
-      return parentCanNotifyCleared() && isValidRequest(request);
+      return parentCanNotifyCleared() && request.equals(primary);
     }
   }
 
@@ -136,9 +138,17 @@ private boolean parentCanNotifyStatusChanged() {
   }
 
   @GuardedBy("requestLock")
-  private boolean isValidRequest(Request request) {
-    return request.equals(primary)
-        || (primaryState == RequestState.FAILED && request.equals(error));
+  private boolean isValidRequestForStatusChanged(Request request) {
+    if (primaryState != RequestState.FAILED) {
+      return request.equals(primary);
+    } else {
+      return request.equals(error)
+          // We don't want to call onLoadStarted once for the primary request and then again
+          // if it fails and the error request starts. It's already running, so we might as well
+          // avoid the duplicate notification by only notifying about the error state when it's
+          // final.
+          && (errorState == RequestState.SUCCESS || errorState == RequestState.FAILED);
+    }
   }
 
   @Override

diff --git a/library/test/src/test/java/com/bumptech/glide/request/ErrorRequestCoordinatorTest.java b/library/test/src/test/java/com/bumptech/glide/request/ErrorRequestCoordinatorTest.java
@@ -234,11 +234,6 @@ public void canSetImage_withNotFailedPrimary_andNullParent_returnsTrue() {
     assertThat(coordinator.canSetImage(primary)).isTrue();
   }
 
-  @Test
-  public void canSetImage_withError_andNullParent_andNotFailedPrimary_returnsFalse() {
-    assertThat(coordinator.canSetImage(error)).isFalse();
-  }
-
   @Test
   public void canSetImage_withNotFailedPrimary_parentCanSetImage_returnsTrue() {
     coordinator = newCoordinator(parent);
@@ -309,9 +304,19 @@ public void canNotifyStatusChanged_withError_notFailedPrimary_nullParent_returns
   }
 
   @Test
-  public void canNotifyStatusChanged_withError_failedPrimary_nullParent_returnsTrue() {
+  public void
+      canNotifyStatusChanged_withErrorRequest_failedPrimary_nullParent_errorIsNotFailed_returnsFalse() {
     coordinator.onRequestFailed(primary);
 
+    assertThat(coordinator.canNotifyStatusChanged(error)).isFalse();
+  }
+
+  @Test
+  public void
+      canNotifyStatusChanged_withErrorRequest_failedPrimary_nullParent_failedError_returnsTrue() {
+    coordinator.onRequestFailed(primary);
+    coordinator.onRequestFailed(error);
+
     assertThat(coordinator.canNotifyStatusChanged(error)).isTrue();
   }
 
@@ -325,13 +330,26 @@ public void canNotifyStatusChanged_withError_failedPrimary_nonNullParentCantNoti
   }
 
   @Test
-  public void canNotifyStatusChanged_withError_failedPrimary_nonNullParentCanNotify_returnsTrue() {
+  public void
+      canNotifyStatusChanged_withError_failedPrimary_notFailedError_nonNullParentCanNotify_returnsFalse() {
     coordinator = newCoordinator(parent);
     coordinator.setRequests(primary, error);
     coordinator.onRequestFailed(primary);
     when(parent.canNotifyStatusChanged(coordinator)).thenReturn(true);
 
-    assertThat(coordinator.canNotifyStatusChanged(primary)).isTrue();
+    assertThat(coordinator.canNotifyStatusChanged(error)).isFalse();
+  }
+
+  @Test
+  public void
+      canNotifyStatusChanged_withError_failedPrimary_failedError_nonNullParentCanNotify_returnsTrue() {
+    coordinator = newCoordinator(parent);
+    coordinator.setRequests(primary, error);
+    coordinator.onRequestFailed(primary);
+    when(parent.canNotifyStatusChanged(coordinator)).thenReturn(true);
+    coordinator.onRequestFailed(error);
+
+    assertThat(coordinator.canNotifyStatusChanged(error)).isTrue();
   }
 
   @Test
@@ -532,9 +550,20 @@ public void canNotifyCleared_errorRequest_nullParent_returnsFalse() {
   }
 
   @Test
-  public void canNotifyCleared_errorRequest_primaryFailed_nullParent_returnsTrue() {
+  public void canNotifyCleared_errorRequest_primaryFailed_nullParent_returnsFalse() {
     coordinator.onRequestFailed(primary);
-    assertThat(coordinator.canNotifyCleared(error)).isTrue();
+    assertThat(coordinator.canNotifyCleared(error)).isFalse();
+  }
+
+  @Test
+  public void
+      canNotifyCleared_primaryRequest_primaryFailed_nonNullParentCanNotNotify_returnsFalse() {
+    coordinator = newCoordinator(parent);
+    coordinator.setRequests(primary, error);
+    when(parent.canNotifyCleared(coordinator)).thenReturn(false);
+    coordinator.onRequestFailed(primary);
+
+    assertThat(coordinator.canNotifyCleared(primary)).isFalse();
   }
 
   @Test
@@ -548,13 +577,23 @@ public void canNotifyCleared_errorRequest_primaryFailed_nonNullParentCanNotNotif
   }
 
   @Test
-  public void canNotifyCleared_errorRequest_primaryFailed_nonNullParentCanNotify_returnsTrue() {
+  public void canNotifyCleared_errorRequest_primaryFailed_nonNullParentCanNotify_returnsFalse() {
     coordinator = newCoordinator(parent);
     coordinator.setRequests(primary, error);
     when(parent.canNotifyCleared(coordinator)).thenReturn(true);
     coordinator.onRequestFailed(primary);
 
-    assertThat(coordinator.canNotifyCleared(error)).isTrue();
+    assertThat(coordinator.canNotifyCleared(error)).isFalse();
+  }
+
+  @Test
+  public void canNotifyCleared_primaryRequest_primaryFailed_nonNullParentCanNotify_returnsTrue() {
+    coordinator = newCoordinator(parent);
+    coordinator.setRequests(primary, error);
+    when(parent.canNotifyCleared(coordinator)).thenReturn(true);
+    coordinator.onRequestFailed(primary);
+
+    assertThat(coordinator.canNotifyCleared(primary)).isTrue();
   }
 
   private static ErrorRequestCoordinator newCoordinator() {