Неправильная ленивая инициализация

Findbug сказал мне, что я использую неправильную ленивую инициализацию.

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

    instance = new Object();
    return instance;
}

Я не вижу ничего плохого здесь. Это неправильное поведение findbug, или я что-то упустил?

8 53
findbugs

8 ответов:

Findbug ссылается на потенциальную проблему с потоками. В многопоточной среде существует вероятность того, что ваш синглтон будет создан более одного раза с вашим текущим кодом.

Здесь много чтения , но это поможет объяснить.

Условие гонки здесь находится на if check. При первом вызове поток войдет в if check, создаст экземпляр и назначит его "экземпляру". Но есть потенциал для того, чтобы другая нить стала активной между ними. if check и создание/назначение экземпляра. Этот поток также может передать if check, потому что назначение еще не произошло. Таким образом, будут созданы два экземпляра (или больше, если войдет больше потоков), и ваши потоки будут иметь ссылки на разные объекты.

59

Ваш код немного сложнее, чем требуется, что может быть причиной путаницы.

Edit: это определенно проблема с потоками, как и другие опубликованные, но я подумал, что я бы разместил реализацию проверки двойной блокировки здесь для справки ниже: 

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

Примечание : решение проверки двойного замка Джонклема лучше. Оставляя этот ответ здесь по историческим причинам.

На самом деле это должно быть 

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

    return instance;
}
11

Вам нужно поставить блокировку вокруг экземпляра, чтобы сделать это правильно

LI: неправильная ленивая инициализация статического поля (LI_LAZY_INIT_STATIC)

Этот метод содержит несинхронизированную ленивую инициализацию a энергонезависимое статическое поле. Потому что компилятор или процессор может переупорядочить инструкции, потоки не гарантированно увидеть полностью инициализированный объект, если метод может быть вызван несколькими потоками. Вы можете сделать поле изменчивым, чтобы исправить проблема. Для большего сведения см. На веб-сайте модели памяти Java.

4

Спасибо Джону клему за опубликованный образец

Также может попытаться назначить экземпляр объекта в синхронизированном блоке напрямую 

synchronized (MyCurrentClass.myLock=new Object())

То есть 

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;

}
2

Вы пропустили проблему многопоточности, 

private static Object instance;

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

Ваш статический объект не синхронизирован. Более того, ваш метод не является ленивой инициализацией. Обычно вы храните карту объекта и инициализируете нужный объект по требованию. Таким образом, вы не инициализируете все из них в начале, а не вызываете их, когда это необходимо(вызывается).

0

Начиная с 1.5: экземпляр должен быть летучим, и вы должны интегрировать переменную tmp, чтобы избежать использования экземпляра, который создан, но его инициализация еще не завершена.

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;

}
0