-
Notifications
You must be signed in to change notification settings - Fork 299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ANR related to AbstractFuture.get() #890
Comments
I agree this seems surprising - like something in Guava isn't working the way we expect. What happens if you replace the call to |
Hi @icbaker, thanks for taking time to look at this, I appreciate your help! I've experimented with both of your suggestions (using In thinking about this further, something else occurred to me. There is second place in the app where we create a SessionToken sessionToken =
new SessionToken(this, new ComponentName(this, PlaybackService.class));
controllerFuture = new MediaController.Builder(this, sessionToken).buildAsync();
controllerFuture.addListener(() -> {
try {
mediaController = controllerFuture.get();
mediaController.addListener(playerListener);
playerListener.onIsPlayingChanged(mediaController.isPlaying());
updatePlayerUI();
} catch (InterruptedException | ExecutionException | CancellationException e) {
Log.e(LOG_TAG, "Could not create media controller. " + e.toString());
}
}, ContextCompat.getMainExecutor(this)); But interestingly, I haven't seen any similar ANRs happening in
That last sentence makes me suspect it might not be safe to garbage collect Any thoughts or insights you have around this would be much appreciated! Thanks again for your help, and have a nice day. |
For background on the issue, and why this change may help, see androidx/media#890 The basic idea here is to reduce the rate at which we create and release MediaControllers in MiniPlayerFragment, by moving the creation and releasing to fragment lifecycle callbacks that are invoked less frequently than `onResume()` and `onPause()`. It's not completely clear that this will definitely solve the problems we're seeing in production, but it seems like a worthwhile experiment.
Ah I hadn't spotted in your original code snippet that Given that, it seems much safer to switch to my |
Thanks @icbaker. I had actually originally implemented
So I switched
I'm pretty sure there is only a single thread (the main application thread) that calls any of the In any case, I'm very open to your suggestion to move to using Thanks again for your help here! |
For background on the issue, and why this change may help, see androidx/media#890. I had hoped that upgrading the Media3 library version back in #92 would completely solve the problems we had been seeing in production. While it did help in some ways, by eliminating `ForegroundServiceDidNotStartInTimeException`s (see androidx/media#167), it also seems to have morphed a lot of problems into a new form, namely App Not Responding (ANR) problems that arise in `MiniPlayerFragment`. The basic idea here is to reduce the rate at which we create and release `MediaController`s in `MiniPlayerFragment`, by moving the creation and releasing to fragment lifecycle callbacks that are invoked less frequently than `onResume()` and `onPause()`. My hypothesis is that rapid creation and releasing of `MediaController`s results in some kind of race condition that is very hard to reproduce locally but will occur in production with enough use of the app. It's not completely clear that this PR will definitely solve the problems we're seeing in production, but it seems like a worthwhile experiment to try. Other than potentially decreasing the rate of ANRs, it should not have any other noticeable effects on the behavior of the app.
Yeah that would be my assumption too, but I haven't come up with a single-threaded sequence of events that explains the issue you're seeing - and it's also worth noting that you can't repro locally, so your local testing may not accurately represent the problematic scenario.
Edit: This isn't true (see comment below).
It's not really a Guava-specific thing, it's just about Java scopes and when references are resolved. You could also use Some comparisons (in Java, sorry, hopefully clear enough though): class Foo {
private ListenableFuture<Bar> barFutureField;
private void usingFieldReference() {
barFutureField = createBarFuture();
barFuture.addListener(
() -> {
try {
// References barFutureField, and this resolves to a particular object
// *when this line is executed inside the callback* (i.e. after the future
// is done), meaning if the field is reassigned then we will access the
// 'new' (possibly incomplete) object here.
Bar bar = barFutureField.get();
// Do something with bar
} catch (ExecutionException | InterruptedException e) {
// Handle exception
}
}, ContextCompat.getMainExecutor(this));
}
private void usingLocalReference() {
barFutureField = createBarFuture();
// Resolves to a specific object *before* the callback is invoked
// (or even registered).
ListenableFuture<Bar> barFuturelocal = barFutureField;
// Add the listener to the local, not the field
barFuturelocal.addListener(
() -> {
try {
// This *must* be the same object the listener was registered on,
// since it's a local.
Bar bar = barFuturelocal.get();
// Do something with bar
} catch (ExecutionException | InterruptedException e) {
// Handle exception
}
}, ContextCompat.getMainExecutor(this));
}
private void usingAddCallback() {
barFutureField = createBarFuture();
Futures.addCallback(
barFutureField,
new FutureCallback() {
public void onSuccess(Bar result) {
// You get the result passed in here, so no need to
// call barFutureField.get() and no scope for accidentally
// accessing a new, reassigned, incomplete future.
}
public void onFailure(Throwable t) {
// Handle exception
}
},
ContextCompat.getMainExecutor(this));
}
} |
Yes, that's a fair point! 🙂
Ah, thank you - I missed that
Thank you, I really appreciate you taking the time to write this out. For some reason, I had been under the mistaken assumption that As an experiment, we released a small change to production two days ago, which simply moves the media controller initialization code in I'm not completely sure what to make of this yet, other than to hypothesize there may be some kind of interaction taking place between In any case, I think our next step should be to switch to your suggestion of trying to use |
My statement here is incorrect (I've edited the comment above). So you could be right and this code is brittle in the same way. One difference I noticed between the session demo app and your example is that you explicitly pass
The controller future is completed here: media/libraries/session/src/main/java/androidx/media3/session/MediaController.java Line 321 in f6fe90f
This completion happens on the application looper, which is by default is the looper of the thread used to build the I don't have solid theory here, but I wonder if the fact your listener invocation is added to the back of the main looper's queue instead of being executed immediately can result in it executing 'late', with the field already having been reassigned (during a previous loper iteration). |
Oh, that's an interesting idea about the possible difference in executor behavior causing a problem, thanks! Switching to Thanks again for helping to dig into this! |
This merge pulls in changes that were made to fix the "App Not Responding" issues that we have been seeing in production. Apart from updating to the latest release of the media3 library, the changes concern two ways in which the new `PlaybackService` works. **Retrieval of the MediaController** Our ANR problem has been discussed in [this](androidx/media#890) open issue. There was a very helpful response regarding what might be done. In particular, the suggestions were: 1. Using `Futures.addCallback()` instead of `controllerFuture.addListener()` 2. Using `MoreExecutors.directExecutor()` instead of `ContextCompat.getMainExecutor()` Both of these were implemented in 403c480 **Stopping the `MediaSessionService` when the app is killed in the background** Another potential source of ANRs and crashes was described in [this issue](androidx/media#805), which applies to our app as well. I've also been able to reproduce a similar and perhaps directly related issue on a phone running an older version of android (see the video in #99). To limit the scope of the changes in this pull request, I've opted for the simple solution of ending the `MediaSessionService` when the app is closed in the background (see c980064). With this change, the media notification no longer "hangs around" when the app is stopped in the background, thereby preventing this kind of issue. The disadvantage of this approach is that the media notification is gone and the user has to explicitly re-start the app and find the right talk to continue listening. I've created the branch `playback_resumption` to implement a more satisfactory solution (following the [official documentation](https://developer.android.com/media/media3/session/background-playback#resumption)).
With some delay, we have finally gotten around to implementing your suggestions, @icbaker. I'm happy to report that this has solved our problem: we are no longer seeing any related ANRs in production! To summarize, the changes we implemented were the following:
I didn't have the patience to phase in one change at a time, so we can't be 100% certain which of the two resolved the issue. Looking at the code, however, I agree with @bb4242 that a race condition related to the Thanks again for your support! Your clear and detailed answers were very helpful. |
I originally tried switching to `Futures.addCallback` (as a follow-up to Issue: #890), but it seemed like a good chance to go further into Kotlin-ification. Before this change, if the connection to the session failed, the app would hang at the 'waiting' screen with nothing logged (and the music keeps playing). This behaviour is maintained with the `try/catch` around the `.await()` call (with additional logging). Without this, the failed connection causes the `PlayerActivity` to crash and the music in the background stops. The `try/catch` is used to flag to developers who might be using this app as an example that connecting to the session may fail, and they may want to handle that. This change also switches `this.controller` to be `lateinit` instead of nullable. Issue: #890 PiperOrigin-RevId: 638948568
Thanks for the update. I spent a bit of time looking at the session demo app's usage of I'm going to close this issue, as I think the question has been resolved. |
Version
Media3 1.2.0
More version details
No response
Devices that reproduce the issue
I've seen this happen on many different devices, including:
Google bluejay
Google sunfish
Samsung dm1q
Samsung r9q
Mix of Android 13 and Android 14
Devices that do not reproduce the issue
I'm not sure if I've noticed any specific devices that don't have this problem.
Reproducible in the demo app?
Not tested
Reproduction steps
Hello! I have an app that plays media using the Media3 library. We're currently seeing a number of ANRs in production in the Google Play console. The cause is shown as
The stack trace for the main thread is
The
MiniPlayerFragment
code in the stack trace attempts to create aMediaController
instance inonResume
, which connects to ourMediaSessionService
, like this:It also attempts to release the controller in
onPause
:This ANR is very difficult to reproduce for me when testing locally, but it shows up regularly in production. As far as I can tell, the root of the problem is that
controllerFuture.get()
is blocking long enough to cause the ANR. I'm not sure why it would block, since as far as I know, that code should only be executed once the future is complete. Do you have any idea why this is happening? It seems like this might be somewhat related to #167, but I'm not seeing anyForegroundServiceDidNotStartInTimeException
s happening in production.Expected result
The future should complete successfully and return the
MediaController
object.Actual result
The
controllerFuture.get()
call blocks long enough to cause an ANR.Media
I don't think this is related to the specific media being played, since it's happening while trying to get a
MediaController
object.Bug Report
adb bugreport
to android-media-github@google.com after filing this issue.The text was updated successfully, but these errors were encountered: