Skip to content
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

Thread deadlock when using the media2 extension #8011

Closed
georgi-neykov-hub opened this issue Sep 29, 2020 · 7 comments
Closed

Thread deadlock when using the media2 extension #8011

georgi-neykov-hub opened this issue Sep 29, 2020 · 7 comments
Assignees
Labels

Comments

@georgi-neykov-hub
Copy link

georgi-neykov-hub commented Sep 29, 2020

[REQUIRED] Issue description

I am experiencing thread deadlocks on the project I am working on. It uses the new media2 extension and the latest Exoplayer library artifact versions (2.12.0). The issue occurs when a non-default Executor is being set as a callback executor to a MediaLibraryService.MediaLibrarySession.Builder. The created MediaLibrarySession is connected to a SessionPlayerConnector.

[REQUIRED] Reproduction steps

I've attached thread dumps at the end of the report which display what's getting blocked and why.
The issue reproduction depends on changes of the buffering state being reported to SessionPlayerConnector.setBufferingState().

At some point MediaSessionImpl... is calling SessionPlayerConnector.getBufferingState() which internally posts a callback to the main thread's event loop, then blocks the thread waiting for the callback to execute (SessionPlayerConnector.runPlayerCallableBlocking()). The problem is this is all happening while an internal lock is being held inside MediaSessionImplBase.createPlaybackStateCompat(), which will not be released until the main thread looper processes the posted callbacks. At the same on the main thread event queue there's a preceding callback which is trying to acquire the same lock, thus waiting forever.

To me it seems the preceding callback originates from a call to SessionPlayerConnector.notifySessionPlayerCallback() and has something to do with the following code and comments in SessionCallback's constructor:

// Register PlayerCallback and make it to be called before the ListenableFuture set the result. // It help the PlayerCallback to update allowed commands before pended Player APIs are executed. sessionPlayerConnector.registerPlayerCallback(Runnable::run, new PlayerCallback());

At the time of the deadlock there were 2 registered SessionPlayer.PlayerCallback/Executor pairs, the one from the comment above, and the second being a MediaSessionImplBase.SessionPlayerCallback tied to the media session's callback executor.

[REQUIRED] Link to test content

The issue isn't media content-specific, but related to state handling and a concurrency issue.

[REQUIRED] A full bug report captured from the device

bugreport-sdk_gphone_x86_64-RSR1.200819.001.A1-2020-09-29-12-01-30.zip

Thread dump
Thread dump

[REQUIRED] Version of ExoPlayer being used

2.12.0 (+ media2 extension)

[REQUIRED] Device(s) and version(s) of Android being used

This isn't device or API-specific, reproduced and reported with an Android 11 (API30) emulator, (Image rev. 8, emulator v30.0.26) on a Windows 10 machine

@krocard
Copy link
Contributor

krocard commented Oct 1, 2020

Thank you for the analysis of the issue. @jaewan-github can you take a look at this main thread deadlock situation?

@ojw28
Copy link
Contributor

ojw28 commented Oct 13, 2020

I see the deadlock in the thread dump (I only looked at the first one, but it was sufficient by itself). I'm not really sure how to resolve it, or whether the combination of threads you're using is just fundamentally not going to work. It doesn't look like something that's easy to fix, in any case!

The issue occurs when a non-default Executor is being set as a callback executor to a MediaLibraryService.MediaLibrarySession.Builder.

Is there a reason you need to do this, or can you just use the default one for the time being?

@georgi-neykov-hub
Copy link
Author

Is there a reason you need to do this, or can you just use the default one for the time being?

Well, the controller-side media2 APIs all return ListenableFuture allowing for chaining of operations. The service/session side of the media2 (and the media2 extension for ExoPlayer) APIs are all synchronous, requiring the operation to be completed in the scope of the called callback method. Implementing custom actions (or actions not part of the built-in ones) that require some long running operations (DB access, file writing, etc) can't be executed on the main thread.
The media2 APIs are allowing end users to provide their own executor for handling the MediaSession.SessionCallback calls to handle these problems so it's a valid use case. Either document somewhere this feature does not work well with the media2 extension or change the execution waiting logic.

I've been monitoring the media2 library's progress very much since it has been published. I've also started closely monitoring each Exoplayer release, waiting for the extension that was promised in one the project's issues. And suddenly it came, just in time for an unpublished project that was being revived and updated. My initial enthusiasm is gone after 3+ weeks of 12h+ daily battles with the API, obscure bugs, deadlocks, guava dependency craziness and complete lag of documentation, training, sample project or even a blog post on the media2 topic from the AndroidX team.

The non-published project that was using the "deprecated" mediacompat library and I wanted to migrate the project to use the newly suggested APIs. In the end I've reverted it all, a MediaLibraryService implementation, updated MediaBrowser request handling code, custom actions for playlist management, all neatly divided, hooked up to Dagger modules, unit tests.

Don't get me wrong, I've been keeping track on using the exoplayer library for quite some time, you've done a great job with the good documentation and clean APIs that allow extended support for playback formats and keeps developers aways of the platform MediaPlayer's state craziness.

I am sure you have some form of contact with the team driving the media2 extension. There are gaps in the documentation, ambiguity in some the APIs (Why having an empty playlist is such a sin?). Even your team slipped on this problem, there's not much talk about concurrency, deadlocks, and how to make use of the ListenableFuture returned by most methods that need to be async.

@SungsooLim
Copy link

I would be the person who can fix both media2 extention and AndroidX media2 as well.
Thank you for your investigation and I'll take this and the other issue(#8047) with priority.

@SungsooLim
Copy link

This issue is fixed in AndroidX media2, so the actual fix in media2-extension would take time until the next release of media2.

@SungsooLim
Copy link

AndroidX media2 1.1.0 was release, and expect this issue is resolved in the next release of media2-extension.

@georgi-neykov-hub
Copy link
Author

georgi-neykov-hub commented Dec 14, 2020 via email

icbaker pushed a commit that referenced this issue Dec 14, 2020
Issue: #8011
#minor-release
PiperOrigin-RevId: 347288689
icbaker pushed a commit that referenced this issue Jan 11, 2021
@google google locked and limited conversation to collaborators Feb 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants