Incorrect lazy initialization

Question

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?

Maybe it expects `if (instance == null) instance = new Object(); return instance;` ? — JAB, Jul 21 '11 at 20:56
@Matt: it is lazy initialisation alright, and it only makes a singleton if all of the class's constructors are private. — tdammers, Jul 21 '11 at 21:04
Yes, it does seem to expect the pattern to be: ``` if (instance == null) { instance = new Object(); } return instance; ``` — intel_chris, May 16 '21 at 12:19

score 66 · Accepted Answer · edited Oct 16 '12 at 14:50

66

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.

edited Oct 16 '12 at 14:50

Diarmaid

2,706
2
16
19

answered Jul 21 '11 at 20:56

nicholas.hauschild

42,483
9
127
120

19

Making the method `synchronized` would be the simplest cure to this problem. – John Flatness Jul 21 '11 at 20:59
@John: Correct, that is almost always the best solution in this case. – nicholas.hauschild Jul 21 '11 at 21:06
5

Just making it synchronized will incur the synchronization overhead on each call; you only need it on the first call. – antlersoft Jul 21 '11 at 21:18
@antlersoft, But you don't actually even need it on a non-threaded app. – Pacerier Jun 13 '14 at 14:33
1

Multiple instantiation is only one possibility. Even worse, the compiler could reorder instructions and the caller could have an instance of a partially constructed object with default values. – mvd Aug 18 '14 at 21:09

score 24 · Answer 2 · edited Dec 23 '15 at 20:20

24

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;
}

edited Dec 23 '15 at 20:20

Jared Burrows

54,294
25
151
185

answered Jul 21 '11 at 20:57

JohnKlehm

2,368
15
9

Is this really an improvement over making the whole method synchronized and only doing the test once? – Daniel Lyons Jul 21 '11 at 21:11
2

I've always been told that synchronization is more expensive than an if check. I've never actually measured it myself. – JohnKlehm Jul 21 '11 at 21:15
Read here for a good discussion on how to do a double lock check (its more tricky than it should be): http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html – JohnKlehm Jul 21 '11 at 21:37
1

For this to work properly, your lock must be `volatile`. I know you have already illustrated this, but I think you should make it a bit more explicit in the answer. – nicholas.hauschild May 26 '13 at 17:08
I've added a comment to call attention to the volatile keyword – JohnKlehm May 28 '13 at 18:00
5

It doesn't help to declare the variable `myLock` as `volatile`. Instead, you have to declare the variable `instance` as `volatile`. – nosid Sep 13 '13 at 12:19
@nosid Excellent observation, I misspoke when I suggested that the Lock should be `volatile`. It is indeed the `instance` variable that should be `volatile`. – nicholas.hauschild May 08 '14 at 13:47

Varun Achar · Answer 3 · 2015-12-11T12:25:06.750

13

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;
}

edited Dec 11 '15 at 12:25

answered Jul 21 '11 at 21:22

Varun Achar

14,781
7
57
74

I like this code better than how it was originally, but this does not explain why the original code is wrong. – emory Jul 21 '11 at 22:11
At the time of posting one of the answers had already mentioned the ill effects of a non synchronized lazy instantitation in a multi threaded enviornment. I just put it into code. :) – Varun Achar Jul 22 '11 at 05:16
This still doesn't fix the violation though. – Brian Pipa Dec 16 '13 at 20:55
Ah - it's synchronized now. I missed that (or it was changed). With synchronized, it does fix the problem :) – Brian Pipa Mar 25 '14 at 17:50

score 4 · Answer 4 · answered Jul 21 '11 at 20:59

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.

score 3 · Answer 5 · answered Jul 05 '16 at 10:32

3

You missed multi threading issue,

private static Object instance;

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

answered Jul 05 '16 at 10:32

Nilesh

2,089
3
29
53

score 2 · Answer 6 · edited Mar 06 '12 at 18:11

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;

}

score 0 · Answer 7 · answered Aug 25 '17 at 15:06

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;

}

Shahriar · Answer 8 · 2011-07-21T21:07:50.193

0

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).

edited Jul 21 '11 at 21:07

answered Jul 21 '11 at 21:00

Shahriar

825
1
15
31

Incorrect lazy initialization

8 Answers8

Linked