CODEWITHSUNDEEP
androidandroid-serviceintentservicecommonsware-cwac
Mr_and_Mrs_D
Mr_and_Mrs_D

Reputation: 34026

WakefulIntentService implementation clarifications

Commonsware's WakefulIntentService works beautifully but there are some things I do not quite get. Below is the core of the service - a stripped down version of the source :

class WIS extends IntentService {

    private static final String NAME = WIS.class.getName() + ".Lock";
    private static volatile WakeLock lockStatic = null;

    synchronized private static PowerManager.WakeLock getLock(Context context) {
        if (lockStatic == null) {
            PowerManager mgr = (PowerManager) context
                    .getSystemService(Context.POWER_SERVICE);
            lockStatic = mgr.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, NAME);
            lockStatic.setReferenceCounted(true);
        }
        return (lockStatic);
    }

    public static void startWIS(Context ctxt, Intent i) {
        getLock(ctxt.getApplicationContext()).acquire();
        ctxt.startService(i);
    }

    public WIS(String name) {
        super(name);
        setIntentRedelivery(true);
    }

    @Override
    public int onStartCommand(Intent intent, int flags, int startId) {
        PowerManager.WakeLock lock = getLock(this.getApplicationContext());
        if (!lock.isHeld() || (flags & START_FLAG_REDELIVERY) != 0) { // ?
            lock.acquire();
        }
        super.onStartCommand(intent, flags, startId);
        return (START_REDELIVER_INTENT);
    }

    @Override
    protected void onHandleIntent(Intent intent) {
        try {
            // do your thing
        } finally {
            PowerManager.WakeLock lock = getLock(this.getApplicationContext());
            if (lock.isHeld()) lock.release();
        }
    }
}

Questions

Upvotes: 5

Views: 4545

Answers (2)

Creos
Creos

Reputation: 2525

Sorry don't have enough rep to comment but it looks like you have a race condition with the two checks if(held)release and if(!held)acquire. I.e. making the ref volatile is not enough to guard you against races.

These are composite statements invoked on different threads. You likely want to enter a sync block on a private final Object lock = new Object() for those two checks so they are done atomically. Very corner-y but thought I'd mention it. Let me know if you disagree. Thanks.

Upvotes: 1

CommonsWare
CommonsWare

Reputation: 1006724

AFAIK a process killed takes its locks with it.

Correct.

Or is this an impossible scenario ?

It's fairly unlikely, but certainly not impossible.

In view of the previous should (flags & START_FLAG_RETRY) be added ?

That should be covered by START_FLAG_REDELIVERY. AFAIK, with START_REDELIVER_INTENT, there is no RETRY without REDELIVERY. If you have evidence to the contrary, I'd love to see it.

Why the if (!lock.isHeld()) check ?

Calling release() on a WakeLock that is not held results in an exception. This is just a safety blanket to ensure we don't wind up throwing an unnecessary exception. In theory, it should never be needed; in theory, I should have hair.

Why is this.getApplicationContext() needed ? is not this enough ?

We create a WakeLock, which we hold in a static data member. Probably the getSystemService() call does not wind up putting the Context that called it inside the PowerManager. And, even if it did, probably the Context would not be passed to the resulting WakeLock instance. However, to be safe, by using getApplicationContext(), we obtain the WakeLock in a fashion that ensures that the only Context we could possibly "leak" is the singleton application context, which, as a singleton, is effectively pre-leaked. :-)

Upvotes: 6

Related Questions