From e9364b0f6e1a104de9cf4393daab521edc4eccf4 Mon Sep 17 00:00:00 2001 From: tonihei Date: Thu, 8 Dec 2022 11:02:56 +0000 Subject: [PATCH] Clarify and correct allowed multi-threading for some Player methods Some Player methods like getting the Looper and adding listeners were always allowed to be called from any thread, but this is undocumented. This change makes the threading rules of these methods more explicit. Removing listeners was never meant to be called from another thread and we also don't support it safely because final callbacks may be triggered from the wrong thread. To find potential issues, we can assert the correct thread when releasing listeners. Finally, there is a potential race condition when calling addListener from a different thread at the same time as release, which may lead to a registered listener that could receive callbacks after the player is released. PiperOrigin-RevId: 493843981 --- .../com/google/android/exoplayer2/Player.java | 8 +++ .../android/exoplayer2/SimpleBasePlayer.java | 3 +- .../android/exoplayer2/util/ListenerSet.java | 60 +++++++++++++++++-- .../google/android/exoplayer2/ExoPlayer.java | 34 +++++++---- .../android/exoplayer2/ExoPlayerImpl.java | 16 +++-- .../analytics/DefaultAnalyticsCollector.java | 14 +++++ .../android/exoplayer2/ExoPlayerTest.java | 14 ++++- 7 files changed, 123 insertions(+), 26 deletions(-) diff --git a/library/common/src/main/java/com/google/android/exoplayer2/Player.java b/library/common/src/main/java/com/google/android/exoplayer2/Player.java index 1cad966edd..9058ac9f60 100644 --- a/library/common/src/main/java/com/google/android/exoplayer2/Player.java +++ b/library/common/src/main/java/com/google/android/exoplayer2/Player.java @@ -53,6 +53,10 @@ import java.util.List; * A media player interface defining traditional high-level functionality, such as the ability to * play, pause, seek and query properties of the currently playing media. * + *

