Wrong lazy initialization

Question

Findbug told me that I am using the wrong lazy initialization.

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

I don’t see anything bad here. Is this the wrong findbug behavior, or am I missing something?

Answer 1

Findbug refers to a potential thread issue. In a multi-threaded environment, it is likely that your singleton will be created multiple times with your current code.

It reads a lot here, but it helps to explain.

The race status here is on if check . At the first call, the thread will fall into the if check and create an instance and assign it to the "instance". But there is the potential for another thread to become active between if check and instance creation / assignment. This thread can also pass if check , because the assignment has not yet been completed. This way, two (or more if more threads) are created, and your threads will refer to different objects.

Answer 2

Your code is a little more complicated than necessary and therefore can be confused.

Edit: this is definitely a threading issue, like others, but I thought that I would post the implementation of double-lock verification here for reference below:

 private static final Object lock = new Object(); private static volatile Object instance; // must be declared volatile public static Object getInstance() { if (instance == null) { // avoid sync penalty if we can synchronized (lock) { // declare a private static Object to use for mutex if (instance == null) { // have to do this inside the sync instance = new Object(); } } } return instance; } 

Answer 3

NOTE : the best JohnKlehm double lock verification solution. Leaving this answer here for historical reasons.

In fact, it should be

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

Answer 4

You need to put a lock to create an instance to do it right

LI: invalid lazy static field initialization (LI_LAZY_INIT_STATIC)

This method contains unsynchronized lazy initialization an unstable static field. Because the compiler or processor can reorder instructions, threads do not guarantee that you will see a fully initialized object if a method can be called by multiple threads. You can make volatile field to fix the problem. See the Java memory model website for more information.

Answer 5

Thanks to John Klehm for the published sample.

may also try to directly assign an instance of an object in a synchronized block

 synchronized (MyCurrentClass.myLock=new Object()) 

i.e.

 private static volatile Object myLock = new Object(); public static Object getInstance() { if (instance == null) { // avoid sync penalty if we can synchronized (MyCurrentClass.myLock**=new Object()**) { // declare a private static Object to use for mutex if (instance == null) { // have to do this inside the sync instance = new Object(); } } } return instance; } 

Answer 6

You missed the problem with multiple threads,

 private static Object instance; public static synchronized Object getInstance() { return (instance != null ? instance : (instance = new Object())); } 

Answer 7

your static object is out of sync. Also, your method is not lazy initialization. Usually, what you do, you save a map of the object, and you initialize what you want on demand. Thus, you do not initialize all of them at the beginning, but do not call them when necessary (called).

Answer 8

Starting with version 1.5: the instance must be volatile and yould integrate the tmp variable in order to avoid using the created instance, but its initialization has not yet been completed.

 private static volatile Object myLock = new Object(); private static volatile Object instance; public static Object getInstance() { if (instance == null) { Object tmpObj; synchronized (myLock) { tmpObj = instance; if (tmpObj == null) { tmpObj = new Object(); } } instance = tmpObj; } return instance; }