Is necessary synchronize thread safe singleton's methods

Question

Let say that I have a singleton:

public class MySinglton {

    private static volatile MySinglton s;
    private int x; 

    public static MySinglton getInstance() {
        if (s != null) return s;
        synchronized (MySinglton.class) {
            if (s == null) {
                s = new MySinglton();
            }
        }
        return s;
    }

    public void setX(int x){
        this.x = x;
    }

} 

Ok, method getInstance is thread safe. My question is. Is it necessary to modify method setX, or it is thread safe because getInsatnce method is thread safe. If it is not what is better.

    synchronized public void setX(int x){
        this.x = x;
    }

    public void setX(int x){
        synchronized (this) {
            this.x = x;
        }
    }

or finally

    ReadWriteLock readWriteLock = new ReentrantReadWriteLock();

    public void setX(int x){
        readWriteLock.readLock().lock();
        this.x = x;
        readWriteLock.readLock().unlock();
    }

Answer 1

Just because getInstance() is thread safe, that does not mean any other methods of the class are thread safe. Multiple threads can still perform operations on the singleton object simultaneously.

You should synchronize a private final class member object inside the setX function. Do not synchronize this and do not synchronize the entire method. Synchronizing this synchronizes the object and if other code also synchronized the object returned by getInstance(), you could have yourself a deadlock. Putting the synchronized keyword before methods means that only one synchronized method of the synchronized methods in your class can be executed by a thread on an instance at any given time. It can also give the impression to clients/consumers of the class that the class is thread safe even though it may not be.

A good approach to writing a singleton is to use an enum as that guarantees there will only ever be one instance of it. However, the member functions of the enum will still need to be synchronized if you want it to be thread safe. See page 311 of Effective Java for an example of this: http://uet.vnu.edu.vn/~chauttm/e-books/java/Effective.Java.2nd.Edition.May.2008.3000th.Release.pdf

Answer 2

No setX is not thread safe as there might be multiple threads having reference of singleton's instance and they may try to execute setX api on the same instance.

You will need to make setX threadsafe. Both your method implementation with synchronized keyword would work.

I would not use read lock of readWriteLock as am writing a value to it. Also what if say something happens in between you acquire lock and unlock? You would ever unlock and hence lead to deadlock. So always use lock and unlock in try/catch/finally block where you unlock in finally block.

Answer 3

Having a singleton doesn't prevent multiple threads from calling setX() at the same time. So you definitely need synchronized here.

And it seems awkward to fetch a READ lock before WRITING. Point is: readers (that invoke a missing method "getX()") need a readlock; when writing, you want a WRITE lock!

To be precise: you need a ReadWrite lock if the property that is updated isn't "atomic" AND your program "knows" different "reader" and "writer" roles.

If there is just a "setX()" method (and no readers around, then "synchronized" should do; in that case you don't need a RW lock).

Answer 4

setX is called on an instance of s. So there is only one instance of s at any given time in the JVM (bcos it's a singleton). If two threads simulataeneously call setX, they could both ovewrite the same x or step on each other.

• For eg. Without Synchronization, if a thread A & thread B update x at the exact same point in time, other threads accessing these values may see different values for x.

setX is not implicitly threadsafe.

This is a good to have

public void setX(int x){
    synchronized (this) {
        this.x = x;
    }
}

Here is a similar post Java fine-grained synchronization in getter/setter methods and singleton pattern of a class

Answer 5

public final class MySinglton 
{ 
   private final static MySinglton s; 
   private volatile int x; 

   static {
       s = new MySinglton();
   }

   private MySinglton() {}

   public static MySinglton getInstance() 
   {
       return s;
   }

   public void setX(int x)
   {
      this.x = x; 
   }

}

If you do not have any other requirements for you Singleton you can go with this. Provide a default constructor to avoid other instances and mark the class as final so noone can extend it. Placing the initialization in a static block will work fine for most programs. If you encounter problems just wrap it in a static inner class.

Answer 6

All you have shown us is a singleton object with a method setX(int x). But, the setX(x) method doesn't do anything except set a private variable that no other class can see.

You haven't shown us any code that uses x. Until you show us how x is used, "thread safe" means nothing.

"Thread safety" is all about making sure that your program will do what you want it to do no matter how the executions of its threads are interleaved. What do you want your program to do?

Sometimes we talk about a "thread safe" class. People say, for example, that java.util.concurrent.ArrayBlockingQueue is "thread-safe". But that only means that the threads in your program can not put an ArrayBlockingQueue into a bad internal state. It does not mean that the queue will always contain the objects that your program wants it to contain or, in the order that your program wants. Building a program out of "thread safe" objects does not make the program thread safe.

What other methods are in your MySingleton class? How is it supposed to be used?