From fe7c62afe0b39f8d6617cf610dbdccc9e6adcfb4 Mon Sep 17 00:00:00 2001 From: tonihei Date: Tue, 10 Oct 2023 02:12:22 -0700 Subject: [PATCH] Add missing Future cancellation checks Future.isDone and getDone doesn't imply the Future was successful and it may have been cancelled or failed. In case where we handle failure, we should also handle cancellation to avoid CancellationException to bubble up unchecked. In demo app code where we use isDone for field initialization, we want to crash in the failure case (usually security exception where the connection is disallowed), but we want to gracefully handle cancellation. Cancellation of these variables usually happens in Activity.onDestroy/onStop, but methods may be called after this point. #minor-release PiperOrigin-RevId: 572178018 --- .../androidx/media3/demo/session/PlayableFolderActivity.kt | 2 +- .../main/java/androidx/media3/demo/session/PlayerActivity.kt | 3 ++- .../media3/session/DefaultMediaNotificationProvider.java | 3 ++- .../androidx/media3/session/MediaNotificationManager.java | 4 ++-- .../java/androidx/media3/session/MediaSessionLegacyStub.java | 2 +- 5 files changed, 8 insertions(+), 6 deletions(-) diff --git a/demos/session/src/main/java/androidx/media3/demo/session/PlayableFolderActivity.kt b/demos/session/src/main/java/androidx/media3/demo/session/PlayableFolderActivity.kt index f9c6873e80..e4a048ae91 100644 --- a/demos/session/src/main/java/androidx/media3/demo/session/PlayableFolderActivity.kt +++ b/demos/session/src/main/java/androidx/media3/demo/session/PlayableFolderActivity.kt @@ -43,7 +43,7 @@ import com.google.common.util.concurrent.ListenableFuture class PlayableFolderActivity : AppCompatActivity() { private lateinit var browserFuture: ListenableFuture private val browser: MediaBrowser? - get() = if (browserFuture.isDone) browserFuture.get() else null + get() = if (browserFuture.isDone && !browserFuture.isCancelled) browserFuture.get() else null private lateinit var mediaList: ListView private lateinit var mediaListAdapter: PlayableMediaItemArrayAdapter diff --git a/demos/session/src/main/java/androidx/media3/demo/session/PlayerActivity.kt b/demos/session/src/main/java/androidx/media3/demo/session/PlayerActivity.kt index af1f15c7d8..a87e177b58 100644 --- a/demos/session/src/main/java/androidx/media3/demo/session/PlayerActivity.kt +++ b/demos/session/src/main/java/androidx/media3/demo/session/PlayerActivity.kt @@ -45,7 +45,8 @@ import com.google.common.util.concurrent.MoreExecutors class PlayerActivity : AppCompatActivity() { private lateinit var controllerFuture: ListenableFuture private val controller: MediaController? - get() = if (controllerFuture.isDone) controllerFuture.get() else null + get() = + if (controllerFuture.isDone && !controllerFuture.isCancelled) controllerFuture.get() else null private lateinit var playerView: PlayerView private lateinit var mediaItemListView: ListView diff --git a/libraries/session/src/main/java/androidx/media3/session/DefaultMediaNotificationProvider.java b/libraries/session/src/main/java/androidx/media3/session/DefaultMediaNotificationProvider.java index a11aa3d821..e9d77e75f7 100644 --- a/libraries/session/src/main/java/androidx/media3/session/DefaultMediaNotificationProvider.java +++ b/libraries/session/src/main/java/androidx/media3/session/DefaultMediaNotificationProvider.java @@ -53,6 +53,7 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.util.Arrays; import java.util.List; +import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; import org.checkerframework.checker.nullness.qual.MonotonicNonNull; @@ -348,7 +349,7 @@ public class DefaultMediaNotificationProvider implements MediaNotification.Provi if (bitmapFuture.isDone()) { try { builder.setLargeIcon(Futures.getDone(bitmapFuture)); - } catch (ExecutionException e) { + } catch (CancellationException | ExecutionException e) { Log.w(TAG, getBitmapLoadErrorMessage(e)); } } else { diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaNotificationManager.java b/libraries/session/src/main/java/androidx/media3/session/MediaNotificationManager.java index b6c3d56280..82e497c537 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaNotificationManager.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaNotificationManager.java @@ -214,7 +214,7 @@ import java.util.concurrent.TimeoutException; if (controllerAndListener != null && controllerAndListener.controller.isDone()) { try { mediaNotificationController = Futures.getDone(controllerAndListener.controller); - } catch (ExecutionException e) { + } catch (CancellationException | ExecutionException e) { // Ignore. } } @@ -323,7 +323,7 @@ import java.util.concurrent.TimeoutException; } try { return Futures.getDone(controllerAndListener.controller); - } catch (ExecutionException exception) { + } catch (CancellationException | ExecutionException exception) { // We should never reach this. throw new IllegalStateException(exception); } diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaSessionLegacyStub.java b/libraries/session/src/main/java/androidx/media3/session/MediaSessionLegacyStub.java index 7d5e4d76b2..26875d9e1a 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaSessionLegacyStub.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaSessionLegacyStub.java @@ -1375,7 +1375,7 @@ import org.checkerframework.checker.initialization.qual.Initialized; if (bitmapFuture.isDone()) { try { artworkBitmap = Futures.getDone(bitmapFuture); - } catch (ExecutionException e) { + } catch (CancellationException | ExecutionException e) { Log.w(TAG, getBitmapLoadErrorMessage(e)); } } else {