CODEWITHSUNDEEP
javamultithreadingsynchronization
Swaranga Sarma
Swaranga Sarma

Reputation: 13413

Double checked locking in modern JVMs

I have a class that might throw any run-time exceptions during initialization. I want the class to be a singleton since the cost of keeping several objects in memory is high. I am using that class in another class.

My use case is as follows:

So I check if my Controller instance is null when I try to do a "get" on the Controller, if yes, I try to instantiate it again.

Following is my code:

class Parent
{
    private static volatile Controller controller;
    private static final Object lock = new Object();

    static
    {
        try
        {
            controller = new Controller();
        }
        catch(Exception ex)
        {
            controller = null;
        }
    }

    private Controller getController() throws ControllerInstantiationException
    {
        if(controller == null)
        {
            synchronized(lock)
            {
                if(controller == null)
                {
                    try
                    {
                        controller = new Controller();
                    }
                    catch(Exception ex)
                    {
                        controller = null;
                        throw new ControllerInstatntationException(ex);
                    }
                }
            }
        }
        return controller;
    }

    //other methods that uses getController() 
}

My question is, is this code broken? I read somewhere that the above code would be a problem in JVM 1.4 or earlier. Can you provide references/solutions? Please note that I am asking this question because there is a lot of confusion regarding this topic in the internet.

Thanks.

Upvotes: 4

Views: 1216

Answers (5)

Kuldeep Jain
Kuldeep Jain

Reputation: 8598

It is not an answer to your question but this famous article on Double-Checked Locking is Broken explains well as to why it is broken for java 1.4 or earlier version.

Upvotes: 0

Avi
Avi

Reputation: 20142

Yes, it is guaranteed to work by the Java Memory Model on modern JVMs. See the section Under the new Java Memory Model in The "Double-Checked Locking is Broken" Declaration.

As other answers have pointed out, there are simpler singleton patterns, using Holder classes or enums. However, in cases like yours, where you want to allow for trying to reinitialize several times if the first try fails, I believe that double-checked locking with a volatile instance variable is fine.

Upvotes: 1

OldCurmudgeon
OldCurmudgeon

Reputation: 65811

Using an AtomicBoolean (much like I suggested here) would be safer and allows for repeat attempts at instantiation on failure.

public static class ControllerFactory {
  // AtomicBolean defaults to the value false.
  private static final AtomicBoolean creatingController = new AtomicBoolean();
  private static volatile Controller controller = null;

  // NB: This can return null if the Controller fails to instantiate or is in the process of instantiation by another thread.
  public static Controller getController() throws ControllerInstantiationException {
    if (controller == null) {
      // Stop another thread creating it while I do.
      if (creatingController.compareAndSet(false, true)) {
        try {
          // Can fail.
          controller = new Controller();
        } catch (Exception ex) {
          // Failed init. Leave it at null so we try again next time.
          controller = null;
          throw new ControllerInstantiationException(ex);
        } finally {
          // Not initialising any more.
          creatingController.set(false);
        }
      } else {
        // Already in progress.
        throw new ControllerInstantiationException("Controller creation in progress by another thread.");
      }
    }
    return controller;
  }

  public static class ControllerInstantiationException extends Exception {
    final Exception cause;

    public ControllerInstantiationException(Exception cause) {
      this.cause = cause;
    }

    public ControllerInstantiationException(String cause) {
      this.cause = new Exception(cause);
    }
  }

  public static class Controller {
    private Controller() {
    }
  }
}

Upvotes: 1

Peter Lawrey
Peter Lawrey

Reputation: 533502

The only thing which is broken is to make the example far more complicated than it needs to be.

All you need is an enum

// a simple lazy loaded, thread safe singleton.
enum Controller {
    INSTANCE
}

Upvotes: 1

Anton
Anton

Reputation: 6051

I believe it's not broken, cause of volatile declaration. But imho better to avoid code like this. There is no guarantee, that this code will work with Java 8 for example. There are another way to create lazy singleton. I always (almost) use this method. First time faced with it in Java Concurrency in Practice book.

public class Singleton {
        private Singleton() { }

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

        public static Singleton getInstance() {
                return SingletonHolder.instance;
        }
}

I don't know what you are doing in your code, it's hard to say, how to tweak it. The most straightforward way, simply use synchronize method. Do you seriously want to receive some performance benefit using double-check-locking ? Is there bottle-neck in synch method ?

Upvotes: 5

Related Questions