1

I'm getting a very strange error. I have the following code:

while (true) {
    System.out.println(model.getLightState());
    if (model.getLightState() == 1) {
        System.out.println("Entered..");
        view.driveThroughJunction();
        break;
    }
}

Now, my program enters the if statement when the light state changes to '1' and executes the code that's fine. But this ONLY works if I have that print out statement after the while loop is entered. I find that weird. Will a 'sysout' line have any effect on the if statement? Apparently it does for the above case.

Any ideas to why this is ?

EDIT:

(in the model class)
public final byte getLightState() {
    return lightChanger.getLightValue();
}

(in lightchanger class)
public byte getLightValue() {
    return light.getState();
}

(in the light class)
public final byte getState() {
    return this.state;
}
Luiggi Mendoza
  • 85,076
  • 16
  • 154
  • 332
Force444
  • 3,321
  • 9
  • 39
  • 77

2 Answers2

5

You have a synchronization problem, leading to a visibility problem. If no synchronization is used when reading and writing a variable shared by multiple threads, there is no guarantee that the value written by one thread is visible to other threads.

To fix that you need to do one of those things:

  • make the state field volatile
  • change the state field type to AtomicInteger
  • synchronize every access to the state field (using the synchronized keyword), using the same lock every time, of course.

Your sysout call makes the value visible because it internally calls some method that flushes the state of the registers to the main memory (by writing to a volatile field or calling a synchronized method, for example).

JB Nizet
  • 678,734
  • 91
  • 1,224
  • 1,255
2

You should declare your model instance as volatile.

Volatile keyword in Java is used as an indicator to Java compiler and Thread that do not cache value of this variable and always read it from main memory. So if you want to share any variable in which read and write operation is atomic by implementation you should declare them as volatile variable.

Read more: http://javarevisited.blogspot.com/2011/06/volatile-keyword-java-example-tutorial.html#ixzz2DFw4l8AW

From your comments it is apparent that more than one thread is accessing the same instance of model and performing read/write in the same instance of model. Without volatile threads do cache the value of model instance and hence end up in dirty reads. With System.out.println happening your read/write operation is delayed by a bit (due to output writing) hence thread is able to refresh the cached value from main memory.

anubhava
  • 761,203
  • 64
  • 569
  • 643
  • Making model volatile is not going to solve the issue - he is accessing another variable. If it does solve the issue it is pure coincidence. The other answer is correct. – assylias Nov 25 '12 at 17:50
  • May be you can explain this `pure coincidence` theory better. And btw other answer also recommends using volatile variable. – anubhava Nov 25 '12 at 17:56
  • Making `model` volatile will only provide a guarantee that if you write `model = new Model();` it will be visible by other threads. It does not guarantee that changes to its fields will be visible. In other words, it gives no guarantee that `model.state = someNewState;` will be visible from another thread. The other answer recommends using volatile on the `state` variable. – assylias Nov 25 '12 at 17:59