Initialisation paresseuse incorrecte

Findbug m’a dit que j’utilisais une initialisation paresseuse incorrecte.

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

Je ne vois rien de mal ici. Est-ce un mauvais comportement de findbug, ou j’ai raté quelque chose?

Findbug fait référence à un problème de threading potentiel. Dans un environnement multi-thread, votre singleton pourrait être créé plusieurs fois avec votre code actuel.

Il y a beaucoup de lectures ici , mais cela aidera à expliquer.

La condition de course est ici sur la if check . Au premier appel, un thread entrera dans la if check et créera l’instance et l’assignera à «instance». Mais il est possible qu’un autre thread devienne actif entre la if check et la création / affectation de l’instance. Ce thread peut également passer le if check car l’affectation n’a pas encore eu lieu. Par conséquent, deux instances (ou plus, si plusieurs threads entraient) seraient créées et vos threads auraient des références à des objects différents.

Votre code est légèrement plus complexe que nécessaire, ce qui pourrait expliquer sa confusion.

Edit: C’est certainement le problème de threading que les autres ont posté, mais j’ai pensé que je mettrais en place l’implémentation de vérification à double locking ici pour référence ci-dessous:

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

REMARQUE : La solution de vérification à double locking de JohnKlehm est meilleure. Laissant cette réponse ici pour des raisons historiques.

Il devrait effectivement être

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

Vous devez mettre un verrou autour de l’instanciation pour que cela soit correct

LI: initialisation paresseuse incorrecte du champ statique (LI_LAZY_INIT_STATIC)

Cette méthode contient une initialisation différée non synchronisée d’un champ statique non volatile. Comme le compilateur ou le processeur peut réorganiser les instructions, il n’est pas garanti que les threads voient un object complètement initialisé si la méthode peut être appelée par plusieurs threads. Vous pouvez rendre le champ volatile pour corriger le problème. Pour plus d’informations, consultez le site Web Java Memory Model.

Merci à John Klehm pour l’exemple publié

peut également essayer d’assigner l’instance d’object dans le bloc sychronised directement

 synchronized (MyCurrentClass.myLock=new Object()) 

c’est à dire

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

Vous avez manqué le problème du multi threading,

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

votre object statique n’est pas synchronisé. De plus, votre méthode n’est pas une initialisation paresseuse. Normalement, ce que vous faites est de conserver une carte d’object et d’initialiser celle qui est souhaitée à la demande. Donc, vous ne les initialisez pas tous au début plutôt que de les appeler quand cela est nécessaire (appelé).

Depuis 1.5: l’instance doit être volatile et vous devez intégrer une variable tmp pour éviter d’utiliser une instance créée mais son initialisation n’est pas encore terminée.

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