Double-checked locking and Singleton

[avihai marchiano]

Hey,

The following article explian about issues that can pop up when you implement Singleton.

http://www.javaworld.com/javaworld/jw-04-2003/jw-0425-designpatterns.html?page=3

They claim there that the double checked locking is not good because -

Unfortunately, double-checked locking is not guaranteed to work because the compiler is free to assign a value to the singleton member variable before the singleton's constructor is called. If that happens, Thread 1 can be preempted after the singleton reference has been assigned, but before the singleton is initialized, so Thread 2 can return a reference to an uninitialized singleton instance.

Is this problem still relevant to java 5?

What is the solution in case that my constructor needs to throw exceptions? (In this case I can�t initialize the singleton instance in the declaration as suggested by them)

[Ernest Friedman-Hill]

Basically you just want to synchronize the whole access method -- i.e.,

private static Foo theFoo;
public static synchronized Foo getInstance() {
    if (theFoo == null)
        theFoo = new Foo();
    return theFoo;
}

People worry far too much about the cost of synchronization. It's quite minimal, really.

[avihai marchiano]

This is one of the suggested solution.

but -

however, the astute reader may realize that the getInstance() method only needs to be synchronized the first time it is called. Because synchronization is very expensive performance-wise (synchronized methods can run up to 100 times slower than unsynchronized methods),

[Jim Yingst]

Synchronization has gotten much cheaper over the years. Frankly I think that article in 2003 was already rather out-of-date in this area, and now it's more out-of date. Synchronization is not that big a deal, performance-wise, and much convoluted, hard-to-maintain code has been written needlessly by people trying to avoid it.

As for your original question, Java 5 does allow the double-checked locking technique to work correctly, if you mark the variable as volatile. If you don't mark the variable volatile, it's still broken. See this FAQ for more info. And note that they suggest a better way to achieve a lazily-initialized singleton anyway:

public class Something {
  private static class LazySomethingHolder {
    public static Something something = new Something();
  }
 
  public static Something getInstance() {
    return LazySomethingHolder.something;
  }
 
  ...
}

[Stan James]

Oh, neat, the holder makes it lazy? If you don't care about that - the only methods you use on Something are the member methods on the Singleton - can we just leave the holder out?

// non-lazy 
public class Something {
  private static Something something = new Something();
 
  public static Something getInstance() {
    return something;
  }
 
  ...
}

edit - fixed code tag
[ September 20, 2007: Message edited by: Stan James ]

[avihai marchiano]

The problem with this is you cant use this code in case that the constrctur throw exceptions.

[Ernest Friedman-Hill]

Originally posted by avihai marchiano:
The problem with this is you cant use this code in case that the constrctur throw exceptions.

No, there's a fairly trivial extension to this:

    public class Something {
      private static class LazySomethingHolder {
        public static Something something;
        static {
            try {
                something = new Something();
            } catch (Whatever ex) {
                throw new RuntimeException(ex);
            }
        }
      }
     
      public static Something getInstance() {
        return LazySomethingHolder.something;
      }
     
      ...
    }

[avihai marchiano]

This is a big restriction to dont allow to throw any exception that is not run time exception.

I think that if the memory mechanism fixed in 1.5 it will be better to use in the violatile solution.

[Jim Yingst]

[Stan]: Oh, neat, the holder makes it lazy?

Yep. This is guaranteed in JLS 12.4.1 - the Holder class won't be initialized until just before the first time its static field is used.

[Stan]: If you don't care about that - the only methods you use on Something are the member methods on the Singleton - can we just leave the holder out?

Absolutely. A non-lazy singleton is even easier for people to understand, and often laziness is not necessary or even desirable.

[EFH]: No, there's a fairly trivial extension to this:

Well, if a runtime exception is thrown by a static initializer, then what happens is the JVM wraps it with an ExceptionInInitializerError. So I'm inclined to just throw that in the first place:

public class Something {
    private static class Holder {
        final static Something instance;
        static {
            try {
                instance = new Something();
            } catch (SomeCheckedException e) {
                throw new ExceptionInInitializerError(e);
            }
        }
    }
 
    public static Something getInstance() {
        return Holder.instance;
    }
}

This is getting a bit ugly though, interfering with the initial simplicity of the Holder code.

Also annoying is the fact that if someone does catch this Error, then the second time someone tries to call getInstance(), it throws a NoClassDefFoundError, which has been known to confuse people. From the JVM's perspective, it knows it already found the one class definition, but it errored when it tried to load that definition. So now it just errors again rather than retrying.

[avihai]: This is a big restriction to dont allow to throw any exception that is not run time exception.

Hm, I suppose it depends on where you stand on the use of checked exceptions vs. unchecked exceptions or Errors. Personally I tend to avoid throwing checked exceptions, as I want the code calling getInstance() to be easy to write, and I generally figure that if the singleton instance can't be created, there usually isn't a good way to recover anyway. It's usually some sort of configuration error, and the user needs to fix the configuration and restart the application. So throwing an Error in this case usually suits me just fine. If you think you need a better recovery strategy, then yes, throwing a checked exception may be more appropriate.

[avihai]: I think that if the memory mechanism fixed in 1.5 it will be better to use in the violatile solution.

Eh, I'd probably still prefer using simple synchronization:

public class Something {
    private static Something instance;
 
    public static synchronized Something getInstance() throws new YouAreScrewedException {
        if (instance == null)
            instance = new Instance();
        return instance;
    }
}

That way (a) I don't have to worry about what happens if someone runs this on an older JVM, and (b) I don't have to spend time convincing other developers (who have heard that double-checked locking is broken) that it's now OK when using volatile and JDK 5+. Because, again, the supposed "cost" of synchronization is usually really trivial compared to the energy people spend trying to avoid it.

[Ernest Friedman-Hill]

For some reason people seem to think that "volatile" is free -- it's not free, and in fact, in the limit of the most efficient possible implementation of "synchronized", and uncontested access after initialization, the synchronized solution and the volatile/double-checked solution ought to cost about the same. Because both require invalidating a cache line and going to main memory for the data, then testing the value of the variable before returning it.

P.S. RE ExceptionInInitializer: yeah, I forgot the name of the class and was just too lazy to look that up. That would be better. But remember, I advocated just synchronizing the whole thing in the first place.

[Ernest Friedman-Hill]

What is all this gibberish supposed to do?

[avihai marchiano]

I didnt understand too.

[Nicholas Jordan]

[Ernest Friedman-Hill:] What is all this gibberish supposed to do? (8:24 AM)
[avihai marchiano :]I didnt understand too. (10:59 AM)

Okay, I deleted it.

The discussion had addressed performance cost of synchronization.

I thought I should be revalatory and showed the code used to obtain test results.

There was a difference of 32.293 seconds between the cost of newing one million objects through a synchronized method call v same method call without the synchronized keyword.

There may be some initialization overhead, but did not try to produce histograms on the basis that doing this testing on a uni-processor machine may not be effective.

You two may have my apologies if you want it.