Prevent leaking of the Thread.interrupt flag to other LoadTasks

PiperOrigin-RevId: 311290214

library/core/src/main/java/com/google/android/exoplayer2/upstream/Loader.java

@@ -32,6 +32,7 @@ import java.lang.annotation.Documented;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.util.concurrent.ExecutorService;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * Manages the background loading of {@link Loadable}s.
@@ -56,6 +57,21 @@ public final class Loader implements LoaderErrorThrower {
 
     /**
      * Cancels the load.
+     *
+     * <p>Loadable implementations should ensure that a currently executing {@link #load()} call
+     * will exit reasonably quickly after this method is called. The {@link #load()} call may exit
+     * either by returning or by throwing an {@link IOException}.
+     *
+     * <p>If there is a currently executing {@link #load()} call, then the thread on which that call
+     * is being made will be interrupted immediately after the call to this method. Hence
+     * implementations do not need to (and should not attempt to) interrupt the loading thread
+     * themselves.
+     *
+     * <p>Although the loading thread will be interrupted, Loadable implementations should not use
+     * the interrupted status of the loading thread in {@link #load()} to determine whether the load
+     * has been canceled. This approach is not robust [Internal ref: b/79223737]. Instead,
+     * implementations should use their own flag to signal cancelation (for example, using {@link
+     * AtomicBoolean}).
      */
     void cancelLoad();
 
@@ -307,10 +323,9 @@ public final class Loader implements LoaderErrorThrower {
     private static final String TAG = "LoadTask";
 
     private static final int MSG_START = 0;
-    private static final int MSG_CANCEL = 1;
+    private static final int MSG_FINISH = 1;
-    private static final int MSG_END_OF_SOURCE = 2;
+    private static final int MSG_IO_EXCEPTION = 2;
-    private static final int MSG_IO_EXCEPTION = 3;
+    private static final int MSG_FATAL_ERROR = 3;
-    private static final int MSG_FATAL_ERROR = 4;
 
     public final int defaultMinRetryCount;
 
@@ -321,8 +336,8 @@ public final class Loader implements LoaderErrorThrower {
     @Nullable private IOException currentError;
     private int errorCount;
 
-    @Nullable private volatile Thread executorThread;
+    @Nullable private Thread executorThread;
-    private volatile boolean canceled;
+    private boolean canceled;
     private volatile boolean released;
 
     public LoadTask(Looper looper, T loadable, Loader.Callback<T> callback,
@@ -354,16 +369,21 @@ public final class Loader implements LoaderErrorThrower {
       this.released = released;
       currentError = null;
       if (hasMessages(MSG_START)) {
+        // The task has not been given to the executor yet.
+        canceled = true;
         removeMessages(MSG_START);
         if (!released) {
-          sendEmptyMessage(MSG_CANCEL);
+          sendEmptyMessage(MSG_FINISH);
         }
       } else {
-        canceled = true;
+        // The task has been given to the executor.
-        loadable.cancelLoad();
+        synchronized (this) {
-        @Nullable Thread executorThread = this.executorThread;
+          canceled = true;
-        if (executorThread != null) {
+          loadable.cancelLoad();
-          executorThread.interrupt();
+          @Nullable Thread executorThread = this.executorThread;
+          if (executorThread != null) {
+            executorThread.interrupt();
+          }
         }
       }
       if (released) {
@@ -382,8 +402,12 @@ public final class Loader implements LoaderErrorThrower {
     @Override
     public void run() {
       try {
-        executorThread = Thread.currentThread();
+        boolean shouldLoad;
-        if (!canceled) {
+        synchronized (this) {
+          shouldLoad = !canceled;
+          executorThread = Thread.currentThread();
+        }
+        if (shouldLoad) {
           TraceUtil.beginSection("load:" + loadable.getClass().getSimpleName());
           try {
             loadable.load();
@@ -391,8 +415,13 @@ public final class Loader implements LoaderErrorThrower {
             TraceUtil.endSection();
           }
         }
+        synchronized (this) {
+          executorThread = null;
+          // Clear the interrupted flag if set, to avoid it leaking into a subsequent task.
+          Thread.interrupted();
+        }
         if (!released) {
-          sendEmptyMessage(MSG_END_OF_SOURCE);
+          sendEmptyMessage(MSG_FINISH);
         }
       } catch (IOException e) {
         if (!released) {
@@ -445,10 +474,7 @@ public final class Loader implements LoaderErrorThrower {
         return;
       }
       switch (msg.what) {
-        case MSG_CANCEL:
+        case MSG_FINISH:
-          callback.onLoadCanceled(loadable, nowMs, durationMs, false);
-          break;
-        case MSG_END_OF_SOURCE:
           try {
             callback.onLoadCompleted(loadable, nowMs, durationMs);
           } catch (RuntimeException e) {