Does this concurrent legacy code needs volatile

Question

I'm struggling with some concurrent legacy code and wonder whether stopLatch and/or mode should be volatile:

public class MyClass {

    private final ExecutorService executor = Executors.newSingleThreadExecutor();
    private MyModeEnum mode = MyModeEnum.NONE;
    private CountDownLatch stopLatch;

    public synchronized void start(final MyModeEnum mode) {
        assert mode != null && mode != MyModeEnum.NONE;
        if (isRunning()) {
            // Throw an exception.
        }
        this.mode = mode;
        stopLatch = new CountDownLatch(1);
        executor.execute(() -> {
            try {
                // Pre and post operations not under control around stopLatch.await().
            } catch (final Exception e) {
                stop();
                // Further exception handling.
            } finally {
                MyClass.this.mode = MyModeEnum.NONE;
            }
        });
    }

    public synchronized void stop() {
        if (!isRunning()) {
            return;
        }
        stopLatch.countDown();
    }

    public boolean isRunning() {
        return mode != MyModeEnum.NONE;
    }

    public MyModeEnum getMode() {
        return mode;
    }

}

Elaborate explanation very appreciated.

EDIT: I wasn't able to boil down general questions/answers such as When exactly do you use the volatile keyword in Java? to this particular problem.

sync blocks have to synchronize across all threads when they're entered and exited, so I don't think that `volatile` is necessary. — khelwood, Feb 10 '17 at 16:30
I would say only `mode` needs to be `volatile`, as any change needs to be visible to other threads. Side note: that `assert` seems to do the same job as `isRunning()`, am I wrong? — sp00m, Feb 10 '17 at 16:33
`isRunning() ` is not `synchronized`, so `mode` should be `volatile ` — Mick Mnemonic, Feb 10 '17 at 16:34
@khelwood but, for example, the code within `executor.execute(...)` isn't synchronized anymore. I don't think this is safe. — beatngu13, Feb 10 '17 at 16:38
Possible duplicate of [When exactly do you use the volatile keyword in Java?](http://stackoverflow.com/questions/3488703/when-exactly-do-you-use-the-volatile-keyword-in-java) — Andremoniy, Feb 10 '17 at 16:43
@sp00m @MickMnemonic I thought of that, too. Are there're any noteworthy pros/cons making `isRunning()` `synchronized` compared with `mode` being `volatile`? — beatngu13, Feb 10 '17 at 16:44
http://stackoverflow.com/questions/3214938/java-volatile-modifier-and-synchronized-blocks — Andremoniy, Feb 10 '17 at 16:46
You have data races on `mode`. It is accessed from outside the sync block. First choice should be to synchronize `isRunning` and `getMode`; making mode `volatile` is a more advanced play. (Walk first, then fly.) — Brian Goetz, Feb 10 '17 at 16:51
@BrianGoetz, is the code within the anonymous lambda that's passed to `executor.execute()` `synchronized` in the same sense that the rest of the `start()` method is? That's the interesting question here, in my opinion. — Mick Mnemonic, Feb 10 '17 at 17:04
The call to `execute()` _happens-before_ the `Runnable` is executed, whether the execution happens in the local thread or off-thread. But there's no corresponding _happens-before_ between the completion of the `Runnable` and any action in the initiating thread (or any other thread.) — Brian Goetz, Feb 10 '17 at 17:08
@BrianGoetz @sp00m @MickMnemonic thanks for your suggestions regarding the `mode` field. Would anybody mind adding this as an answer so I can accept it? — beatngu13, Feb 10 '17 at 19:05
Code so young that it uses lambda expressions, but still already “legacy code”… — Holger, Feb 10 '17 at 19:21
@Holger I simplified the original code—written in Java 6— for SO. — beatngu13, Feb 10 '17 at 19:27
What’s the point of this `stopLatch.await()`? Is the main purpose of the background task to wait to be stopped by someone else? — Holger, Feb 10 '17 at 19:54
@Holger as far as I can tell it acts like a barrier for the post operations to be started. — beatngu13, Feb 10 '17 at 20:11
So `stop()` means “continue”? But why does the exception handler call `stop()`? It doesn’t do anything useful at the point. — Holger, Feb 10 '17 at 20:13

score 0 · Accepted Answer · edited Jun 20 '20 at 09:12

0

According to the most upvoted comments:

Mick Mnemonic:

isRunning() is not synchronized, so mode should be volatile.
Brian Goetz:

You have data races on mode. It is accessed from outside the sync block. First choice should be to synchronize isRunning and getMode; making mode volatile is a more advanced play. (Walk first, then fly.)

edited Jun 20 '20 at 09:12

Community

1
1

answered Feb 12 '17 at 10:58

beatngu13

7,201
6
37
66

Does this concurrent legacy code needs volatile

1 Answers1