Fix #1377: app process not stopping #1447
Conversation
…ng even with crashes and errors
|
@mpivchev is this PR ready to be tested? Not sure why you added HockeyApp code (RNHockeyAppUsageTracker) to the lib? |
Hi, it's not fully ready, hence why it's still a draft. Use at your own risk. That file is used for experimenting and will be removed in final release. |
…-stopping # Conflicts: # android/build.gradle # android/src/main/java/com/doublesymmetry/trackplayer/service/MusicService.kt # example/android/build.gradle
|
I know it's still WIP, but I'm getting the below crash: |
android/build.gradle
Outdated
| @@ -49,14 +49,15 @@ repositories { | |||
| } | |||
|
|
|||
| dependencies { | |||
| implementation 'com.github.DoubleSymmetry:KotlinAudio:v0.1.31' | |||
| implementation "com.github.doublesymmetry:kotlin-audio:0.1.35" | |||
| // implementation "com.github.DoubleSymmetry:KotlinAudio:v0.1.33" | |||
There was a problem hiding this comment.
non-blocking: We can delete this right?
There was a problem hiding this comment.
I think this needs to be reverted as this is pointing the RNTP dependency at the locally built KotlinAudio project.
| @@ -1,9 +1,10 @@ | |||
| package com.example; | |||
|
|
|||
| import android.os.Bundle; | |||
There was a problem hiding this comment.
non-blocking: Leftover import?
| ) | ||
| is NotificationState.CANCELLED -> stopForeground(true) | ||
| is NotificationState.POSTED -> { | ||
| if (!isForegroundService) { |
There was a problem hiding this comment.
non-blocking: What does this isForegroundService flag do? Makes sure we only start once? Then we should use a different name like hasStartedForegroundService or something.
There was a problem hiding this comment.
I think both names work, since only when we call startForeground, does the service become foreground.
| } | ||
|
|
||
| override fun onHeadlessJsTaskFinish(taskId: Int) { | ||
| // Overridden to prevent the service from being terminated |
There was a problem hiding this comment.
Ok, so we don't use this anymore to prevent the service from terminating? 🌟
There was a problem hiding this comment.
We use START_STICKY to prevent the service from terminating instead of this. The former is the proper way of keeping a service alive in Android.
| callback.resolve(null) | ||
| } | ||
|
|
||
| @ReactMethod | ||
| fun play(callback: Promise) { | ||
| if (verifyServiceBoundOrReject(callback)) return | ||
| // if (verifyServiceBoundOrReject(callback)) return |
There was a problem hiding this comment.
blocking: why did we remove all of these checks? users might still try to perform an action before they call setupPlayer and it should throw.
| if (verifyServiceBoundOrReject(callback)) return | ||
|
|
||
| musicService.removeUpcomingTracks() | ||
| callback.resolve(null) | ||
| } | ||
|
|
||
| @ReactMethod | ||
| fun skip(index: Int, callback: Promise) { | ||
| if (verifyServiceBoundOrReject(callback)) return | ||
|
|
||
| musicService.skip(index) | ||
| callback.resolve(null) | ||
| } | ||
|
|
||
| @ReactMethod | ||
| fun skipToNext(callback: Promise) { | ||
| if (verifyServiceBoundOrReject(callback)) return | ||
| musicService.skipToNext() | ||
| callback.resolve(null) | ||
| } | ||
|
|
||
| @ReactMethod | ||
| fun skipToPrevious(callback: Promise) { |
There was a problem hiding this comment.
A lot of these functions are called from the notification buttons, which happens when the service is unbound. We should not have these checks here, or the notification buttons would not work if the app is in recents (and unbound).
| override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int { | ||
| startTask(getTaskConfig(intent)) | ||
| return START_STICKY | ||
| } | ||
|
|
There was a problem hiding this comment.
We overwrite the JSTaskService here, and returning START_STICKY, which causes the service to try and recover itself if it dies for any reason.
| destroyIfAllowed() | ||
| override fun onActivityStopped(activity: Activity) { | ||
| // Service MUST be unbound during activity onStop | ||
| unbindService() |
There was a problem hiding this comment.
Unbinding seems to kill the service automatically. Usually this is prevented by calling ContextCompat.startForegroundService to manually start the service (which we already do below), but it seems like it still doesn't work.
|
@mpivchev Do you think this PR solves all crashes we're seeing on Samsung devices?
|
# Conflicts: # android/build.gradle # android/src/main/java/com/doublesymmetry/trackplayer/module/MusicModule.kt # android/src/main/java/com/doublesymmetry/trackplayer/service/MusicService.kt # example/App.tsx # example/android/app/src/main/java/com/example/MainActivity.java
|
We are changing how the audio service handles its lifecycle. Previously we had the
@afkcodes cc |
|
This is cool @mpivchev no words for the effort you have put in and the amount of research, just a few question & observation that i have.
|
|
Hi @mpivchev, we tried your changes because we also have the same problem as you. We encountered a problem when trying to play audio and the app crashes with the below error.
|
|
@nicomontanari This PR is still a WIP so crashes are expected, @mpivchev is working hard to get this through, lets hope for the best. |
|
Tested on 2 devices and experienced no crashes. Does this crash also happen on other branches/commits? |
It's not a WIP anymore since it's ready for review :) |
|
Awesome looking forward. |
Hi @mpivchev I have the same issue, I think it is a problem with Exoplayer, but with the stable version it never happens (v2.2.0-rc4) |
|
@filippobusi What android version and device did this happen on? |
Android 10, Xiaomi Mi 8 |
|
@filippobusi can you try testing on a non-xaomi device and/or a simulator? |
| player.stop() | ||
| player.destroy() |
There was a problem hiding this comment.
so we destroy the player and it gets recreated by setupplayer again?
There was a problem hiding this comment.
This function should only be called in onDestroy of the service, but I see we also call it in reset react method. Do we need reset at this point? It's a very ambiguous function too, since we don’t really know what exactly we want to reset, but we are calling random functions in both iOS and Android in it.
in case of Android, it will break the service since we are indeed destroying the player with no way of recreating it. It's supposed to never get destroyed until the service dies:
@ReactMethod
fun reset(callback: Promise) = scope.launch {
if (verifyServiceBoundOrReject(callback)) return@launch
musicService.stopPlayer()
callback.resolve(null)
}| } | ||
|
|
||
| override fun onHeadlessJsTaskFinish(taskId: Int) { | ||
| // Overridden to prevent the service from being terminated |
| @@ -9,7 +9,7 @@ export const SetupService = async (): Promise<boolean> => { | |||
| } catch { | |||
| await TrackPlayer.setupPlayer(); | |||
| await TrackPlayer.updateOptions({ | |||
| stopWithApp: false, | |||
| stoppingAppPausesPlayback: true, | |||
There was a problem hiding this comment.
we need to update the typescript sources as well to remove the old key and add this one.
There was a problem hiding this comment.
Looks like someone has done that here: #1625
There was a problem hiding this comment.
we should provide some sort of alias that maps stopWithApp to stoppingAppPausesPlayback to prevent a breaking change.
There was a problem hiding this comment.
We are introducing other breaking changes, like removing the destroy method. We originally agreed to not introduce breaking changes in 2.X, but in this case I think it's inevitable, unless we want to deprecate everything and just confuse people on why destroy is not doing anything.
There was a problem hiding this comment.
This is a tough one.. we considered it but its quite misleading -- although it would kind of prevent a breaking change. We also removed some methods that no longer make sense like destroy. We could also add TS stubs for them that don't actually do anything to keep compatibility but at that point its very much like "silently failing" vs actually keeping compatibility.
I'm up for either or.. but not sure what's the right thing to do here. Either define a breaking change or silently stub out most of the missing parts. If any app depends on current behaviour though it will still break.
There was a problem hiding this comment.
I think it's ok to define breaking changes for this PR, some things in this rewrite became bigger than we thought and at this point having no breaking changes is impossible IMO. Of course we should still strive to have as few as possible breaking changes until 3.0, but sometimes exceptions must be made.
There was a problem hiding this comment.
I think this is the best as well.
There was a problem hiding this comment.
I'm fine with introducing breaking changes, but if we do so I'd vote that we bump the major version, and make the next release a 3.0.0-rc.0 or something.
| @@ -51,14 +51,15 @@ repositories { | |||
| } | |||
|
|
|||
| dependencies { | |||
| implementation 'com.github.DoubleSymmetry:KotlinAudio:v0.1.34' | |||
| implementation "com.github.DoubleSymmetry:KotlinAudio:v0.1.34" | |||
| // implementation "com.github.doublesymmetry:kotlin-audio:0.1.36" | |||
There was a problem hiding this comment.
I think it's ok if it stays, since we almost always need to replace remote with local KA.
| @@ -51,14 +51,15 @@ repositories { | |||
| } | |||
|
|
|||
| dependencies { | |||
| implementation 'com.github.DoubleSymmetry:KotlinAudio:v0.1.34' | |||
| implementation "com.github.DoubleSymmetry:KotlinAudio:v0.1.34" | |||
There was a problem hiding this comment.
so which version of KA do we want to use?
There was a problem hiding this comment.
This PR is using remote 0.1.34
|
@mpivchev I've tested on Nexus 5X Android 8.1.0 and on emulator with Android 12 but nothing changes, the ANR is still here. |
…sources and documentations (#1625)
|
Hi @mpivchev, ANR it seems caused by foreground service. I think the command startForeground() is not launched within 10s. But if I use this code inside setupPlayer in MusicModule.kt the ANR not appear. @ReactMethod
fun setupPlayer(data: ReadableMap?, promise: Promise) {
...
Intent(context, MusicService::class.java).also { intent ->
context.startService(intent)//<---- add this
context.bindService(intent, this, Context.BIND_AUTO_CREATE)
//ContextCompat.startForegroundService(context, intent) <--- remove this line
}
} |
Thanks for the hint. We don't need to start a foreground service in the module, since the service itself tries to start itself as foreground internally. Trying to do it twice is a bad idea. I implemented your change so please update if you still get more ANRs. |
|
We realized we can no longer keep updating the project without breaking changes, so this and future PRs will now be part of version 3.0 of RNTP. |
This PR is aimed to fix the bug where the app process would not die even when the app + service were killed.
The cause of this bug is not concrete, and a lot of the issues here are very strange, but I do have some theories:
The good news is it did cause the process to die properly, but it introduced other bugs related to notifications (services are heavily tied to Android notifications). More info in this separate PR.
Here is how the current implementation of this PR: