Incorrect lazy initialization
JavaFindbugsJava Problem Overview
Findbug told me that I use incorrect lazy initialization.
public static Object getInstance() {
if (instance != null) {
return instance;
}
instance = new Object();
return instance;
}
I don't see anything wrong here. Is it wrong behaviour of findbug, or I missed something?
Java Solutions
Solution 1 - Java
Findbug is referencing a potential threading issue. In a multi thread environment, there would be potential for your singleton to be created more than once with your current code.
There is a lot of reading here, but it will help explain.
The race condition here is on the if check
. On the first call, a thread will get into the if check
, and will create the instance and assign it to 'instance'. But there is potential for another thread to become active between the if check
and the instance creation/assignment. This thread could also pass the if check
because the assignment hasn't happened yet. Therefore, two (or more, if more threads got in) instances would be created, and your threads would have references to different objects.
Solution 2 - Java
Your code is slightly more complex than needed which might be why it's confused.
Edit: It's definitely the threading issue as the others posted but thought I'd post the double lock check implementation 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;
}
Solution 3 - Java
NOTE: JohnKlehm's double lock checking solution is better. Leaving this answer here for historical reasons.
It should actually be
public synchronized static Object getInstance() {
if (instance == null) {
instance = new Object();
}
return instance;
}
Solution 4 - Java
You need to put a lock around instantiation to make this correct
> LI: Incorrect lazy initialization of static field > (LI_LAZY_INIT_STATIC) > > This method contains an unsynchronized lazy initialization of a > non-volatile static field. Because the compiler or processor may > reorder instructions, threads are not guaranteed to see a completely > initialized object, if the method can be called by multiple threads. > You can make the field volatile to correct the problem. For more > information, see the Java Memory Model web site.
Solution 5 - Java
Thanks to John Klehm for posted sample
also may try to assign object instance in sychronised block directly
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;
}
Solution 6 - Java
You missed multi threading issue,
private static Object instance;
public static synchronized Object getInstance() {
return (instance != null ? instance : (instance = new Object()));
}
Solution 7 - Java
your static object is not synchronized. Moreover your method is not a lazy initialization. Normally what you do is you keep a Map of object,and you initialize the desired one on demand. So you do not initialize all of them at the beginning rather than calling them when it is needed(called).
Solution 8 - Java
Since 1.5: the instance should be volatile and yould integrate a tmp variable to avoid using an instance that is created but its initialization is not finished yet.
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;
}