diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java b/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java index 6385e1eebe..50be173374 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java @@ -313,7 +313,7 @@ import org.checkerframework.checker.initialization.qual.Initialized; if (broadcastReceiver != null) { context.unregisterReceiver(broadcastReceiver); } - dispatchRemoteControllerTaskWithoutReturn(ControllerCb::onDisconnected); + sessionStub.release(); } public PlayerWrapper getPlayerWrapper() { @@ -604,7 +604,10 @@ import org.checkerframework.checker.initialization.qual.Initialized; // 0 is OK for legacy controllers, because they didn't have sequence numbers. seq = 0; } - task.run(checkStateNotNull(controller.getControllerCb()), seq); + ControllerCb cb = controller.getControllerCb(); + if (cb != null) { + task.run(cb, seq); + } } catch (DeadObjectException e) { onDeadObjectException(controller, e); } catch (RemoteException e) { @@ -637,7 +640,10 @@ import org.checkerframework.checker.initialization.qual.Initialized; // Tell that operation is successful, although we don't know the actual result. future = Futures.immediateFuture(new SessionResult(SessionResult.RESULT_SUCCESS)); } - task.run(checkStateNotNull(controller.getControllerCb()), seq); + ControllerCb cb = controller.getControllerCb(); + if (cb != null) { + task.run(cb, seq); + } return future; } catch (DeadObjectException e) { onDeadObjectException(controller, e); diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java b/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java index 41b3db0dd9..63f77581b3 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java @@ -81,9 +81,12 @@ import com.google.common.collect.ImmutableList; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.MoreExecutors; import java.lang.ref.WeakReference; +import java.util.Collections; import java.util.Deque; import java.util.List; +import java.util.Set; import java.util.concurrent.CancellationException; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutionException; /** @@ -96,19 +99,17 @@ import java.util.concurrent.ExecutionException; private static final String TAG = "MediaSessionStub"; - private final Object lock; private final WeakReference sessionImpl; private final MediaSessionManager sessionManager; private final ConnectedControllersManager connectedControllersManager; + private final Set pendingControllers; public MediaSessionStub(MediaSessionImpl sessionImpl) { - // Initialize default values. - lock = new Object(); - // Initialize members with params. this.sessionImpl = new WeakReference<>(sessionImpl); sessionManager = MediaSessionManager.getSessionManager(sessionImpl.getContext()); connectedControllersManager = new ConnectedControllersManager<>(sessionImpl); + pendingControllers = Collections.newSetFromMap(new ConcurrentHashMap<>()); } public ConnectedControllersManager getConnectedControllersManager() { @@ -331,50 +332,58 @@ import java.util.concurrent.ExecutionException; connectionHints); @Nullable MediaSessionImpl sessionImpl = this.sessionImpl.get(); if (sessionImpl == null || sessionImpl.isReleased()) { + try { + caller.onDisconnected(/* seq= */ 0); + } catch (RemoteException e) { + // Controller may be died prematurely. + // Not an issue because we'll ignore it anyway. + } return; } + pendingControllers.add(controllerInfo); postOrRun( sessionImpl.getApplicationHandler(), () -> { - if (sessionImpl.isReleased()) { - return; - } - IBinder callbackBinder = - checkStateNotNull((Controller2Cb) controllerInfo.getControllerCb()) - .getCallbackBinder(); - MediaSession.ConnectionResult connectionResult = - sessionImpl.onConnectOnHandler(controllerInfo); - // Don't reject connection for the request from trusted app. - // Otherwise server will fail to retrieve session's information to dispatch - // media keys to. - boolean accept = connectionResult.isAccepted || controllerInfo.isTrusted(); - if (accept) { + boolean connected = false; + try { + pendingControllers.remove(controllerInfo); + if (sessionImpl.isReleased()) { + return; + } + IBinder callbackBinder = + checkStateNotNull((Controller2Cb) controllerInfo.getControllerCb()) + .getCallbackBinder(); + MediaSession.ConnectionResult connectionResult = + sessionImpl.onConnectOnHandler(controllerInfo); + // Don't reject connection for the request from trusted app. + // Otherwise server will fail to retrieve session's information to dispatch + // media keys to. + if (!connectionResult.isAccepted && !controllerInfo.isTrusted()) { + return; + } if (!connectionResult.isAccepted) { - // For trusted apps, send non-null allowed commands to keep - // connection. + // For the accepted controller, send non-null allowed commands to keep connection. connectionResult = MediaSession.ConnectionResult.accept( SessionCommands.EMPTY, Player.Commands.EMPTY); } SequencedFutureManager sequencedFutureManager; - synchronized (lock) { - if (connectedControllersManager.isConnected(controllerInfo)) { - Log.w( - TAG, - "Controller " - + controllerInfo - + " has sent connection" - + " request multiple times"); - } - connectedControllersManager.addController( - callbackBinder, - controllerInfo, - connectionResult.availableSessionCommands, - connectionResult.availablePlayerCommands); - sequencedFutureManager = - checkStateNotNull( - connectedControllersManager.getSequencedFutureManager(controllerInfo)); + if (connectedControllersManager.isConnected(controllerInfo)) { + Log.w( + TAG, + "Controller " + + controllerInfo + + " has sent connection" + + " request multiple times"); } + connectedControllersManager.addController( + callbackBinder, + controllerInfo, + connectionResult.availableSessionCommands, + connectionResult.availablePlayerCommands); + sequencedFutureManager = + checkStateNotNull( + connectedControllersManager.getSequencedFutureManager(controllerInfo)); // If connection is accepted, notify the current state to the controller. // It's needed because we cannot call synchronous calls between // session/controller. @@ -399,22 +408,48 @@ import java.util.concurrent.ExecutionException; try { caller.onConnected( sequencedFutureManager.obtainNextSequenceNumber(), state.toBundle()); + connected = true; } catch (RemoteException e) { // Controller may be died prematurely. } - sessionImpl.onPostConnectOnHandler(controllerInfo); - } else { - try { - caller.onDisconnected(0); - } catch (RemoteException e) { - // Controller may be died prematurely. - // Not an issue because we'll ignore it anyway. + } finally { + if (!connected) { + try { + caller.onDisconnected(/* seq= */ 0); + } catch (RemoteException e) { + // Controller may be died prematurely. + // Not an issue because we'll ignore it anyway. + } } } }); } + public void release() { + List controllers = connectedControllersManager.getConnectedControllers(); + for (ControllerInfo controller : controllers) { + ControllerCb cb = controller.getControllerCb(); + if (cb != null) { + try { + cb.onDisconnected(/* seq= */ 0); + } catch (RemoteException e) { + // Ignore. We're releasing. + } + } + } + for (ControllerInfo controller : pendingControllers) { + ControllerCb cb = controller.getControllerCb(); + if (cb != null) { + try { + cb.onDisconnected(/* seq= */ 0); + } catch (RemoteException e) { + // Ignore. We're releasing. + } + } + } + } + ////////////////////////////////////////////////////////////////////////////////////////////// // AIDL methods for session overrides ////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerListenerTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerListenerTest.java index 100ed12dce..cdc6011b3d 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerListenerTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerListenerTest.java @@ -83,6 +83,7 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; import org.junit.After; +import org.junit.Assert; import org.junit.Before; import org.junit.ClassRule; import org.junit.Rule; @@ -172,6 +173,21 @@ public class MediaControllerListenerTest { assertThat(controller).isNotNull(); } + @Test + public void connection_toReleasedSession() throws Exception { + CountDownLatch latch = new CountDownLatch(1); + SessionToken token = remoteSession.getToken(); + remoteSession.release(); + ListenableFuture controllerFuture = + new MediaController.Builder(context, token) + .setApplicationLooper(threadTestRule.getHandler().getLooper()) + .buildAsync(); + controllerFuture.addListener(() -> latch.countDown(), threadTestRule.getHandler()::post); + latch.await(TIMEOUT_MS, MILLISECONDS); + Assert.assertThrows( + ExecutionException.class, () -> controllerFuture.get(/* timeout= */ 0, MILLISECONDS)); + } + @Test public void connection_sessionReleased() throws Exception { CountDownLatch latch = new CountDownLatch(1); diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java index ac261bfad6..6b04d16644 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java @@ -22,14 +22,21 @@ import static com.google.common.truth.Truth.assertThat; import static java.util.concurrent.TimeUnit.MILLISECONDS; import android.content.Context; +import android.os.Handler; import android.os.Looper; import androidx.media3.common.Player; +import androidx.media3.common.util.Util; import androidx.media3.test.session.common.HandlerThreadTestRule; import androidx.media3.test.session.common.MainLooperTestRule; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; import androidx.test.filters.LargeTest; +import com.google.common.util.concurrent.ListenableFuture; +import java.util.concurrent.CancellationException; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executor; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import org.junit.Before; import org.junit.ClassRule; @@ -100,6 +107,53 @@ public class MediaSessionAndControllerTest { }); } + /** Tests to ensure that disconnection is notified. */ + @Test + public void connecting_whileSessionIsReleasing_notified() throws Exception { + MockPlayer player = + new MockPlayer.Builder() + .setApplicationLooper(threadTestRule.getHandler().getLooper()) + .build(); + Handler mainHandler = new Handler(Looper.getMainLooper()); + Executor mainExecutor = (runnable) -> Util.postOrRun(mainHandler, runnable); + for (int i = 0; i < 100; i++) { + int idx = i; + MediaSession session = + sessionTestRule.ensureReleaseAfterTest( + new MediaSession.Builder(context, player).setId(TAG).build()); + CountDownLatch latch = new CountDownLatch(1); + mainHandler.post( + () -> { + ListenableFuture controllerFuture = + new MediaController.Builder(context, session.getToken()) + .setListener( + new MediaController.Listener() { + @Override + public void onDisconnected(MediaController controller) { + latch.countDown(); + } + }) + .buildAsync(); + controllerFuture.addListener( + () -> { + try { + MediaController controller = + controllerFuture.get(/* timeout=* */ 0, MILLISECONDS); + assertThat(controller).isNotNull(); + } catch (ExecutionException + | InterruptedException + | CancellationException + | TimeoutException e) { + latch.countDown(); + } + }, + mainExecutor); + }); + threadTestRule.getHandler().postAndSync(session::release); + assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); + } + } + @Test public void connect_withTheSameLooper_connectsInTheSameLooperIteration() throws Exception { // This may hang if controller cannot be connected immediately in a looper iteration.