From bb81c4969d59d0d0d68c87432fd3c3942d004d11 Mon Sep 17 00:00:00 2001 From: jaewan Date: Wed, 5 Jan 2022 07:27:57 +0000 Subject: [PATCH] Fix missing notification of session release A session may not notify its release back to the controller when the controller is connecting while the session is releasing. Here are issues in detail: - MediaSession doesn't respond to controller's connection request when it's released. - MediaSession discards incoming connection requests when it's released. The requests are only kept in the application looper, but the looper is cleared when the session is released. This CL fixes the above issues, and fixes some flaky tests. PiperOrigin-RevId: 419756158 --- .../media3/session/MediaSessionImpl.java | 12 +- .../media3/session/MediaSessionStub.java | 121 +++++++++++------- .../session/MediaControllerListenerTest.java | 16 +++ .../MediaSessionAndControllerTest.java | 54 ++++++++ 4 files changed, 157 insertions(+), 46 deletions(-) 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.