svn commit: r1573682 - in /jackrabbit/oak/trunk/oak-core/src: main/java/org/apache/jackrabbit/oak/spi/commit/BackgroundObserver.java test/java/org/apache/jackrabbit/oak/spi/commit/BackgroundObserverTest.java

[mduerig]

Author: mduerig
Date: Mon Mar  3 19:10:48 2014
New Revision: 1573682

URL: http://svn.apache.org/r1573682
Log:
OAK-1486: BackgroundObserverTest occasionally failing
- Fix test to correctly apply memory barrier wrt. the returned assertions
- Revert wrong changes in BackgroundObserver from r1573571

Modified:
    
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/commit/BackgroundObserver.java
    
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/commit/BackgroundObserverTest.java

Modified: 
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/commit/BackgroundObserver.java
URL: 
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/commit/BackgroundObserver.java?rev=1573682&r1=1573681&r2=1573682&view=diff
==============================================================================
--- 
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/commit/BackgroundObserver.java
 (original)
+++ 
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/commit/BackgroundObserver.java
 Mon Mar  3 19:10:48 2014
@@ -260,7 +260,9 @@ public class BackgroundObserver implemen
          */
         public void onComplete(Runnable task) {
             this.task = task;
-            run(task);
+            if (isDone()) {
+                run(task);
+            }
         }
 
         @Override

Modified: 
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/commit/BackgroundObserverTest.java
URL: 
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/commit/BackgroundObserverTest.java?rev=1573682&r1=1573681&r2=1573682&view=diff
==============================================================================
--- 
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/commit/BackgroundObserverTest.java
 (original)
+++ 
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/commit/BackgroundObserverTest.java
 Mon Mar  3 19:10:48 2014
@@ -19,13 +19,12 @@
 
 package org.apache.jackrabbit.oak.spi.commit;
 
-import static com.google.common.collect.Iterables.concat;
 import static java.util.concurrent.Executors.newFixedThreadPool;
 import static 
org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.EMPTY_NODE;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
-import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutorService;
@@ -35,7 +34,6 @@ import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 
 import com.google.common.collect.Lists;
-
 import org.apache.jackrabbit.oak.api.Type;
 import org.apache.jackrabbit.oak.spi.state.NodeState;
 import org.junit.Test;
@@ -43,7 +41,7 @@ import org.junit.Test;
 public class BackgroundObserverTest {
     private static final CommitInfo COMMIT_INFO = new CommitInfo("no-session", 
null);
 
-    private final List<List<Runnable>> assertionLists = Lists.newArrayList();
+    private final List<Runnable> assertions = Lists.newArrayList();
     private CountDownLatch doneCounter;
 
     /**
@@ -52,7 +50,7 @@ public class BackgroundObserverTest {
      */
     @Test
     public void concurrentObservers() throws InterruptedException {
-        Observer observer = createCompositeObserver(newFixedThreadPool(32), 
128);
+        Observer observer = createCompositeObserver(newFixedThreadPool(16), 
128);
 
         for (int k = 0; k < 1024; k++) {
             contentChanged(observer, k);
@@ -61,7 +59,7 @@ public class BackgroundObserverTest {
 
         assertTrue(doneCounter.await(5, TimeUnit.SECONDS));
 
-        for (Runnable assertion : concat(assertionLists)) {
+        for (Runnable assertion : assertions) {
             assertion.run();
         }
     }
@@ -86,22 +84,22 @@ public class BackgroundObserverTest {
         return observer;
     }
 
+    private synchronized void done(List<Runnable> assertions) {
+        this.assertions.addAll(assertions);
+        doneCounter.countDown();
+    }
+
     private Observer createBackgroundObserver(ExecutorService executor) {
         return new BackgroundObserver(new Observer() {
-            final List<Runnable> assertions = newAssertionList();
-
-            private List<Runnable> newAssertionList() {
-                ArrayList<Runnable> assertionList = Lists.newArrayList();
-                assertionLists.add(assertionList);
-                return assertionList;
-            }
-
-            NodeState previous;
+            // Need synchronised list here to maintain correct memory barrier
+            // when this is passed on to done(List<Runnable>)
+            final List<Runnable> assertions = 
Collections.synchronizedList(Lists.<Runnable>newArrayList());
+            volatile NodeState previous;
 
             @Override
             public void contentChanged(@Nonnull final NodeState root, 
@Nullable CommitInfo info) {
                 if (root.hasProperty("done")) {
-                    doneCounter.countDown();
+                    done(assertions);
                 } else if (previous != null) {
                     // Copy previous to avoid closing over it
                     final NodeState p = previous;