0

Ok, this is a question I didn't think needed asking but either its the long hours or whatnot, but my mind is foggy so here it goes.

Creating a class with a HandlerThread, Looper, and Handler:

public class MyClass {
    //private volatile boolean mRunning   = false;
    private boolean mRunning              = false;
    private HandlerThread mHandlerThread  = null;
    private Handler mMessageHandler       = null;

    public static final int MESSAGE_START = 1;
    public static final int MESSAGE_STOP  = 2;

    public MyClass() {
        mHandlerThread = new HandlerThread("com.test.myclass");
        mHandlerThread.start();
        mMessageHandler = new MessageHandler(mHandlerThread.getLooper());
    }

    private class MessageHandler extends Handler {
        public MessageHandler(Looper looper) {
            super(looper);
        }

        private void start() {
            mRunning = true;
        }

        private void stop() {
            mRunning = false;
        }

        @Override
        public void handleMessage(final Message msg) {
            try {
                switch (msg.what) {
                    case MESSAGE_START:
                        start();
                        break;

                    case MESSAGE_STOP:
                        stop();
                        break;

                    default:
                        throw new RuntimeException("Invalid message: " + msg.what);
                }
            } catch (RuntimeException e) {
                stop();
            }
        }
    }

    public void release() {
        if (isRunning()) {
            stop();
        }

        // PS: is this a good way to stop HandlerThead/Looper in API =< 17 ?
        if (mHandlerThread != null) {
            mHandlerThread.quit();
            mHandlerThread = null;
        }
    }

    // Should this be a synchronized method
    public boolean isRunning() {
        return mRunning;
        /**
         * Or should the variable be synchronized itself?
         * synchronized(mRunning) { return mRunning; }
         */

        // Or just use a semaphore?
    }

    public void start() {
        mMessageHandler.sendEmptyMessage(MESSAGE_START);
    }

    public void stop() {
        mMessageHandler.sendEmptyMessage(MESSAGE_STOP);
    }
}

So mRunning is accessed by both threads (main and looper). As such, the access should be synchronized. Which way would you choose? Making the variable volatile (so that both threads have up-to-date local values), making the isRunning method synchronized? Or accessing variable via synchronized(mRunning){...}?

Finally, implementation of release method, is that per standard? Or any other way you'd choose?

Vadym
  • 964
  • 1
  • 13
  • 31

1 Answers1

1

If contention for the lock is low, then synchronizing on a lock will cost about the same as a volatile access. If conention is high, then the volatile version probably will perform better.

You can't do this though:

synchronized(mRunning) { ... }  //ERROR!

mRunning is a boolean expression. You can't synchronize on a boolean value, you can only synchronize on an Object.

Solomon Slow
  • 25,130
  • 5
  • 37
  • 57
  • So from http://tutorials.jenkov.com/java-concurrency/volatile.html, it seems that making mRunning volatile should be ok because the HandlerThread is the one that writes (in start/stop) and main thread is the one that reads, correct? Otherwise, if both were writing and reading, I'd use AtomicBoolean. Alternatively, I could `synchronize` start/stop/isRunning (while still keeping volatile mRunning so that threads stay in sync), correct? Synchronization would happen on the instance, similar to synchronize(this) and would lock the three methods. – Vadym Feb 10 '15 at 01:28
  • The `get()` and `set()` methods of `AtomicBoolean` behave exactly like reading or writing a `volatile boolean`. It becomes more useful than `volatile` if you can make use of its `getAndSet()` and/or `compareAndSet()` methods. It's entirely up to you which one to use, but some people consider it good hygiene to always use AtomicWhatever instead of volatile whatever. – Solomon Slow Feb 10 '15 at 14:35