Make sure EngineJob uses the Request's shared lock.

PiperOrigin-RevId: 263201451

Author: sjudd

library/src/main/java/com/bumptech/glide/load/engine/EngineJob.java

@@ -383,16 +383,20 @@ public StateVerifier getVerifier() {
   private class CallLoadFailed implements Runnable {
 
     private final ResourceCallback cb;
+    private final Object lock;
 
     CallLoadFailed(ResourceCallback cb) {
       this.cb = cb;
+      // The lock may be reset if the callback is removed while our Runnable is pending, so memoize
+      // it here. We're assuming that the lock is never re-used.
+      this.lock = Preconditions.checkNotNull(cb.getLock());
     }
 
     @Override
     public void run() {
       // Make sure we always acquire the request lock, then the EngineJob lock to avoid deadlock
       // (b/136032534).
-      synchronized (cb) {
+      synchronized (lock) {
         synchronized (EngineJob.this) {
           if (cbs.contains(cb)) {
             callCallbackOnLoadFailed(cb);
@@ -407,16 +411,20 @@ public void run() {
   private class CallResourceReady implements Runnable {
 
     private final ResourceCallback cb;
+    private final Object lock;
 
     CallResourceReady(ResourceCallback cb) {
       this.cb = cb;
+      // The lock may be reset if the callback is removed while our Runnable is pending, so memoize
+      // it here. We're assuming that the lock is never re-used.
+      this.lock = Preconditions.checkNotNull(cb.getLock());
     }
 
     @Override
     public void run() {
       // Make sure we always acquire the request lock, then the EngineJob lock to avoid deadlock
       // (b/136032534).
-      synchronized (cb) {
+      synchronized (lock) {
         synchronized (EngineJob.this) {
           if (cbs.contains(cb)) {
             // Acquire for this particular callback.

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

@@ -23,4 +23,7 @@ public interface ResourceCallback {
    * @param e a non-null {@link GlideException}.
    */
   void onLoadFailed(GlideException e);
+
+  /** Returns the lock to use when notifying individual requests. */
+  Object getLock();
 }

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

@@ -21,6 +21,7 @@
 import com.bumptech.glide.request.transition.Transition;
 import com.bumptech.glide.request.transition.TransitionFactory;
 import com.bumptech.glide.util.LogTime;
+import com.bumptech.glide.util.Preconditions;
 import com.bumptech.glide.util.Synthetic;
 import com.bumptech.glide.util.Util;
 import com.bumptech.glide.util.pool.FactoryPools;
@@ -229,7 +230,7 @@ private void init(
       Engine engine,
       TransitionFactory<? super R> animationFactory,
       Executor callbackExecutor) {
-    this.requestLock = requestLock;
+    this.requestLock = Preconditions.checkNotNull(requestLock);
     synchronized (this.requestLock) {
       this.context = context;
       this.glideContext = glideContext;
@@ -716,6 +717,12 @@ public void onLoadFailed(GlideException e) {
     onLoadFailed(e, Log.WARN);
   }
 
+  @Override
+  public Object getLock() {
+    stateVerifier.throwIfRecycled();
+    return requestLock;
+  }
+
   private void onLoadFailed(GlideException e, int maxLogLevel) {
     stateVerifier.throwIfRecycled();
     synchronized (requestLock) {

library/test/src/test/java/com/bumptech/glide/load/engine/EngineJobTest.java

@@ -188,7 +188,7 @@ public void testRemovingAllCallbacksCancelsRunner() {
   @Test
   public void removingSomeCallbacksDoesNotCancelRunner() {
     EngineJob<Object> job = harness.getJob();
-    job.addCallback(mock(ResourceCallback.class), Executors.directExecutor());
+    job.addCallback(mockResourceCallback(), Executors.directExecutor());
     job.removeCallback(harness.cb);
 
     assertFalse(job.isCancelled());
@@ -261,8 +261,8 @@ public void testDoesNotAcquireOnceForMemoryCacheIfNotCacheable() {
   @Test
   public void testNotifiesNewCallbackOfResourceIfCallbackIsAddedDuringOnResourceReady() {
     final EngineJob<Object> job = harness.getJob();
-    final ResourceCallback existingCallback = mock(ResourceCallback.class);
-    final ResourceCallback newCallback = mock(ResourceCallback.class);
+    final ResourceCallback existingCallback = mockResourceCallback();
+    final ResourceCallback newCallback = mockResourceCallback();
 
     doAnswer(
             new Answer<Void>() {
@@ -286,8 +286,8 @@ public Void answer(InvocationOnMock invocationOnMock) {
   public void testNotifiesNewCallbackOfExceptionIfCallbackIsAddedDuringOnException() {
     harness = new EngineJobHarness();
     final EngineJob<Object> job = harness.getJob();
-    final ResourceCallback existingCallback = mock(ResourceCallback.class);
-    final ResourceCallback newCallback = mock(ResourceCallback.class);
+    final ResourceCallback existingCallback = mockResourceCallback();
+    final ResourceCallback newCallback = mockResourceCallback();
 
     doAnswer(
             new Answer<Void>() {
@@ -311,7 +311,7 @@ public Void answer(InvocationOnMock invocationOnMock) {
   @Test
   public void testRemovingCallbackDuringOnResourceReadyIsIgnoredIfCallbackHasAlreadyBeenCalled() {
     final EngineJob<Object> job = harness.getJob();
-    final ResourceCallback cb = mock(ResourceCallback.class);
+    final ResourceCallback cb = mockResourceCallback();
 
     doAnswer(
             new Answer<Void>() {
@@ -335,7 +335,7 @@ public Void answer(InvocationOnMock invocationOnMock) {
   public void testRemovingCallbackDuringOnExceptionIsIgnoredIfCallbackHasAlreadyBeenCalled() {
     harness = new EngineJobHarness();
     final EngineJob<Object> job = harness.getJob();
-    final ResourceCallback cb = mock(ResourceCallback.class);
+    final ResourceCallback cb = mockResourceCallback();
 
     doAnswer(
             new Answer<Void>() {
@@ -360,7 +360,7 @@ public Void answer(InvocationOnMock invocationOnMock) {
   public void
       testRemovingCallbackDuringOnResourceReadyPreventsCallbackFromBeingCalledIfNotYetCalled() {
     final EngineJob<Object> job = harness.getJob();
-    final ResourceCallback notYetCalled = mock(ResourceCallback.class);
+    final ResourceCallback notYetCalled = mockResourceCallback();
 
     doAnswer(
             new Answer<Void>() {
@@ -384,7 +384,7 @@ public Void answer(InvocationOnMock invocationOnMock) {
   public void
       testRemovingCallbackDuringOnResourceReadyPreventsResourceFromBeingAcquiredForCallback() {
     final EngineJob<Object> job = harness.getJob();
-    final ResourceCallback notYetCalled = mock(ResourceCallback.class);
+    final ResourceCallback notYetCalled = mockResourceCallback();
 
     doAnswer(
             new Answer<Void>() {
@@ -410,8 +410,8 @@ public Void answer(InvocationOnMock invocationOnMock) {
   public void testRemovingCallbackDuringOnExceptionPreventsCallbackFromBeingCalledIfNotYetCalled() {
     harness = new EngineJobHarness();
     final EngineJob<Object> job = harness.getJob();
-    final ResourceCallback called = mock(ResourceCallback.class);
-    final ResourceCallback notYetCalled = mock(ResourceCallback.class);
+    final ResourceCallback called = mockResourceCallback();
+    final ResourceCallback notYetCalled = mockResourceCallback();
 
     doAnswer(
             new Answer<Void>() {
@@ -482,6 +482,12 @@ public void testSubmitsDecodeJobToUnlimitedSourceServiceWhenDecodingFromSourceOn
     verify(harness.decodeJob).run();
   }
 
+  private static ResourceCallback mockResourceCallback() {
+    ResourceCallback result = mock(ResourceCallback.class);
+    when(result.getLock()).thenReturn(result);
+    return result;
+  }
+
   @SuppressWarnings("unchecked")
   private static class MultiCbHarness {
     final Key key = mock(Key.class);
@@ -524,7 +530,7 @@ public MultiCbHarness() {
           useAnimationPool,
           onlyRetrieveFromCache);
       for (int i = 0; i < numCbs; i++) {
-        cbs.add(mock(ResourceCallback.class));
+        cbs.add(mockResourceCallback());
       }
       for (ResourceCallback cb : cbs) {
         job.addCallback(cb, Executors.directExecutor());
@@ -537,7 +543,7 @@ private static class EngineJobHarness {
     final EngineJob.EngineResourceFactory factory = mock(EngineJob.EngineResourceFactory.class);
     final Key key = mock(Key.class);
     final Handler mainHandler = new Handler();
-    final ResourceCallback cb = mock(ResourceCallback.class);
+    final ResourceCallback cb = mockResourceCallback();
     final Resource<Object> resource = mockResource();
     final EngineResource<Object> engineResource = mock(EngineResource.class);
     final EngineJobListener engineJobListener = mock(EngineJobListener.class);