Java synchronize in singleton pattern

Question

Does the synchronize keyword need to be applied to each method of a class that implements the singleton pattern like this?

public class Singleton {

  private Singleton(){}

  public synchronized static Singleton getInstance()
    {   
        if(instance == null)
            instance = new Singleton ();

        return instance;
    }

  public void DoA(){
  }
}

Since Singletons don't expose a public constructor and the getInstance() method is synchronized, one does not need to synchronize method DoA and any other public methods exposed by the Singleton class.

Is this reasoning correct?

Cleanlimbed

En general sychronization depends if you access shared data in the method. – Bryophyte

This should answer your question – Consequently

I don't think so, If getInstance is sync. that doesn't mean the DoA is sync. too. – Mathias

If you mutate shareable state on the DoA method, then yes it may still need synchronization. What does the method do ? – Countercharge

this is nothing to do with being a singleton – Bread

Answer 1

It's just like any other class. It may or may not need further synchronization.

Consider the following example:

public class Singleton {

  private Singleton() {}

  public synchronized static Singleton getInstance() { ... }

  private int counter = 0;

  public void addToCounter(int val) {
    counter += val;
  }
}

If the class is to be used from multiple threads, addToCounter() has a race condition. One way to fix that is by making addToCounter() synchronized:

  public synchronized void addToCounter(int val) {
    count += val;
  }

There are other ways to fix the race condition, for example by using AtomicInteger:

  private final AtomicInteger counter = new AtomicInteger(0);

  public void addToCounter(int val) {
    counter.addAndGet(val);
  }

Here, we've fixed the race condition without using synchronized.

Fye

How can I solve PMD error "Use block level rather than method level synchronization" on getInstance? – Yippie

Answer 2

Well, the purpose of the Singleton class is that there is at most one instance of it and that all Threads can access that same object.

If you would not synchronize the getInstance method the following could happen

Thread1 enters getInstance()

Thread2 enters getInstance()

Thread1 evaluates instance == null to true

Thread2 evaluates instance == null to true

Thread1 assigns instance and returns

Thread2 reassigns instance = new Singleton() and returns.

Now the threads both have a difference instance of the Singleton class which is what should have been prevented by this pattern.

Synchronizing prevents that both Threads can access the same block of code at the same time. So synchronization is needed in a multithreaded environment when you instantiate singleton classes.

Now assuming that multiple threads will attempt to access the Singletons methods at the same time synchronization might be necessary on those methods as well. Especially if they change data instead of only reading it this is true.

Ollayos

I think the question refers to the public methods of the Singleton instance, and not to the static getInstance. – Cristen

Yeah, I should be more patient and read the question before answering. I'll expand the answer... – Ollayos

Answer 3

The correct(Best actually) way to use Singleton

private static singleton getInstance() {
    if (minstance == null) {
        synchronized (singleton.class) {
            if (minstance == null) {
                minstance = new singleton();
            }
        }
    }
    return minstance;
}

Natiha

if minstance happens to be null, then sychronizing the block with that instance will result in NullPointerException. – Tice

Yeah. Actually I wrote the previous code in a hurry. ;) – Natiha

it's a bad pratice - it's double checked locking pattern - and will not work after all - explaination here cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html – Painkiller

@PawełWoźniak: Quit interesting share you have there. But as of java, I think this is the right thing to do! – Natiha

Answer 4

lazy initialization and thread safe solution:

public class Singleton {

    public static class SingletonHolder {
        public static final Singleton HOLDER_INSTANCE = new Singleton();
    }

    public static Singleton getInstance() {
        return SingletonHolder.HOLDER_INSTANCE;
    }
}

Reverend