views:

224

answers:

8

The default way to implement singleton patter is:

class MyClass {
  private static MyClass instance;
  public static MyClass getInstance() {
    if (instance == null) {
      instance = new MyClass();
    }
    return instance;
  }
}

In an old project, I've tried to simplify the things writing:

class MyClass {
  private static final MyClass instance = new MyClass();
  public static MyClass getInstance() {
    return instance;
  }
}

But it sometimes fail. I just never knew why, and I did the default way. Making a SSCCE to post here today, I've realized the code works.

So, I would like to know opinions.. Is this a aleatory fail code? Is there any chance of the second approach return null? Am I going crazy?

-- Although I don't know if is the right answer for every case, it's a really interesting answer by @Alfred: I also would like to point out that singletons are testing nightmare and that according to the big guys you should use google's dependency injection framework.

A: 

I don't know the answers to your questions, but here is how I might structure the same thing.

class MyClass {
  private static MyClass instance;

  static {
    instance = new MyClass();
  }

  private MyClass() { }

  public static MyClass getInstance() {
    return instance;
  }
}
Jonathon
what is the reason not to do `private static final MyClass instance = new MyClass();` ?
Carlos Heuberger
@Carlos No reason that I know other than stylistically. Static sections are very obvious in their purpose when you're reading code. The final is always great to have, but sometimes I don't like throwing initializations together with field declarations.
Jonathon
@Jonathan: I think that my line is a bit more obvious than having it spread over three lines, at least for such simple code (matter of taste?!)... and using `final` helps in that *direction* too; you still can use the `final` in your code, the compiler should be smart enough to allow it (I suspect it will produce the same bytecode).
Carlos Heuberger
+3  A: 

The second example is preferable, since the first isn't thread safe (as pointed out in the comments). The first example uses a technique called lazy instantiation (or lazy initialization), which ensures that the Singleton instance isn't created unless it's actually needed. This isn't really necessary in Java because of the way Java handles class loading and static instance variable initialization.

Bill the Lizard
the first version is not thread safe.
tony
@tony: You can throw an `AtomicReference` at it and make it thread-safe. :-P (Or, if it's absolutely imperative that the class not be constructed more than once, then synchronise the `getInstance` method.)
Chris Jester-Young
Would +1 this, except for the claim that the non-threadsafe version is "OK".
Software Monkey
@Chris: Or... just synchronize the getInstance() method.
Software Monkey
Oh right, that isn't safe, is it? I'll edit.
Bill the Lizard
+8  A: 

The recommended (by Effective Java 2nd ed) way is to do the "enum singleton pattern":

enum MyClass {
    INSTANCE;

    // rest of singleton goes here
}

The key insight here is that enum values are single-instance, just like singleton. So, by making a one-value enum, you have just made yourself a singleton. The beauty of this approach is that it's completely thread-safe, and it's also safe against any kinds of loopholes that would allow people to create other instances.

Chris Jester-Young
Horrible. Just because all enums are singletons does not mean every singleton must be an enum. Creating a singleton which was not an enum as an enum is horribly misleading.
Software Monkey
@Software Monkey: What makes a singleton "not an enum"? Where do you draw the line? An enum of 1 value (or even an enum of no values, which is legal BTW) is as good as any other enum, just like a tuple/sequence/list/vector of 1 or 0 values is as good as any other.
Chris Jester-Young
@Chris: It's a conceptual thing. An enumeration is a series of flag-like identifiers, not a general purpose object.
Software Monkey
@Software Monkey - take it up with the Java language designers for the name "enum" or Joshua Bloch for the pattern. That's not a good reason not to use the best version of the Singleton pattern in Java (post 1.5) (and not that there's usually a good reason to use a Singleton pattern anyway...)
Nate
+4  A: 

The first solution is (I believe) not thread-safe.

The second solution is (I believe) thread-safe, but might not work if you have complicated initialization dependencies in which MyClass.getInstance() is called before the MyClass static initializations are completed. That's probably the problem you were seeing.

Both solutions allow someone to create another instance of your (notionally) singleton class.

A more robust solution is:

class MyClass {

  private static MyClass instance;

  private MyClass() { }

  public synchronized static MyClass getInstance() {
    if (instance == null) {
      instance = new MyClass();
    }
    return instance;
  }
}

In a modern JVM, the cost of acquiring a lock is miniscule, provided that there is no contention over the lock.

EDIT @Nate questions my statement about static initialization order possibly causing problems. Consider the following (pathological) example:

public ClassA {
    public static ClassB myB = ClassB.getInstance();
    public static ClassA me = new ClassA();
    public static ClassA getInstance() {
        return me;
    }
}

public ClassB {
    public static ClassA myA = ClassA.getInstance();
    public static ClassB me = new ClassB();
    public static ClassB getInstance() {
        return me;
    }
}

There are two possible initialization orders for these two classes. Both result in a static method being called before the method's classes static initialization has been performed. This will result in either ClassA.myB or ClassB.myA being initialized to null.

In practice, cyclic dependencies between statics are less obvious than this. But the fact remains that if there is a cyclic dependency: 1) the Java compiler won't be able to tell you about it, 2) the JVM will not tell you about it. Rather, the JVM will silently pick an initialization order without "understanding" the semantics of what you are trying to do ... possibly resulting in something unexpected / wrong.

EDIT 2 - This is described in the JLS 12.4.1 as follows:

As shown in an example in §8.3.2.3, the fact that initialization code is unrestricted allows examples to be constructed where the value of a class variable can be observed when it still has its initial default value, before its initializing expression is evaluated, but such examples are rare in practice. (Such examples can be also constructed for instance variable initialization; see the example at the end of §12.5). The full power of the language is available in these initializers; programmers must exercise some care. ...

Stephen C
"in which MyClass.getInstance() is called before the MyClass static initializations are completed. That's probably the problem you were seeing." - not possible. Static initializers run when the class is first referenced. The class is referenced when a static method is called. Static initialization will complete before the static method is invoked.
Nate
Your example runs fine - I've added output from each class's static initializer, getInstance(), and constructor. I added a class with a main() method that calls A.getInstance() then B.getInstance(). I've never gotten a null reference Every time it executes in the same order... `A static init``B static init``A getInstance``B constructor``B getInstance()``A constructor``A getInstance``B getInstance()`Which means this is deterministic, and you're wrong.
Nate
@Nate - have you examined the values of `myA` and `myB` ????? That is where the problem will manifest itself.
Stephen C
Ah - OK, yes, I see what you mean. Sorry for doubting it before, but thanks for describing it more so I could understand, and thanks for enlightening me on this weird 'gotcha'. It also looks like you can cause this by creating a static block at the top of your class and calling getInstance() there too.
Nate
A: 

Remember, you need to declare a private constructor to ensure the singleton property.

The second case could be even simpler with just

class MyClass {
  public static final MyClass instance = new MyClass();

  private MyClass() {
      super()
  }
}

`

Tarski
+1  A: 

As others have noted, the first is not thread-safe. Don't bother with it as the second is perfectly fine, and will instantiate the object only when MyClass is referenced. Further it makes the reference final which expresses the intent better.

Just make sure that the declaration

private static final MyClass INSTANCE = new MyClass();

is the first static declaration in your class to avoid risk of calls to getInstance() or INSTANCE before it is initialized.

Software Monkey
+1  A: 

I also would like to point out that singletons are testing nightmare and that according to the big guys you should use google's dependency injection framework.

Alfred
Interesting, I'll read more about.
Tom Brito
And why **only** Google's DI framework?
Pascal Thivent
+1  A: 

Don't forget the SingletonHolder pattern. See this SO question.

Trevor Harrison