Why is synchronizing a method not advisable?

user_13822387 0
Viewed 82

Consider the below code snippet:

public class DbSingleton {

    private static volatile DbSingleton instance = null;

    private DbSingleton() {
        if (instance != null) {
            throw new RuntimeException("Use getInstance()");
        }
    }

    public static DbSingleton getInstance() {
        if (instance == null) {
            synchronized (DbSingleton.class) {
                if (instance == null) {
                    instance = new DbSingleton();
                }
            }
        }
        return instance;
    }
}

Why is the above preferred to declaring getInstance() as synchronized? What does it mean when a method is declared as synchronized? Will it synchronize the entire class and slow down the performance?

synchronized multithreading java
3 Answers

If you declare getInstance as synchronized, you will pay for the synchronization penalty every time you call getInstance even after it is initialized. With this implementation, getInstance will never go into a synchronized block after its first call because the instance is initialized already.

user_119239990

Acquiring a lock is expensive. In the given code, a lock is only acquired if instance == null.

Consider the following

  1. Thread A calls getInstance()
  2. Thread B calls getInstance()
  3. Thread A acquires the lock
  4. Thread B waits on the lock
  5. Thread A checks that instance == null initializes it, and releases the lock
  6. Thread B acquires the lock
  7. Thread B checks that instance != null and returns instance.

This scenario will happen with either a synchronized method or double checked locking. However, after the instance has been instantiated, consider the difference.

(This is the same as public static synchronized DbSingleton getInstance())

public static DbSingleton getInstance() {
    synchronized (DbSingleton.class) {
        if (instance == null) {
            instance = new DbSingleton();
        }
        return instance;
    }
}

With this method, after instance has been initialized..

  1. Thread A calls getInstance, acquires the lock, returns instance, and releases the lock
  2. Thread B calls getInstance, acquires the lock, returns instance, and releases the lock
  3. etc.

However, acquiring the lock is redundant in this case, which is the purpose of this code:

public static DbSingleton getInstance() {
    if (instance == null) {
        synchronized (DbSingleton.class) {
            if (instance == null) {
                instance = new DbSingleton();
            }
        }
    }
    return instance;
}

If instance does not equal null, a lock is never acquired and the flow looks like

  1. Thread A calls getInstance and returns instance (no lock)
  2. Thread B calls getInstance and returns instance (no lock)
user_53781870

Double-checked locking is evil and unnecessary, as is the Singleton antipattern. Also, initializing the static variable to null is a bad idea. Just initialize the variable as a final variable, or use an enum.

private static final DbSingleton instance = new DbSingleton();
…
public static DbSingleton getInstance() {
    return instance;
}

The instance is already lazily initialized when the class is, so any additional effort to accomplish that is stupid.

user_9050330
Related