Leaking services is scary easy

[Mike Hearn]

I've got kind of concerned lately at how many apps appear to leak
services, that is, start them with an intent and then never properly
stop them.

I don't want to point fingers, but I've just filed a bug against one
open source library that starts a service and doesn't actually contain
a call to stopSelf or stopService anywhere within it. It uses the
service for the questionable purpose of keeping a process alive during
an HTTP submission. But it's not only one app - it seems whenever I
run "ps" on my phone I see processes running for apps I haven't used
in a long time, because there's a service running inside them.

How services work is clearly stated in the documentation. This is not
a fault of Android. Still, there are a few reasons this mistake is
easy to make. I propose possible fixes below.

1) The first thing is, the consequences of bad lifecycle management
aren't really spelled out in the docs. It *should* be obvious, but
apparently isn't. Calling out with a big red stop sign that failing to
pair a call to startService with a stopSelf or stopService call will
*degrade somebodies phone until next reboot* might make people pay
attention.

2) Service lifetime is infinite once started. I suspect (but can't
prove) that many uses of services don't actually require infinite
lifetimes.

3) The fact that services can be oom killed at any time means proper
resumability is essential. I suspect (but again can't prove) that a
common bug is a service which starts some work on a thread, then when
the thread is done it stops the service. But if the services process
is oom killed, the thread will die, onCreate() will be called but
onStart() never will, and the service will permanently leak. The docs
say

"you may want to write information about that work into persistent
storage during the onStart() call so that it does not get lost if the
service later gets killed."

The use of "may" makes it sound like an optional nice to have, whereas
in fact failure to do this is likely to lead to a service leak and
consequent degradation of the users phone. In my mind that makes it
essential.


The first problem can be fixed with some simple HTML callouts in the
documentation.

The second problem could be fixed by deprecating the current "public
void onCreate()" callback and replacing it with "public int onCreate
()" (or whatever the nicest backwards compatible solution is). The
return value would be a deadline in seconds. After that deadline
passes, onDeadlineExpiry() would be called which can return another
deadline, or the default implementation returns zero indicating no
extension. Making an infinite service then just requires you to return
a large, static value from these functions ..... but it'd be explicit,
and hopefully service authors would consider whether they really need
an infinite deadline, or whether setting a long but none-infinite
deadline would be just as good. For instance submitting something to a
server shouldn't take longer than 30 seconds. If it does then it's
probably worth giving up.

The final problem is the hardest to fix. It could maybe be solved with
a higher level wrapper around the Service API, which handles queuing
of work items received via intents, persistence, checkpointing, and
correct resume/shutdown. Kind of like how AsyncTask abstracts you from
the details of UI threading.

[Mark Murphy]

Mike Hearn wrote:
> The final problem is the hardest to fix. It could maybe be solved with
> a higher level wrapper around the Service API, which handles queuing
> of work items received via intents, persistence, checkpointing, and
> correct resume/shutdown. Kind of like how AsyncTask abstracts you from
> the details of UI threading.

Check out IntentService, new to Android 1.5. If it offered WakeLock
integration, it'd be excellent for the AlarmManager pattern of periodic
service operation.

--
Mark Murphy (a Commons Guy)
http://commonsware.com | http://twitter.com/commonsguy

Need Android talent? Ask on HADO! http://wiki.andmob.org/hado

[Fred Grott]

May I suggest a short gap solution?

Seems liek a few of us could get together and compose some new PMD rules to cover this and other issues and submit back to PMD project.

I do know some app devlopers do use that new PMD ruleset that has Android code rules in it

[Mike Hearn]

> Check out IntentService, new to Android 1.5. If it offered WakeLock
> integration, it'd be excellent for the AlarmManager pattern of periodic
> service operation.

That's the sort of thing I had in mind, yep. It'd be nice if it was
referenced from the Service documentation.

It doesn't handle persistence though. If your service is killed whilst
it's handling an intent, the service will still leak AFAICT.

[Mark Murphy]

Mike Hearn wrote:
> It doesn't handle persistence though. If your service is killed whilst
> it's handling an intent, the service will still leak AFAICT.

?

If Android kills the process, you're gone. Dead processes leak no RAM.

[Mike Hearn]

> > It doesn't handle persistence though. If your service is killed whilst
> > it's handling an intent, the service will still leak AFAICT.
>
> ?
>
> If Android kills the process, you're gone. Dead processes leak no RAM.

No, that's not correct. Service processes can be and often are OOM
killed by the kernel, then restarted 5 seconds later by the activity
manager. When that happens the new service receives onCreate() but not
onStart(). Thus if you don't have a codepath that ensures the service
is eventually stopped from onCreate() you effectively "leak" an entire
process, ie, leave it around in a state such that Android will never
let it shut down.

Just logcat a phone which has been running for a week or so with a lot
of apps on it, I think you'll be surprised at how often service
processes are killed and restarted despite it being days since you
last used the program. At least, I always am surprised.

[Mark Murphy]

Mike Hearn wrote:
> No, that's not correct. Service processes can be and often are OOM
> killed by the kernel, then restarted 5 seconds later by the activity
> manager. When that happens the new service receives onCreate() but not
> onStart(). Thus if you don't have a codepath that ensures the service
> is eventually stopped from onCreate() you effectively "leak" an entire
> process, ie, leave it around in a state such that Android will never
> let it shut down.

Ah, now I understand what you're referring to. Pardon my confusion.

--
Mark Murphy (a Commons Guy)
http://commonsware.com | http://twitter.com/commonsguy

Android App Developer Books: http://commonsware.com/books.html

[Mike Hearn]

> May I suggest a short gap solution?
>
> Seems liek a few of us could get together and compose some new PMD rules to
> cover this and other issues and submit back to PMD project.
>
> I do know some app devlopers do use that new PMD ruleset that has Android
> code rules in it

Cool, I've never heard of PMD and didn't know there was an Android
ruleset for it. I'm completely in favor of static analysis to catch
problems like this. It'd be cool if the Android SDK could ship
FindBugs and PMD with the Android ruleset. The current PMD Android
rule set looks pretty spartan, but it looks easy to extend too. Thanks
for the suggestion!