java synchronized keyword needed on primitive getter / setter method?

Question

I read some java code, and found these functions:

synchronized void setConnected(boolean connected){
   this.connected = connected;
}

synchronized boolean isConnected(){
   return connected;
}

I wonder if synchronization makes any sense here, or just author didn't understand the need for synchronized keyword?

I'd suppose that synchronized is useless here. Or am I mistaken?

Celebration

The synchronized modifier only makes sense if more than one thread is going to attempt to access the connected variable - at least that's my understanding. – Nematode

Please refer #11460043. It answers your question and places it nicely in a larger context. – Belfry

read these Q + A #11460043 – Illiterate

synchronized method makes sense here (Of course when this code is executed in multi-threaded environment). It will allow the connected variable to be accessed in a thread safe way. Why do you think it's useless? – Katzman

@StevenWolfe, this is not correct, things are not that simple (or, in other words, are way more complicated...). Without synchronized, it is possible that reader threads doesn't see the modification made by even one single writer thread. – Abran

@Bruno Reis, I'm not so sure about that otherwise I'd be forever adding synchronized to all my getter and setter methods - or I'd be then forced to make all my member variables volatile. If multiple threads aren't being utilized then the synchronized modifier is unnecessary. – Nematode

@StevenWolfe, true, synchronized is useless if you have only one thread. I assume we are discussing multithreaded applications. However, it is true that this is unclear in the question. – Abran

Please refer an explanation here [1]- [1]: https://mcmap.net/q/268975/-should-getters-and-setters-be-synchronized – Aqaba

Answer 1

The keyword synchronized is one way of ensuring thread-safety. Beware: there's (way) more to thread-safety than deadlocks, or missing updates because of two threads incrementing an int without synchronization.

Consider the following class:

class Connection {
  private boolean connected; 
  synchronized void setConnected(boolean connected){
    this.connected = connected;
  }
  synchronized boolean isConnected(){
    return connected;
  }
}

If multiple threads share an instance of Connection and one thread calls setConnected(true), without synchronized it is possible that other threads keep seeing isConnected() == false. The synchronized keyword guarantees that all threads sees the current value of the field.

In more technical terms, the synchronized keyword ensures a memory barrier (hint: google that).

In more details: every write made before releasing a monitor (ie, before leaving a synchronized block) is guaranteed to be seen by every read made after acquiring the same monitor (ie, after entering a block synchronizing on the same object). In Java, there's something called happens-before (hint: google that), which is not as trivial as "I wrote the code in this order, so things get executed in this order". Using synchronized is a way to establish a happens-before relationship and guarantee that threads see the memory as you would expect them to see.

Another way to achieve the same guarantees, in this case, would be to eliminate the synchronized keyword and mark the field volatile. The guarantees provided by volatile are as follows: all writes made by a thread before a volatile write are guaranteed to be visible to a thread after a subsequent volatile read of the same field.

As a final note, in this particular case it might be better to use a volatile field instead of synchronized accessors, because the two approaches provide the same guarantees and the volatile-field approach allows simultaneous accesses to the field from different threads (which might improve performance if the synchronized version has too much contention).

Abran

However, some consider a slightly better practice to sync on a final field, as suggested by Mr. Skeet at this thread: #6911307 – Ozone

My question is maybe stupid, but your boolean should not be volatile ? – Speculative

@pith: no, since all the accesses to it (ie, all reads and writes) are done inside synchronized methods. – Abran

Answer 2

Synchronization is needed here to prevent memory consistency errors, see http://docs.oracle.com/javase/tutorial/essential/concurrency/memconsist.html. Though in this concrete case volatile would be much more efficient solution

private volatile boolean connected;

void setConnected(boolean connected){
   this.connected = connected;
}

boolean isConnected(){
   return connected;
}

Basset

Doesn't answer the question – Infanticide

Answer 3

The author has probably designed the code with a multi-threaded approach in mind. This means that the methods are synchronized and more than one thread will not be able to access the synchronized code at the same time on the same object instance.

Prebend