Wednesday, August 16, 2006

Java Singleton Revised

While working on a Java project and making a search related to this project, I stumbled upon an interesting article by Peter Haggar called "Double-checked locking and the Singleton pattern".

In this article the author describes problems with different Java implementations of singleton pattern that are most of the time visible only in multi-threaded environments.

To make long story short his conclusion is that it is not good practice writing a singleton class as:

class Singleton {
private static Singleton instance;

private Singleton() {
//initialize instance
}

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

return instance;
}
}

This is commonly used but as described in the article, not a thread safe implementation. Developers should neither use a double-checked locking implementation (described in the article), but rather use one of the two options:

  1. make getInstance() method synchronized - which will be thread safe, but will have impact on performance
  2. initialize static class variable instance at declaration time like this:
class Singleton {
private static Singleton instance = new Singleton();

private Singleton() {
//initialize instance
}

public Singleton getInstance() {
return instance;
}
}

I like simple solutions to problems that are not always simple and so I like this solution. However IMHO, this implementation is not suitable for all cases where you might want to use singleton pattern.

Let's take a singleton that executes operations on a file as an example. At some point in your program you need to close the RandomAccessFile, FileOutputStream or FileWriter that you are using (you always close files and db connections right? :) ), so I would expect that you would add another static method to the Singleton class that would close all necessary resources that you opened in the private constructor.

Let's call this method closeInstance() and implement it like this:

public static synchronized void closeInstance() {
//close all necessary resources
instance = null;
}

We closed all resources and since these resources (required for this class to operate) are closed, we can also set the instance to null to prevent someone using Singleton instance while it is an in unusable state. This method is synchronized to prevent some problems with concurrent access.

What will happen if you try to call getInstance() now? A null will be returned. That is not really what you would expect, is it? What would you do if for whatever reason you closed the Singleton instance and later on realized that you need to open it again. You don't have access to constructor and no other way how to initialize the singleton again.

My solution is based on the solution of Peter Haggar, but enhances it to prevent the issues I mentioned above.

public class Singleton {
private static Singleton instance = new Singleton();

private Singleton() {
//initialize instance
//opens all necessary resources
}

/**
* Returns the singleton instance. If the singleton instance was
* not previously closed, the instance is initialized before first
* call of this method automatically.
*
* @throws  IllegalStateException   if instance had been closed by
*                                  closeInstance() method
*/
public static Singleton getInstance() {
if (instance == null)
  throw new IllegalStateException(
    "Singleton instance has been closed already");
return instance;
}

/**
* Cleanly closes singleton resources and removes the singleton
* instance
*/
public static synchronized void closeInstance() {
//close all necessary resources
instance = null;
}

/**
* Creates and returns a reference to the new instance.
* If the instance already exists, then the reference to this
* instance is returned. This method is intended to be used only
* in cases when there is a chance that the instance was previously
* closed. In other cases use of getInstance() is encouraged because
* of performance benefits, since this method is synchronized
* as opposed to getInstance().
*/
public static synchronized Singleton createInstance() {
if (instance == null)
  instance = new Singleton();
return instance;
}

/**
* Checks the status of the singleton instance
*
* @return  true if singleton instance is initialized, if instance
*          was closed returns false.
*/
public static boolean instanceCreated() {
return instance != null;
}
}

This implementation of Singleton gives you all performance benefits of the first mentioned implementation while being thread safe and giving you all the flexibility of reinitializing the singleton instance once it was previously, intentionally destroyed.

2 comments:

Maurizio said...

I think the problem with DCL comes from checking the wrong variable. Instead of checking the static instance itself one should check a boolean, which is by nature an atomic operation. This would prevent the JVM memory model "problem". What do you think?


public class Singleton {

private static Singleton _instance = null;
private static final Object _synch = new Object();
private static boolean _initialized = false;

/** Creates a new instance of Singleton */
private Singleton() {
}

public static Singleton getInstance() {
if (!_initialized) {
synchronized (_synch) {
if (!_initialized) {
_instance = new Singleton();
_initialized = true;
}
}
}
return _instance;
}

}

Igor Minar said...

I believe that using primitives could help, but this solution is ugly and complicated (as the whole DCL idea) to say the least.

Static initialization is very simple, elegant and is guaranteed to work by JLS.

Other alternative is Initialization on demand holder idiom, which in most cases behaves the same as static initialization.