Should getters and setters be synchronized?

private double value;

public synchronized void setValue(double value) {
    this.value = value;
}
public double getValue() {
    return this.value;
}

In the above example is there any point in making the getter synchronized?

I think its best to cite Java Concurrency in Practice here:

>It is a common mistake to assume that synchronization needs to be used only when writing to shared variables; this is simply not true.

> For each mutable state variable that may be accessed by more than one > thread, all accesses to that variable must be performed with the same > lock held. In this case, we say that the variable is guarded by that > lock.

> In the absence of synchronization, the compiler, processor, and runtime can do some downright weird things to the order in which operations appear to execute. Attempts to reason about the order in which memory actions "must" happen in insufflciently synchronized multithreaded programs will almost certainly be incorrect.

Normally, you don't have to be so careful with primitives, so if this would be an int or a boolean it might be that:

> When a thread reads a variable without synchronization, it may see a > stale value, but at least it sees a value that was actually placed > there by some thread rather than some random value.

This, however, is not true for 64-bit operations, for instance on long or double if they are not declared volatile:

> The Java Memory Model requires fetch and > store operations to be atomic, but for nonvolatile long and double > variables, the JVM is permitted to treat a 64-bit read or write as two > separate 32-bit operations. If the reads and writes occur in different > threads, it is therefore possible to read a nonvolatile long and get > back the high 32 bits of one value and the low 32 bits of another.

> Thus, even if you don't care about stale values, it is not safe to use > shared mutable long and double variables in multithreaded programs > unless they are declared volatile or guarded by a lock.

Let me show you by example what is a legal way for a JIT to compile your code. You write:

while (myBean.getValue() > 1.0) {
  // perform some action
  Thread.sleep(1);
}

JIT compiles:

if (myBean.getValue() > 1.0) 
  while (true) {
    // perform some action
    Thread.sleep(1);
  }

In just slightly different scenarios even the Java compiler could prouduce similar bytecode (it would only have to eliminate the possibility of dynamic dispatch to a different getValue). This is a textbook example of hoisting.

Why is this legal? The compiler has the right to assume that the result of myBean.getValue() can never change while executing above code. Without synchronized it is allowed to ignore any actions by other threads.

The reason here is to guard against any other thread updating the value when a thread is reading and thus avoid performing any action on stale value.

Here get method will acquire intrinsic lock on "this" and thus any other thread which might attempt to set/update using setter method will have to wait to acquire lock on "this" to enter the setter method which is already acquired by thread performing get.

This is why its recommended to follow the practice of using same lock when performing any operation on a mutable state.

Making the field volatile will work here as there are no compound statements.

---

It is important to note that synchronized methods use intrinsic lock which is "this". So get and set both being synchronized means any thread entering the method will have to acquire lock on this.

---

When performing non atomic 64 bit operations special consideration should be taken. Excerpts from Java Concurrency In Practice could be of help here to understand the situation -

"The Java Memory Model requires fetch and store operations to be atomic, but for non-volatile long and double variables, the JVM is permitted to treat a 64 bit read or write as two separate 32 bit operations. If the reads and writes occur in different threads, it is therefore possible to read a non-volatile long and get back the high 32 bits of one value and the low 32 bits of another. Thus, even if you don't care about stale values, it is not safe to use shared mutable long and double variables in multi-threaded programs unless they are declared volatile or guarded by a lock."

Maybe for someone this code looks awful, but it works very well.

  private Double value;
  public  void setValue(Double value){
    updateValue(value, true);
  }
  public Double getValue(){
      return updateValue(value, false);
  }
  private double updateValue(Double value,boolean set){
    synchronized(MyClass.class){
      if(set)
        this.value = value;
      return value;
    }
  }