1

I am getting confused which one should I use to make a synchronized singleton class object. I know below 2 ways of achieving it.

static volatile PaymentSettings paymentSettings = null;

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

And

 private static class PaymentSettingsInstanceHolder {
    private static PaymentSettings instance = new PaymentSettings();
}

public static PaymentSettings getInstance() {
    return PaymentSettingsInstanceHolder.instance;
}

Please suggest which approach should I use and why?

aman.nepid
  • 2,864
  • 8
  • 40
  • 48
  • 2
    This isn't making an object synchronised. I'm not sure I even know what that means. This creates a lazily initialised singleton! And the second method is far better. – Boris the Spider Oct 12 '15 at 07:44
  • 1
    Syncronization via `synchronized (PaymentSettings.class)` may lead to deadlocks (rarely, but possible) since the lock is available from the outside (that is, if a caller is syncronizing on that lock too). If you like to do synchrinized lazy init, you may just use LazyInitializer from Apache commons. https://commons.apache.org/proper/commons-lang/javadocs/api-3.1/org/apache/commons/lang3/concurrent/LazyInitializer.html - Apart from that: The two approach differ in the time of construction (lazy vs. non-lazy) and not how they do synchronization - which one is better depends on may other aspects. – Christian Fries Oct 12 '15 at 07:45
  • Better always depends on the goals you want to achieve. You don't state those in your question – Jens Schauder Oct 12 '15 at 07:45
  • Possible duplicate of [What is an efficient way to implement a singleton pattern in Java?](http://stackoverflow.com/questions/70689/what-is-an-efficient-way-to-implement-a-singleton-pattern-in-java) – weston Oct 12 '15 at 07:47
  • The difference is the initialization moment. On the first call to `getInstance()` in the first version and the class loading in the second version, which is less predictable. – lexicore Oct 12 '15 at 07:51
  • @ChristianFries this is **a lie**. Both approaches **are lazy**. – Boris the Spider Oct 12 '15 at 08:28
  • @ChristianFries You can't get a deadlock unless there are two or more locks acquired in different orders. What you are describing is merely a *block.* – user207421 Oct 12 '15 at 08:37
  • @BoristheSpider bit harsh, being incorrect != being a liar – weston Oct 12 '15 at 10:59

1 Answers1

1

These two mechanisms are common attempts at achieving a Singleton.

The first is called Double-checked locking and is generally considered broken, although your technique of locking against the class may be an acceptable work-around.

Your second is a tidy solution but may create the object when not needed.

The nowadays considered best solution is using an enum. This ensure that the object is only created when needed and not before. See here for an excellent discourse on why this is good.

final class Singleton {

    private Singleton() {
        // Make sure only I can create one.
    }

    private enum Single {

        INSTANCE;
        // The only instance - ever.
        final Singleton s = new Singleton();

    }

    public static Singleton getInstance() {
        // Will force the construction here only.
        return Single.INSTANCE.s;
    }
}
Community
  • 1
  • 1
OldCurmudgeon
  • 64,482
  • 16
  • 119
  • 213
  • 1
    I guess that most of this is so it matches OP's original signature as `public enum PaymentSettings { INSTANCE; }` is all they really need. Or are they other reasons for this way? – weston Oct 12 '15 at 07:52
  • @weston - Yes there's more. The `enum` ensures construction at the last minute and only once. It also provides serialization out of the box. Also the presence of `private Singleton ...` guarantees the contract of **there can be only one**. Also, you should not expose the `enum` itself, you should let the `enum` instanciate the singleton. – OldCurmudgeon Oct 12 '15 at 07:56
  • I know why enum, I was asking what has your longer way, got over the shorter way I posted (and that you linked to). It hides the enum, but why is that better? Also you should have the class `final` if you do do it your way. – weston Oct 12 '15 at 08:07
  • 1
    Both the solutions proposed by the OP are **lazy**. The DCL is fine because of `volatile`. Your proposed solution is **eager**. Which is more appropriate solely depends on use case. – Boris the Spider Oct 12 '15 at 08:30
  • 1
    It hasn't been 'considered broken' since they tightened the memory model in Java 1.5, quite a few years ago now. – user207421 Oct 12 '15 at 08:38
  • @weston - Your method 2 will create the singleton when *any* access to your `PaymentSettingsInstanceHolder` - e.g. a static method etc. Mine would **only** create the instance when `getInstance` is called and is therefore as late as possible. – OldCurmudgeon Oct 12 '15 at 08:55
  • Fair enough, but easier to keep the static methods elsewhere I would think. – weston Oct 12 '15 at 10:56