Boolean in Multithreaded environment

Question

I have boolean field:

private boolean isReady = false;
private boolean isReady() {
  return isReady;
}

and I am using it inside two methods:

synchronized (topologyLock)
{
   try 
    {
      while(!instance.isReady()) 
      {
        topologyLock.wait();
      }
    } 
    catch (InterruptedException e)
    {               
        Thread.currentThread().interrupt();
    } 

private synchronized boolean topologyChanged()
{           
    synchronized(topologyLock)
    {
        isReady = true;
        topologyLock.notifyAll();
    }
}

I think that above code should work perfectly - or do I need to make this boolean variable volatile?

It doesn't need to be volatile if you're only accessing it from synchronized blocks. — shmosel, Jul 10 '17 at 09:02
@GhostCat using `synchronized` guarantees `happens-before` relationship, so there is no chance that a cached value would be used if **all** access is from within `synchronized` blocks. — Kayaman, Jul 10 '17 at 09:12
Unrelated: A) there is no point in having a private method to read a private field. Just dump that method. B) consider not using signal/wait on that low-level. Java has plenty of "high level" abstractions that help you not writing such code. — GhostCat, Jul 10 '17 at 09:13
@shmosel because it does contain some other code as well which requires lock as well . — TeamZ, Jul 10 '17 at 09:25
@GhostCat thanks for your useful comments . As it is a legacy code i don't want to take any chance so keeping it as it is. — TeamZ, Jul 10 '17 at 09:27

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

Quoting from stone ages:

So, when to make a variable volatile?

When you have a variable which can be accessed by many threads and you want every thread to get the latest updated value of that variable even if the value is updated by any other thread/process/outside of the program.

So one could think that volatile is necessary here. But (as Kayaman pointed out correctly): it is all about the Java memory model. And the fact that synchronized not only prevents race conditions (as only one thread can update data at any point in time) but also establishes a happens before relation.

Therefore: the above code doesn't have race conditions; and it also makes sure that each thread sees the "correct" value of isReady.

Of course, if you ever happen to manipulate isReady out of a synchronized block, then all bets are off.

score 0 · Answer 2 · edited Jul 10 '17 at 09:15

0

No, as you are only interacting with it in synchronized blocks, which by definition run only on one thread.

edited Jul 10 '17 at 09:15

shmosel

49,289
6
73
138

answered Jul 10 '17 at 09:07

Jan Hohenheim

3,552
2
17
42

This answer doesn't address the real issue, which is visibility. – Kayaman Jul 10 '17 at 09:10

score 0 · Answer 3 · answered Jul 10 '17 at 09:21

If the code that you exported is the only code accessing isReady, it is correct.

Instead if this is the only part of code writing isReady, you need to define isReady as volatile to read it correctly from another part of code.

In any case if the relevant synchronized code is the code that you posted, is incorrect defining the method topologyChanged as synchronized because internally you are using the monitor topologyLock. It is enough.

Boolean in Multithreaded environment

3 Answers3