Notify RequestCoordinator before Targets/Listeners when loads finish.

This allows callers (ideally only Glide's framework) to reason about the state of the load based on the Request object. Prior to this change, the Request object is not updated for loads involving thumbnails or error request builders until after the Target/RequestListener is called. When RequestListeners/Targets check the state of the Request, it still has the previous state that doesn't match the callback the RequestListener/Target just received.

PiperOrigin-RevId: 468013881

instrumentation/src/androidTest/java/com/bumptech/glide/MultiRequestTest.java

@@ -0,0 +1,163 @@
+package com.bumptech.glide;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import android.content.Context;
+import android.graphics.Bitmap;
+import android.graphics.Bitmap.CompressFormat;
+import android.graphics.Bitmap.Config;
+import android.graphics.Canvas;
+import android.graphics.Color;
+import android.graphics.drawable.Drawable;
+import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
+import androidx.test.core.app.ApplicationProvider;
+import androidx.test.ext.junit.runners.AndroidJUnit4;
+import com.bumptech.glide.load.DataSource;
+import com.bumptech.glide.load.engine.GlideException;
+import com.bumptech.glide.load.engine.executor.GlideExecutor;
+import com.bumptech.glide.request.Request;
+import com.bumptech.glide.request.RequestListener;
+import com.bumptech.glide.request.target.CustomTarget;
+import com.bumptech.glide.request.target.Target;
+import com.bumptech.glide.request.transition.Transition;
+import com.bumptech.glide.test.ModelGeneratorRule;
+import com.bumptech.glide.testutil.ConcurrencyHelper;
+import com.bumptech.glide.testutil.TearDownGlide;
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+
+@RunWith(AndroidJUnit4.class)
+public class MultiRequestTest {
+  private final Context context = ApplicationProvider.getApplicationContext();
+  @Rule public final TearDownGlide tearDownGlide = new TearDownGlide();
+  @Rule public final ModelGeneratorRule modelGeneratorRule = new ModelGeneratorRule();
+  @Rule public final TemporaryFolder temporaryFolder = new TemporaryFolder();
+  private final ConcurrencyHelper concurrency = new ConcurrencyHelper();
+
+  @Test
+  public void thumbnail_onResourceReady_forPrimary_isComplete_whenRequestListenerIsCalled()
+      throws IOException, InterruptedException {
+
+    // Make sure the requests complete in the same order
+    Glide.init(
+        context,
+        new GlideBuilder()
+            .setSourceExecutor(GlideExecutor.newSourceBuilder().setThreadCount(1).build()));
+
+    AtomicBoolean isPrimaryRequestComplete = new AtomicBoolean(false);
+    CountDownLatch countDownLatch = new CountDownLatch(1);
+
+    RequestBuilder<Drawable> request =
+        Glide.with(context)
+            .load(newImageFile())
+            .thumbnail(Glide.with(context).load(newImageFile()))
+            .listener(
+                new RequestListener<>() {
+                  @Override
+                  public boolean onLoadFailed(
+                      @Nullable GlideException e,
+                      Object model,
+                      Target<Drawable> target,
+                      boolean isFirstResource) {
+                    return false;
+                  }
+
+                  @Override
+                  public boolean onResourceReady(
+                      Drawable resource,
+                      Object model,
+                      Target<Drawable> target,
+                      DataSource dataSource,
+                      boolean isFirstResource) {
+                    isPrimaryRequestComplete.set(target.getRequest().isComplete());
+                    countDownLatch.countDown();
+                    return false;
+                  }
+                });
+    concurrency.runOnMainThread(() -> request.into(new DoNothingTarget()));
+
+    assertThat(countDownLatch.await(3, TimeUnit.SECONDS)).isTrue();
+    assertThat(isPrimaryRequestComplete.get()).isTrue();
+  }
+
+  @Test
+  public void thumbnail_onLoadFailed_forPrimary_isNotRunningOrComplete_whenRequestListenerIsCalled()
+      throws IOException, InterruptedException {
+
+    // Make sure the requests complete in the same order
+    Glide.init(
+        context,
+        new GlideBuilder()
+            .setSourceExecutor(GlideExecutor.newSourceBuilder().setThreadCount(1).build()));
+
+    AtomicBoolean isNeitherRunningNorComplete = new AtomicBoolean(false);
+    CountDownLatch countDownLatch = new CountDownLatch(1);
+
+    int missingResourceId = 123;
+    RequestBuilder<Drawable> requestBuilder =
+        Glide.with(context)
+            .load(missingResourceId)
+            .thumbnail(Glide.with(context).load(newImageFile()))
+            .listener(
+                new RequestListener<>() {
+                  @Override
+                  public boolean onLoadFailed(
+                      @Nullable GlideException e,
+                      Object model,
+                      Target<Drawable> target,
+                      boolean isFirstResource) {
+                    Request request = target.getRequest();
+                    isNeitherRunningNorComplete.set(!request.isComplete() && !request.isRunning());
+                    countDownLatch.countDown();
+                    return false;
+                  }
+
+                  @Override
+                  public boolean onResourceReady(
+                      Drawable resource,
+                      Object model,
+                      Target<Drawable> target,
+                      DataSource dataSource,
+                      boolean isFirstResource) {
+                    return false;
+                  }
+                });
+    concurrency.runOnMainThread(() -> requestBuilder.into(new DoNothingTarget()));
+
+    assertThat(countDownLatch.await(3, TimeUnit.SECONDS)).isTrue();
+    assertThat(isNeitherRunningNorComplete.get()).isTrue();
+  }
+
+  private File newImageFile() throws IOException {
+    Bitmap bitmap = Bitmap.createBitmap(100, 100, Config.ARGB_8888);
+    Canvas canvas = new Canvas(bitmap);
+    canvas.drawColor(Color.RED);
+    File result = temporaryFolder.newFile();
+    try (OutputStream os = new BufferedOutputStream(new FileOutputStream(result))) {
+      bitmap.compress(CompressFormat.JPEG, 75, os);
+    }
+    return result;
+  }
+
+  // We don't store or do anything with the resource, so we don't need to do anything to release it
+  // in onLoadCleared.
+  private static final class DoNothingTarget extends CustomTarget<Drawable> {
+    @Override
+    public void onResourceReady(
+        @NonNull Drawable resource, @Nullable Transition<? super Drawable> transition) {}
+
+    @Override
+    public void onLoadCleared(@Nullable Drawable placeholder) {}
+  }
+}

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

@@ -523,14 +523,14 @@ private boolean isFirstReadyResource() {
   }
 
   @GuardedBy("requestLock")
-  private void notifyLoadSuccess() {
+  private void notifyRequestCoordinatorLoadSucceeded() {
     if (requestCoordinator != null) {
       requestCoordinator.onRequestSuccess(this);
     }
   }
 
   @GuardedBy("requestLock")
-  private void notifyLoadFailed() {
+  private void notifyRequestCoordinatorLoadFailed() {
     if (requestCoordinator != null) {
       requestCoordinator.onRequestFailed(this);
     }
@@ -639,6 +639,8 @@ private void onResourceReady(
               + " ms");
     }
 
+    notifyRequestCoordinatorLoadSucceeded();
+
     isCallingCallbacks = true;
     try {
       boolean anyListenerHandledUpdatingTarget = false;
@@ -660,7 +662,6 @@ private void onResourceReady(
       isCallingCallbacks = false;
     }
 
-    notifyLoadSuccess();
     GlideTrace.endSectionAsync(TAG, cookie);
   }
 
@@ -694,6 +695,8 @@ private void onLoadFailed(GlideException e, int maxLogLevel) {
       loadStatus = null;
       status = Status.FAILED;
 
+      notifyRequestCoordinatorLoadFailed();
+
       isCallingCallbacks = true;
       try {
         // TODO: what if this is a thumbnail request?
@@ -715,7 +718,6 @@ private void onLoadFailed(GlideException e, int maxLogLevel) {
         isCallingCallbacks = false;
       }
 
-      notifyLoadFailed();
       GlideTrace.endSectionAsync(TAG, cookie);
     }
   }

library/test/src/test/java/com/bumptech/glide/request/SingleRequestTest.java

@@ -33,6 +33,7 @@
 import com.bumptech.glide.load.engine.Engine;
 import com.bumptech.glide.load.engine.GlideException;
 import com.bumptech.glide.load.engine.Resource;
+import com.bumptech.glide.request.target.CustomTarget;
 import com.bumptech.glide.request.target.SizeReadyCallback;
 import com.bumptech.glide.request.target.Target;
 import com.bumptech.glide.request.transition.Transition;
@@ -46,6 +47,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Executor;
+import java.util.concurrent.atomic.AtomicBoolean;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -649,6 +651,89 @@ public void testRequestListenerIsCalledWithFirstImageIfRequestCoordinatorReturns
             eq(builder.result), any(Number.class), isAListTarget(), isADataSource(), eq(false));
   }
 
+  @Test
+  public void onResourceReady_notifiesRequestCoordinator_beforeCallingRequestListeners() {
+    AtomicBoolean isRequestCoordinatorVerified = new AtomicBoolean();
+    SingleRequest<List> request =
+        builder
+            .setTarget(new DoNothingTarget())
+            .addRequestListener(
+                new RequestListener<>() {
+                  @Override
+                  public boolean onLoadFailed(
+                      @Nullable GlideException e,
+                      Object model,
+                      Target<List> target,
+                      boolean isFirstResource) {
+                    return false;
+                  }
+
+                  @Override
+                  public boolean onResourceReady(
+                      List resource,
+                      Object model,
+                      Target<List> target,
+                      DataSource dataSource,
+                      boolean isFirstResource) {
+                    verify(builder.requestCoordinator).onRequestSuccess(target.getRequest());
+                    isRequestCoordinatorVerified.set(true);
+                    return false;
+                  }
+                })
+            .build();
+    builder.target.setRequest(request);
+    request.onResourceReady(
+        builder.resource, DataSource.DATA_DISK_CACHE, /* isLoadedFromAlternateCacheKey= */ false);
+
+    assertThat(isRequestCoordinatorVerified.get()).isTrue();
+  }
+
+  @Test
+  public void onLoadFailed_notifiesRequestCoordinator_beforeCallingRequestListeners() {
+    AtomicBoolean isRequestCoordinatorVerified = new AtomicBoolean();
+    SingleRequest<List> request =
+        builder
+            .setTarget(new DoNothingTarget())
+            .addRequestListener(
+                new RequestListener<>() {
+                  @Override
+                  public boolean onLoadFailed(
+                      @Nullable GlideException e,
+                      Object model,
+                      Target<List> target,
+                      boolean isFirstResource) {
+                    verify(builder.requestCoordinator).onRequestFailed(target.getRequest());
+                    isRequestCoordinatorVerified.set(true);
+                    return false;
+                  }
+
+                  @Override
+                  public boolean onResourceReady(
+                      List resource,
+                      Object model,
+                      Target<List> target,
+                      DataSource dataSource,
+                      boolean isFirstResource) {
+                    return false;
+                  }
+                })
+            .build();
+    builder.target.setRequest(request);
+    request.onLoadFailed(new GlideException("test"));
+
+    assertThat(isRequestCoordinatorVerified.get()).isTrue();
+  }
+
+  // We don't need to clear a resource since we're not using it to being with.
+  private static final class DoNothingTarget extends CustomTarget<List> {
+    @Override
+    public void onResourceReady(
+        @NonNull List resource, @Nullable Transition<? super List> transition) {}
+
+    @Override
+    public void onLoadCleared(@Nullable Drawable placeholder) {}
+  }
+
   @Test
   public void
       testRequestListenerIsCalledWithNotIsFirstRequestIfRequestCoordinatorParentReturnsResourceSet() {