Thread switch/case ignoring statement

Question

I'm just testing some concurrent programming in Java. Basically I have a class (Light) which is a kind of finite state machine, and changing its state regarding the commands. That's what I'm trying to: The light is in ON state, I send a command to the thread of this class for changing the state in OFF. But I got a problem during the execution.

First, let me present the class:

enum State {ON, OFF}; 

public class Light implements Runnable {

private boolean cmdOn;
private boolean cmdOff;
State state;

public Light() {
    cmdOn = false;
    cmdOff = false;
    state = State.ON;
}
@Override
public void run() {
    while(true) {
        switch(state) {
        case ON:
            if(cmdOff) {
                try {
                    Thread.currentThread().sleep(500);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
                state = State.OFF;
            }
            break;

        case OFF:
            if(cmdOn) {
                try {
                    Thread.currentThread().sleep(500);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
                state = State.ON;
            }
            break;
        }


    }
}
public void setCmdOn(boolean cmdOn) {
    this.cmdOn = cmdOn;
}
public void setCmdOff(boolean cmdOff) {
    this.cmdOff = cmdOff;
}
public State getState() {
    return state;
}
}

And my main class:

public class Main {

public static void main(String args[]) throws InterruptedException {
    Light light = new Light();
    Thread t = new Thread(light);
    t.start();
    printState(light, 500, 1);
    light.setCmdOff(true);
    printState(light, 500, 4);

}
public static void printState(Light l, int time, int number) throws InterruptedException {
    for(int i= 0; i < number; i++) {
        System.out.println(l.getState());
        Thread.currentThread().sleep(time);
    }
}

The output shows me that I'm stuck in the ON state while I should be in OFF state.

In a second run, after putting an instruction (System.out.println or whatever...) above the if statement which verify that cmdOff is true, it's magically works.

I don't understand why the cmdOff variable is not pass to true during the first run !? And why in the second run it works? I miss something, probably a synchronizing block. But I don't find the explanation to deal with this.

Thanks.

Best regards,

Can you please explain why you have three variables indication whether or not the light is on or off (`cmbOff`, `cmbOn`, `state`)? — stealthjong, Aug 19 '14 at 12:57
Of course. That's not a design issue. This is just a simplify example, normally my class have several state and considering several input as well. If you are familiar with finite state machine, that's exactly what I'm implementing. — Poke, Aug 19 '14 at 13:01
The attempted communication between your threads is not *thread safe*. You have not implemented *anything* that can be used for thread safe communication. So I guess you don't know anything about threads safety. That means that any answer that *really* tells you what you need to know will have to explain thread safety to you. That is too much for an answer on SO. — Raedwald, Aug 19 '14 at 13:15
@Raedwald As far as I understand, and from what I read, dealing with thread safe is not a requirement when implementing thread. IMO, it depends of how you will _use_ your thread and even more, how you will _design_ your different piece of software. In my case, it seems to be "overkill" to keep my class thread-safe, as long as only one thread will modify one of the boolean variable. Your comment is relevant but it doesn't apply in this particular context. — Poke, Aug 19 '14 at 19:12

John Humphreys · Accepted Answer · 2014-08-19T13:19:00.443

1

You should read about synchronization. Without synchronization you risk getting visibility errors where one thread can't see changes another thread made to a variable shared between the threads.

Tutorial: http://docs.oracle.com/javase/tutorial/essential/concurrency/sync.html

You can use a synchronized block that uses an object both threads know about to do locking. If both threads always synchronize on that known object when reading or updating your shared data, then visibility and atomicity will never be an issue.

Read here to fully understand "synchronized": http://tutorials.jenkov.com/java-concurrency/synchronized.html You should also be able to just declare the shared variable as volatile. This means all writes and reads on it create a happens-before relationship with other threads, which is what you want. Read the tutorial above to fully understand the issues and terminology.

Read here to fully understand "volatile": http://docs.oracle.com/javase/tutorial/essential/concurrency/atomic.html.

edited Aug 19 '14 at 13:19

answered Aug 19 '14 at 12:58

John Humphreys

37,047
37
155
255

Thanks for your answer and the links, for sure it will be very useful. Also, a second question, why adding an instruction made the execution correct although my variable were **not volatile** ? – Poke Aug 19 '14 at 13:09
You should understand that after the tutorials. But suffice to say that concurrency (multi-threading) errors are pretty erratic. You may have code that works for years and suddenly hits a bug. Very crazy things can happen, like if you set two variables without synchronization, A then B, the other thread may see B change before A even though it was changed after A. The real take-home here is just that once you mess up synchronization, you can make no assumptions about your shared variables in your program. Adding that line and fixing it is just bad luck (confusion) and is not re-usable. – John Humphreys Aug 19 '14 at 13:17
PS: you should note the "reading or updating" change I made. You nee to synchronize every time you use the variable, not just when changing it. That's how you can be sure changes will always be properly seen by all threads. – John Humphreys Aug 19 '14 at 13:19
1

Just an additional link that I found: http://stackoverflow.com/questions/10324272/why-is-it-not-good-practice-to-synchronize-on-boolean . As I'm improving my code, `AtomicBoolean` fits perfectly for my need. – Poke Aug 21 '14 at 14:18

score 1 · Answer 2 · edited May 23 '17 at 10:33

1

Try using volatile on cmdOn and cmdOff:

private volatile boolean cmdOn;
private volatile boolean cmdOff;

Volatile variable explanation in Java docs

Without it (or synchronization) changes may not be visible.

edited May 23 '17 at 10:33

Community

1
1

answered Aug 19 '14 at 12:58

Michał Schielmann

1,372
8
17

score 0 · Answer 3 · answered Aug 19 '14 at 12:58

Without any synchronization, there are no guarantees that the running thread will ever see the values written to cmdOff and cmdOn by the other thread. Also, lack of synchronization on state means any changes by the running thread may not be seen by the other thread. Try making cmdOn, cmdOff and state volatile .

Thread switch/case ignoring statement

3 Answers3