13

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?

Pointer Null
  • 39,597
  • 13
  • 90
  • 111
  • 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. – Steven Apr 05 '13 at 07:23
  • Please refer http://stackoverflow.com/questions/11459543/java-synchronized-getters-and-setters. It answers your question and places it nicely in a larger context. – prashant Apr 05 '13 at 07:24
  • 2
    read these Q + A http://stackoverflow.com/questions/11459543/java-synchronized-getters-and-setters – Zelldon Apr 05 '13 at 07:24
  • `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? – Rohit Jain Apr 05 '13 at 07:24
  • @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. – Bruno Reis Apr 05 '13 at 07:25
  • @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. – Steven Apr 05 '13 at 07:31
  • @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. – Bruno Reis Apr 05 '13 at 07:32
  • Please refer an explanation here [1]- [1]: http://stackoverflow.com/a/23213409/3559363 – Upendra Upadhyay Apr 22 '14 at 07:54

3 Answers3

19

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).

Bruno Reis
  • 37,201
  • 11
  • 119
  • 156
  • However, some consider a slightly better practice to sync on a final field, as suggested by Mr. Skeet at this thread: http://stackoverflow.com/questions/6910807/synchronization-of-non-final-field – Nikola Yovchev Apr 05 '13 at 07:26
  • My question is maybe stupid, but your boolean should not be `volatile` ? – Pith Apr 05 '13 at 10:24
  • @pith: no, since all the accesses to it (ie, all reads and writes) are done inside synchronized methods. – Bruno Reis Apr 05 '13 at 20:24
8

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;
}
Evgeniy Dorofeev
  • 133,369
  • 30
  • 199
  • 275
2

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.

Dan D.
  • 32,246
  • 5
  • 63
  • 79