All methods must be called from a single {@linkplain #getApplicationLooper() application + * thread} unless indicated otherwise. Callbacks in registered listeners are called on the same + * thread. + * *

This interface includes some convenience methods that can be implemented by calling other * methods in the interface. {@link BasePlayer} implements these convenience methods so inheriting * {@link BasePlayer} is recommended when implementing the interface so that only the minimal set of @@ -1546,6 +1550,8 @@ public interface Player { /** * Returns the {@link Looper} associated with the application thread that's used to access the * player and on which player events are received. + * + *

This method can be called from any thread. */ Looper getApplicationLooper(); @@ -1555,6 +1561,8 @@ public interface Player { *

The listener's methods will be called on the thread associated with {@link * #getApplicationLooper()}. * + *

This method can be called from any thread. + * * @param listener The listener to register. */ void addListener(Listener listener); diff --git a/library/common/src/main/java/com/google/android/exoplayer2/SimpleBasePlayer.java b/library/common/src/main/java/com/google/android/exoplayer2/SimpleBasePlayer.java index 66a3d8ced9..ae6c89dbc1 100644 --- a/library/common/src/main/java/com/google/android/exoplayer2/SimpleBasePlayer.java +++ b/library/common/src/main/java/com/google/android/exoplayer2/SimpleBasePlayer.java @@ -1981,8 +1981,7 @@ public abstract class SimpleBasePlayer extends BasePlayer { @Override public final void removeListener(Listener listener) { - // Don't verify application thread. We allow calls to this method from any thread. - checkNotNull(listener); + verifyApplicationThreadAndInitState(); listeners.remove(listener); } diff --git a/library/common/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java b/library/common/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java index 52c7c0bf9a..e1a6ec6504 100644 --- a/library/common/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java +++ b/library/common/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java @@ -15,9 +15,12 @@ */ package com.google.android.exoplayer2.util; +import static com.google.android.exoplayer2.util.Assertions.checkState; + import android.os.Looper; import android.os.Message; import androidx.annotation.CheckResult; +import androidx.annotation.GuardedBy; import androidx.annotation.Nullable; import com.google.android.exoplayer2.C; import java.util.ArrayDeque; @@ -33,6 +36,9 @@ import org.checkerframework.checker.nullness.qual.NonNull; *

Events are also guaranteed to be only sent to the listeners registered at the time the event * was enqueued and haven't been removed since. * + *

All methods must be called on the {@link Looper} passed to the constructor unless indicated + * otherwise. + * * @param The listener type. */ public final class ListenerSet { @@ -74,14 +80,18 @@ public final class ListenerSet { private final CopyOnWriteArraySet> listeners; private final ArrayDeque flushingEvents; private final ArrayDeque queuedEvents; + private final Object releasedLock; + @GuardedBy("releasedLock") private boolean released; + private boolean throwsWhenUsingWrongThread; + /** * Creates a new listener set. * * @param looper A {@link Looper} used to call listeners on. The same {@link Looper} must be used - * to call all other methods of this class. + * to call all other methods of this class unless indicated otherwise. * @param clock A {@link Clock}. * @param iterationFinishedEvent An {@link IterationFinishedEvent} sent when all other events sent * during one {@link Looper} message queue iteration were handled by the listeners. @@ -98,17 +108,21 @@ public final class ListenerSet { this.clock = clock; this.listeners = listeners; this.iterationFinishedEvent = iterationFinishedEvent; + releasedLock = new Object(); flushingEvents = new ArrayDeque<>(); queuedEvents = new ArrayDeque<>(); // It's safe to use "this" because we don't send a message before exiting the constructor. @SuppressWarnings("nullness:methodref.receiver.bound") HandlerWrapper handler = clock.createHandler(looper, this::handleMessage); this.handler = handler; + throwsWhenUsingWrongThread = true; } /** * Copies the listener set. * + *

This method can be called from any thread. + * * @param looper The new {@link Looper} for the copied listener set. * @param iterationFinishedEvent The new {@link IterationFinishedEvent} sent when all other events * sent during one {@link Looper} message queue iteration were handled by the listeners. @@ -122,6 +136,8 @@ public final class ListenerSet { /** * Copies the listener set. * + *

This method can be called from any thread. + * * @param looper The new {@link Looper} for the copied listener set. * @param clock The new {@link Clock} for the copied listener set. * @param iterationFinishedEvent The new {@link IterationFinishedEvent} sent when all other events @@ -139,14 +155,18 @@ public final class ListenerSet { * *

If a listener is already present, it will not be added again. * + *

This method can be called from any thread. + * * @param listener The listener to be added. */ public void add(T listener) { - if (released) { - return; - } Assertions.checkNotNull(listener); - listeners.add(new ListenerHolder<>(listener)); + synchronized (releasedLock) { + if (released) { + return; + } + listeners.add(new ListenerHolder<>(listener)); + } } /** @@ -157,6 +177,7 @@ public final class ListenerSet { * @param listener The listener to be removed. */ public void remove(T listener) { + verifyCurrentThread(); for (ListenerHolder listenerHolder : listeners) { if (listenerHolder.listener.equals(listener)) { listenerHolder.release(iterationFinishedEvent); @@ -167,11 +188,13 @@ public final class ListenerSet { /** Removes all listeners from the set. */ public void clear() { + verifyCurrentThread(); listeners.clear(); } /** Returns the number of added listeners. */ public int size() { + verifyCurrentThread(); return listeners.size(); } @@ -183,6 +206,7 @@ public final class ListenerSet { * @param event The event. */ public void queueEvent(int eventFlag, Event event) { + verifyCurrentThread(); CopyOnWriteArraySet> listenerSnapshot = new CopyOnWriteArraySet<>(listeners); queuedEvents.add( () -> { @@ -194,6 +218,7 @@ public final class ListenerSet { /** Notifies listeners of events previously enqueued with {@link #queueEvent(int, Event)}. */ public void flushEvents() { + verifyCurrentThread(); if (queuedEvents.isEmpty()) { return; } @@ -232,11 +257,27 @@ public final class ListenerSet { *

This will ensure no events are sent to any listener after this method has been called. */ public void release() { + verifyCurrentThread(); + synchronized (releasedLock) { + released = true; + } for (ListenerHolder listenerHolder : listeners) { listenerHolder.release(iterationFinishedEvent); } listeners.clear(); - released = true; + } + + /** + * Sets whether methods throw when using the wrong thread. + * + *

Do not use this method unless to support legacy use cases. + * + * @param throwsWhenUsingWrongThread Whether to throw when using the wrong thread. + * @deprecated Do not use this method and ensure all calls are made from the correct thread. + */ + @Deprecated + public void setThrowsWhenUsingWrongThread(boolean throwsWhenUsingWrongThread) { + this.throwsWhenUsingWrongThread = throwsWhenUsingWrongThread; } private boolean handleMessage(Message message) { @@ -252,6 +293,13 @@ public final class ListenerSet { return true; } + private void verifyCurrentThread() { + if (!throwsWhenUsingWrongThread) { + return; + } + checkState(Thread.currentThread() == handler.getLooper().getThread()); + } + private static final class ListenerHolder { public final T listener; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayer.java b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayer.java index 6889337910..9960f6c4ae 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayer.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayer.java @@ -125,15 +125,15 @@ import java.util.List; * threading model"> * *