Skip to content

Commit

Permalink
Add missing Future cancellation checks
Browse files Browse the repository at this point in the history
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
  • Loading branch information
tonihei authored and copybara-github committed Oct 10, 2023
1 parent dc859ea commit fe7c62a
Show file tree
Hide file tree
Showing 5 changed files with 8 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import com.google.common.util.concurrent.ListenableFuture
class PlayableFolderActivity : AppCompatActivity() {
private lateinit var browserFuture: ListenableFuture<MediaBrowser>
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ import com.google.common.util.concurrent.MoreExecutors
class PlayerActivity : AppCompatActivity() {
private lateinit var controllerFuture: ListenableFuture<MediaController>
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
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;

Expand Down Expand Up @@ -348,7 +349,7 @@ public final MediaNotification createNotification(
if (bitmapFuture.isDone()) {
try {
builder.setLargeIcon(Futures.getDone(bitmapFuture));
} catch (ExecutionException e) {
} catch (CancellationException | ExecutionException e) {
Log.w(TAG, getBitmapLoadErrorMessage(e));
}
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ public void updateNotification(MediaSession session, boolean startInForegroundRe
if (controllerAndListener != null && controllerAndListener.controller.isDone()) {
try {
mediaNotificationController = Futures.getDone(controllerAndListener.controller);
} catch (ExecutionException e) {
} catch (CancellationException | ExecutionException e) {
// Ignore.
}
}
Expand Down Expand Up @@ -323,7 +323,7 @@ private MediaController getConnectedControllerForSession(MediaSession session) {
}
try {
return Futures.getDone(controllerAndListener.controller);
} catch (ExecutionException exception) {
} catch (CancellationException | ExecutionException exception) {
// We should never reach this.
throw new IllegalStateException(exception);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1375,7 +1375,7 @@ private void updateMetadataIfChanged() {
if (bitmapFuture.isDone()) {
try {
artworkBitmap = Futures.getDone(bitmapFuture);
} catch (ExecutionException e) {
} catch (CancellationException | ExecutionException e) {
Log.w(TAG, getBitmapLoadErrorMessage(e));
}
} else {
Expand Down

0 comments on commit fe7c62a

Please sign in to comment.