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

Oreo Background Service Crash #157

Merged
merged 7 commits into from
Jun 15, 2018
Merged

Conversation

electrostat
Copy link
Contributor

With developers targeting Api 26+ now, background service no longer works and is creating a crash. Reworking system to prevent this crash and be Oreo optimized.

- update support library version
- add architecture lifecycle dependencies
- create and integrate ApplicationLifecycleObserver
- start and kill service when moving between background and foreground
@@ -488,7 +490,12 @@ private PermissionCheckRunnable obtainPermissionCheckRunnable() {
}

private void startLocation() {
applicationContext.startService(obtainLocationServiceIntent());
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
applicationContext.startForegroundService(obtainLocationServiceIntent());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could additionally ensure that the service is not started from the background by wrapping this line with ProcessLifecycleOwner.get().lifecycle.currentState.isAtLeast(Lifecycle.State.STARTED).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

starting a long- lived foreground service should only be done when absolutely necessary. It will trigger the OS (Android 8) to show a notification "Application X" is consuming your battery.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@florianPOLARSTEPS that is correct, but I am killing the service when the app enters the background to avoid this issue. When returning to the foreground, the service is spun up again and continues collecting and processing event data.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@electrostat Alright, thanks... I just noticed the LifecyleObserver, sorry about that.

- check to see if device is in background before starting service and lifecycle monitoring

private void initiateForegroundService() {
Notification notification = new Notification();
startForeground(1375, notification);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this result in showing a notification to the end user?

If your service is started (running through Context.startService(Intent)), then also make this service run in the foreground, supplying the ongoing notification to be shown to the user while in this state.

also:

Apps targeting API Build.VERSION_CODES.P or later must request the permission Manifest.permission.FOREGROUND_SERVICE in order to use this API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the above comment, I actually disable the foreground service when the application enters the background, thus removing the notification.

The Android P permission is a new one to me. Haven't looked at the documentation for that yet, but will give it a look. I'm planning on being P compliant much earlier than we are getting to Oreo.

@LukasPaczos
Copy link
Contributor

What's the advantage of using a foreground service in this context? Couldn't we use a background, sticky service? How sensitive is the event processing that it can't be interrupted?

@electrostat
Copy link
Contributor Author

@LukasPaczos Thanks for bringing up this line of thought. I did some digging and realized I made a mistake in my interpretation of the documentation. I read it as removing the startService functionality all together, not just in the background. So they were forcing everyone over to using a foreground service. That's incorrect. We can't start a service while in the background through this method and need to use startForeground service instead. Given this new insight, your earlier suggestion is likely all we need to fix this issue.

Our system is robust and can be interrupted freely, so no worries there. My original solution would've faced the same issues.

I will make the change and test accordingly.

- move maven repo
- remove lifecycle observer
- check if app is in foreground before starting service
- remove all code related to lifecycle observer
- made foreground check only valid for oreo and up environments
- additionally fixes failing test
@LukasPaczos
Copy link
Contributor

I haven't tested it, but based on the Maps SDK setup if a user calls Mapbox#getInstance (effectively initializing telemetry) from the Application#onCreate, or #onCreate of the first Activity (based on ProcessLifecycleOwner docs) the app's process won't be in a Lifecycle.State.STARTED state yet. This means, that we need to keep lifecycle components around to start the service when we eventually do reach that state on Oreo devices, is that right or am I missing something here?

okhttp3 : '3.10.0',
gson : '2.8.2',
espressoVersion : '3.0.1',
archLifecycleVersion: "1.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be any benefits to bumping arch components version? I think 1.1.1 requires support lib 27+, but maybe we could use 1.1.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope 1.1.1 is good with supportLib of 26.1.0

- bump lifecycle dependency to 1.1.1
- make MapboxTelemetry a LIfecycleObserver
- if oreo and not yet "STARTED" will listen for app to be in the foreground then create service (and kill listener)
Copy link
Contributor

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor request, otherwise looks great 🚀

@@ -33,6 +34,10 @@ ext {
// play services
gmsLocation : "com.google.android.gms:play-services-location:${version.gmsLocation}",

//architecture components
archLifecycleExtensions: "android.arch.lifecycle:extensions:${version.archLifecycleVersion}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can save some weight here by using android.arch.lifecycle:runtime:${version.archLifecycleVersion} dependency instead of extensions as we are not using ViewModel or LiveData.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capturing from a chat that it seems like runtime does not contain ProcessLifecycleOwner so we need to stick to extensions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@electrostat any good reason why @LukasPaczos suggestion to cut on weight was not addressed and PR still got merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrlee Yes, per the above comment we discussed that ProcessLifecycleOwner' is not included in runtime, so we had to keep extensions`. We tested and discussed over slack the issue and was just captures in the brief above comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could still exclude livedata and viewmodel right? We shouldn't be including unnecessary deps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants