3

I'm trying to understand this android implementation, but i just can't figure out why static field lockStatic is volatile, since its use is only through getLock method and it is a synchronized block, so there should be no concurrency or object publishing issues.

abstract public class WakefulIntentService extends IntentService {
    abstract protected void doWakefulWork(Intent intent);
    static final String NAME=
      "com.commonsware.cwac.wakeful.WakefulIntentService";
    static final String LAST_ALARM="lastAlarm";

    private static volatile PowerManager.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);
    }

The way i see there would be only the need for volatile if lockStatic was allowed access in any other way, but its private and only method wich deals with it is the getLock method. Is this just an assertion or there is other reasons behind it ?

Thanks in advance for any help resolving this question.

WeakfullIntentService

ZhongYu
  • 19,446
  • 5
  • 33
  • 61
Edu G
  • 1,030
  • 9
  • 23
  • 4
    I'm pretty sure I did it that way in support of a double-checked-locking implementation. However, in general, asking "why did Developer X do Thing Y?" is not great for Stack Overflow. The only person who can answer definitively is Developer X. In this case, I am Developer X, and I wrote that code several years ago. Nobody else can answer definitively. – CommonsWare Nov 22 '16 at 20:28
  • 2
    @CommonsWare It is kinda neat when the author of major framework code shows up to explain, though. – chrylis -cautiouslyoptimistic- Nov 22 '16 at 20:31

1 Answers1

2

In the code sample, volatile is not needed, because the variable is only accessed in that synchronized block.

However, synchronized might be a little too expensive, especially considering that majority invocations to that method simply read the variable and return. It feels too heavy to surround every read with synchronized. Therefore we have the so-called double-checked pattern, which avoids synchronized most of times

if(var==null)
    synchronized(lock)
        if(var==null)         // double-check
            var = something
return var

(OP's code sample however is not double-checked locking; it's all-time locking.)

In doubled-checked locking, usually var should be volatile -- but not always. If the object assigned to it is primitive (like int) or immutable (like String), non-volatile is OK.

And there are more situations where non-volatile is OK. This requires careful evaluations of the use cases. For example, all utility classes in java.util.concurrent (e.g. ReentrantLock) are designed in a way so that they will survive "unsafe publication". When they are used in double-checked locking pattern, they don't have to be volatile. Seeing the source code of WakeLock, I believe it doesn't need volatile either in double-checked locking.

Of course, this is a little too much. So, unless you know exactly what you are doing, play it safe and use volatile. A volatile read isn't that much expensive over an ordinary read.

ZhongYu
  • 19,446
  • 5
  • 33
  • 61
  • 1
    Also note that "volatile" does not provide the memory visibility guarantees on all versions of Android (particularly older Dalvik VM implementations) as Android does not adhere to Java Memory Model. I got burned by that in the past as volatile keyword was essentially ignored by the VM, and it's best to use "AtomicReference" or other java.util.concurrent AtomicXXX classes if you need volatile semantics on Android. – GameSalutes Nov 22 '16 at 22:04
  • @GameSalutes - that sucks:) I woulda thought that google does a better job than Sun given how much money and talent it has. – ZhongYu Nov 22 '16 at 22:32
  • Indeed. At least the atomic classes provide a contract that implementors must adhere to, and I've used them with success on Android and have not used volatile since. Here is a thread that discusses it a bit: http://stackoverflow.com/questions/6973667/dalvik-vm-java-memory-model-concurrent-programming-on-android – GameSalutes Nov 22 '16 at 22:36