Fix race condition in P1.memo()

knutwalker · knutwalker · commit a0d4ec5f15a0 · 2015-02-11T12:58:51.000+01:00
The race was when two thread encountered the uninitialized state, `a == null`. One thread gets suspended before entering the `synchronized` block, while the other runs though it. The latter thread sets `v` to something not null. Then the first thread resumes, enters the `synchronized` block and checks if `(v == null || v.get() == null)` which is now false, and thus returns the still uninitialized `a`. Fixes #105
diff --git a/core/src/main/java/fj/P1.java b/core/src/main/java/fj/P1.java
@@ -223,9 +223,12 @@ public A _1() {
             A a = v != null ? v.get() : null;
             if (a == null)
               synchronized (latch) {
-                if (v == null || v.get() == null)
+                if (v == null || v.get() == null) {
                   a = self._1();
-                v = new SoftReference<A>(a);
+                  v = new SoftReference<A>(a);
+                } else {
+                  a = v.get();
+                }
               }
             return a;
           }
diff --git a/core/src/test/java/fj/P1Test.java b/core/src/test/java/fj/P1Test.java
@@ -0,0 +1,31 @@
+package fj;
+
+import org.junit.Test;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+public final class P1Test {
+
+  @Test
+  public void bug105() throws Exception {
+    final P1<String> p1 = P.p("Foo").memo();
+    final AtomicInteger nullCounter = new AtomicInteger();
+    ExecutorService executorService = Executors.newCachedThreadPool();
+
+    for (int i = 0; i < 10000; i++) {
+      executorService.submit(() -> {
+        if (p1._1() == null) {
+          nullCounter.incrementAndGet();
+        }
+      });
+    }
+
+    executorService.shutdown();
+    executorService.awaitTermination(10, TimeUnit.DAYS);
+
+    org.junit.Assert.assertEquals("Race condition in P1.memo()", 0, nullCounter.get());
+  }
+}
