Simple Singleton instantiation problem

Question

I must be doing something very silly, but I'm getting a ExceptionInInitializerError when I try to instantiate an object in my Singleton:

class MySingleton {

  private static MySingleton instance = null;
  private OtherObject obj;

  // Do I instantiate obj here???
  private MySingleton() {
    //obj = new OtherObject();
  }

  // Or here?
  {
    //obj = new OtherObject();
  }

  // Or where? ...

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

}

Should I make the other object in the constructor, or is that always supposed to be empty for singletons? I get the exception in both the constructor and the initializer block...

Here's the main():

public static void main(String[] args) {
    try {
        MySingleton singleton = MySingleton.getInstance();
    } catch (Error e) {
        System.err.println("Error!");
    }
}

Thanks!

Answer 1

you should instantiate OtherObj in your constructor. What is the exact code that is giving the error?

edit -- The following worked for me

class MySingleton {

  private static MySingleton instance = null;
  private Integer obj;


  private MySingleton() {
    obj = new Integer(2);
  }


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

}

and then I just call getInstance from a main loop. You might want to look at

http://en.wikipedia.org/wiki/Singleton_pattern#Java

Answer 2

class MySingleton {

  private static MySingleton instance = null;
  private OtherObject obj;

  private MySingleton() {
      obj = new OtherObject();
  }

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

}

Answer 3

Static initialization sections are for initializing members of the class marked static. Since OtherObject obj isn't static it should not be initialized there. The correct place is to initialize it in the constructor.

If you are getting an ExceptionInInitializerError when you have obj = new OtherObject() in the constructor, the problem may be with another class (OtherObject perhaps?) that is incorrectly initializing static members.

Answer 4

You can avoid some of your confusion by eliminating the lazy initialization risk (which you're currently paying for in the exception). Since you're really just returning your static instance, why not just create that static instance at run time (i.e., don't wait for null):

class MySingleton {

  private static MySingleton instance = new MySingleton();

  // You could do this here or in the constructor
  // private OtherObject obj = new OtherObject();

  /** Use this if you want to do it in the constructor instead. */
  private OtherObject obj;

  private MySingleton() {
      obj = new OtherObject();
  }

  /** Now you can just return your static reference */
  public static MySingleton getInstance() {
      return instance;
  }

}

If you notice, now everything is deterministic and straightforward. Your MySingleton.instance is populated at runtime and is accessed via the static method MySingleton.getInstance().

I realize that this doesn't match the exact pattern of the original GOF design patterns book but you'll notice that the usage of the class is effectively the same.

EDIT: following up on some of the thread safety points raised in other answers and comments, I'm going to try to illustrate how the original proposed solution in the question and in the GOF book is non-thread safe. For a better reference, see Java Concurrency in Practice which I own and have on my ACM / Safari online bookshelf. It is frankly a better source than my sketched example but, hey, we can but strive....

So, imagine we have two threads named Thread1 and Thread2 and, coincidentally, each hits the MySingleton.getInstance() method at the same moment. This is entirely possible in a modern multicore hyperthreaded system. Now, even though these two threads happened to hit the entry point of this method at the same time, there's no assurance of which is going to hit any particular statement. So, you could see something like this:

// Note that instance == null as we've never called the lazy getInstance() before.

Thread1: if (instance == null)
Thread2: if (instance == null) // both tests pass - DANGER
Thread1:     instance = new MySingleton();
Thread2:     instance = new MySingleton(); // Note - you just called the constructor twice
Thread1: return instance; // Two singletons were actually created 
Thread2: return instance; // Any side-effects in the constructor were called twice

The problem illustrated in the if-null test is called a race condition. You can't know who's going to what statement when. As a result, it's like both threads are racing each other to a cliff.

If you look at the same sort of examination of my example with both threads still hitting getInstance() at the same moment, you see something like this:

Thread1: return instance;
Thread2: return instance;

Simplistic, yes, but that's my point. The object was constructed once, long ago, and the threads can count on its value being consistent. No race exists.

NOTE: you still have to worry about the contents of the constructor for OtherObject. The lesson there is: concurrency is hard. If you're making your constructor thread safe (which you should), make sure that the author of OtherObject is doing you the same courtesy.

Answer 5

Your Singleton class is not at fault here. I would guess a problem in the contructor of the OtherObject class.

A correct syntax would be:

class MySingleton {

  private static MySingleton instance = null;
  private OtherObject obj;

  private MySingleton() {
    obj = new OtherObject();
  }

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

}

Now, you just need to give us more information about the OtherObject :-)

Answer 6

The answer is simple, don't use Singletons. While the GoF book has some good ideas, the singleton pattern is not one of them.

As others have said upthread, the proper thread safe use of singletons is tricky, see

IBM article on thread safe singletons

In reality, all a Singleton implements is a global object. This is bad, it pollutes the namespace. It makes proper unit testing (Junit, etc) much harder if not impossible. You don't need it, ever. A simple static factory class is cleaner code, avoids the global pollution and is less code.