Synchronization of getInstance might fail?

robinschmid · web-flow · commit a5261ebc7a92 · 2021-04-29T14:46:25.000+02:00
Hey everyone,

I found this getInstance method generating a static singleton. I think the second `result = instance` should be moved into the synchronized block. Otherwise, this might result in multiple instances.

One question, is it necessary to use a local variable here? (I mean it does not hurt either)
This could also be changed to:
```java
    public static IChemObjectBuilder getInstance() { 
        if (instance == null) {
            synchronized (LOCK) { 
                if (instance == null) {
                    instance = new SilentChemObjectBuilder();
                }
            }
        }
        return instance;
    }
```
diff --git a/base/silent/src/main/java/org/openscience/cdk/silent/SilentChemObjectBuilder.java b/base/silent/src/main/java/org/openscience/cdk/silent/SilentChemObjectBuilder.java
@@ -220,8 +220,8 @@ public void modify(DoubleBondStereochemistry instance) {
     public static IChemObjectBuilder getInstance() {
         IChemObjectBuilder result = instance;
         if (result == null) {
-            result = instance;
             synchronized (LOCK) {
+                result = instance;
                 if (result == null) {
                     instance = result = new SilentChemObjectBuilder();
                 }
