Make RunNotifier code concurrent.

Squashed commit of the following commits from #625: commit a400a3a Author: Kevin Cooney <[email protected]> Date: Sat Mar 2 08:49:39 2013 -0800 Move wrapIfNotThreadSafe() to RunNotifier commit 38ac72e Author: Kevin Cooney <[email protected]> Date: Thu Feb 28 22:37:29 2013 -0800 Revert changes to build.xml commit e1172af Author: Kevin Cooney <[email protected]> Date: Thu Feb 28 22:13:52 2013 -0800 minor cleanup commit b11b5aa Author: Kevin Cooney <[email protected]> Date: Mon Feb 25 17:09:10 2013 -0800 Undo changes to notice commit a85566f Author: Kevin Cooney <[email protected]> Date: Mon Feb 25 17:08:11 2013 -0800 Make code style consistent commit 4b8efa1 Author: Kevin Cooney <[email protected]> Date: Sun Feb 24 17:45:23 2013 -0800 Update in response to code review commends by dsaff and Tibor17 commit a6fb342 Author: Kevin Cooney <[email protected]> Date: Sun Feb 24 17:44:29 2013 -0800 Make RunNotifierTest a JUnit4-style test again commit 6b39220 Author: Kevin Cooney <[email protected]> Date: Fri Feb 15 08:38:43 2013 -0800 Remove copyright commit 6a65cd9 Author: Kevin Cooney <[email protected]> Date: Thu Feb 14 17:50:25 2013 -0800 Remove unrelated changes commit 3997816 Merge: 10c5130 dbe8a97 Author: Kevin Cooney <[email protected]> Date: Thu Feb 14 17:40:40 2013 -0800 Merge branch 'master' into concurrent-run-listeners commit 10c5130 Author: Kevin Cooney <[email protected]> Date: Thu Feb 14 11:03:39 2013 -0800 Remove mention of jcip-annotations from NOTICE.txt Reduce diffs commit 2873269 Author: Kevin Cooney <[email protected]> Date: Thu Feb 14 10:46:09 2013 -0800 Replace Concurrent annotation with RunNotifier.ThreadSafe Update Javadoc Delete AddRemoveListenerTest (functionality covered by RunNotifierTest) commit 58a5a92 Merge: 02f1486 cc7d45b Author: Kevin Cooney <[email protected]> Date: Thu Feb 7 08:56:08 2013 -0800 Remove dependency on jcip-annotations commit cc7d45b Merge: 789c302 02f1486 Author: Kevin Cooney <[email protected]> Date: Thu Feb 7 08:55:24 2013 -0800 Merge branch 'concurrent-run-listeners' into concurrent-run-listeners_remove-jcip-deps Conflicts: src/test/java/org/junit/runner/notification/SynchronizedRunListenerTest.java commit 789c302 Author: Kevin Cooney <[email protected]> Date: Thu Feb 7 08:53:11 2013 -0800 Remove tabs commit 02f1486 Merge: a04aae1 708d972 Author: Kevin Cooney <[email protected]> Date: Thu Feb 7 08:47:40 2013 -0800 Merge branch 'Tibor17-junit.issues' into concurrent-run-listeners Conflicts: src/test/java/org/junit/runner/notification/SynchronizedRunListenerTest.java src/test/java/org/junit/tests/AllTests.java commit 708d972 Author: Tibor Digana <[email protected]> Date: Thu Feb 7 01:07:10 2013 +0100 threadsafe annotation commit 796801e Author: Kevin Cooney <[email protected]> Date: Mon Feb 4 17:17:16 2013 -0800 Remove jcip-annotations dependency; add @Concurrent annotation commit e30fe38 Author: Tibor Digana <[email protected]> Date: Tue Feb 5 01:18:13 2013 +0100 AllTests run SynchronizedRunListenerTest commit 0c35851 Author: Tibor Digana <[email protected]> Date: Tue Feb 5 01:11:43 2013 +0100 clarified the purpose of SynchronizedRunListener commit f8016a4 Author: Tibor Digana <[email protected]> Date: Tue Feb 5 01:09:25 2013 +0100 merged from https://github.com/kcooney/junit/blob/c099a99b76ccd985639882369652dfcadb3722b4/.classpath commit a5e8a71 Author: Tibor Digana <[email protected]> Date: Tue Feb 5 01:03:44 2013 +0100 merged from https://github.com/kcooney/junit/blob/41626fd3022c0d73d6f02807be0344a9cf6acafa/src/test/java/org/junit/runner/notification/SynchronizedRunListenerTest.java commit a04aae1 Author: Kevin Cooney <[email protected]> Date: Mon Feb 4 13:31:29 2013 -0800 Add tests in AddRemoveTestListenerTest to RunNotifierTest commit 5112c1a Merge: 41626fd c220ad7 Author: Kevin Cooney <[email protected]> Date: Mon Feb 4 13:07:37 2013 -0800 Merge branch 'Tibor17-junit.issues' into concurrent-run-listeners Conflicts: build.xml src/main/java/org/junit/runner/notification/RunNotifier.java src/main/java/org/junit/runner/notification/SynchronizedRunListener.java src/test/java/org/junit/tests/AllTests.java commit c220ad7 Author: Tibor Digana <[email protected]> Date: Mon Feb 4 00:42:22 2013 +0100 using COWAL + backward compatible + SynchronizedRunListener commit 41626fd Author: Kevin Cooney <[email protected]> Date: Sun Feb 3 09:54:24 2013 -0800 Cleanup implementation and tests commit c099a99 Author: Kevin Cooney <[email protected]> Date: Sat Feb 2 17:26:39 2013 -0800 Fix equals method for SynchronizedRunListener commit 1d6914b Author: Tibor Digana <[email protected]> Date: Fri Feb 1 00:42:38 2013 +0100 backward compatible, SynchronizedRunListener, removed EqualRunListener commit 21a926e Author: Tibor Digana <[email protected]> Date: Wed Jan 30 20:01:55 2013 +0100 problem to remove synchronized listener commit 20c6b53 Merge: 573828c 27ba66f Author: Tibor Digana <[email protected]> Date: Tue Jan 29 19:37:25 2013 +0100 Merge branch 'master' of git://github.com/KentBeck/junit into junit.issues commit 573828c Author: Tibor Digana <[email protected]> Date: Sun Jan 20 22:38:12 2013 +0100 AllTests commit 497e6ca Merge: a691962 3aca014 Author: Tibor Digana <[email protected]> Date: Sun Jan 20 22:29:44 2013 +0100 Merge branch 'master' of git://github.com/KentBeck/junit into junit.issues commit a691962 Author: Tibor Digana <[email protected]> Date: Sun Jan 20 22:28:12 2013 +0100 #realUsage test, simplified commit d2f1710 Author: Tibor Digana <[email protected]> Date: Thu Dec 20 16:06:19 2012 +0100 1E3 -> 1000, 1E2 -> 100 commit 4df3e81 Author: Tibor Digana <[email protected]> Date: Wed Dec 19 19:41:23 2012 +0100 synchronized substituted by thread safe collection
junit-team · Mar 6, 2013 · 72af03c · 72af03c
1 parent 6fd44da
commit 72af03c
Show file tree

Hide file tree

Showing 8 changed files with 626 additions and 54 deletions.
diff --git a/src/main/java/org/junit/runner/Result.java b/src/main/java/org/junit/runner/Result.java
@@ -65,6 +65,7 @@ public boolean wasSuccessful() {
         return getFailureCount() == 0;
     }
 
+    @RunListener.ThreadSafe
     private class Listener extends RunListener {
         @Override
         public void testRunStarted(Description description) throws Exception {

diff --git a/src/main/java/org/junit/runner/notification/RunListener.java b/src/main/java/org/junit/runner/notification/RunListener.java
@@ -1,13 +1,20 @@
 package org.junit.runner.notification;
 
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
 import org.junit.internal.AssumptionViolatedException;
 import org.junit.runner.Description;
 import org.junit.runner.Result;
 
 /**
- * <p>If you need to respond to the events during a test run, extend <code>RunListener</code>
- * and override the appropriate methods. If a listener throws an exception while processing a
- * test event, it will be removed for the remainder of the test run.</p>
+ * Register an instance of this class with {@link RunNotifier} to be notified
+ * of events that occur during a test run. All of the methods in this class
+ * are abstract and have no implementation; override one or more methods to
+ * receive events.
  *
  * <p>For example, suppose you have a <code>Cowbell</code>
  * class that you want to make a noise whenever a test fails. You could write:
@@ -18,7 +25,6 @@
  *    }
  * }
  * </pre>
- * </p>
  *
  * <p>To invoke your listener, you need to run your tests through <code>JUnitCore</code>.
  * <pre>
@@ -28,23 +34,36 @@
  *    core.run(MyTestClass.class);
  * }
  * </pre>
- * </p>
+ *
+ * <p>If a listener throws an exception for a test event, the other listeners will
+ * have their {@link RunListener#testFailure(Failure)} called with a {@code Description}
+ * of {@link Description#TEST_MECHANISM} to indicate the failure.
+ *
+ * <p>By default, JUnit will synchronize calls to your listener. If your listener
+ * is thread-safe and you want to allow JUnit to call your listener from
+ * multiple threads when tests are run in parallel, you can annotate your
+ * test class with {@link RunListener.ThreadSafe}.
+ *
+ * <p>Listener methods will be called from the same thread as is running
+ * the test, unless otherwise indicated by the method Javadoc
  *
  * @see org.junit.runner.JUnitCore
  * @since 4.0
  */
 public class RunListener {
 
     /**
-     * Called before any tests have been run.
+     * Called before any tests have been run. This may be called on an
+     * arbitrary thread.
      *
      * @param description describes the tests to be run
      */
     public void testRunStarted(Description description) throws Exception {
     }
 
     /**
-     * Called when all tests have finished
+     * Called when all tests have finished. This may be called on an
+     * arbitrary thread.
      *
      * @param result the summary of the test run, including all the tests that failed
      */
@@ -69,7 +88,16 @@ public void testFinished(Description description) throws Exception {
     }
 
     /**
-     * Called when an atomic test fails.
+     * Called when an atomic test fails, or when a listener throws an exception.
+     *
+     * <p>In the case of a failure of an atomic test, this method will be called
+     * with the same {@code Description} passed to
+     * {@link #testStarted(Description)}, from the same thread that called
+     * {@link #testStarted(Description)}.
+     *
+     * <p>In the case of a listener throwing an exception, this will be called with
+     * a {@code Description} of {@link Description#TEST_MECHANISM}, and may be called
+     * on an arbitrary thread.
      *
      * @param failure describes the test that failed and the exception that was thrown
      */
@@ -94,6 +122,20 @@ public void testAssumptionFailure(Failure failure) {
      */
     public void testIgnored(Description description) throws Exception {
     }
-}
 
 
+    /**
+     * Indicates a {@code RunListener} that can have its methods called
+     * concurrently. This implies that the class is thread-safe (i.e. no set of
+     * listener calls can put the listener into an invalid state, even if those
+     * listener calls are being made by multiple threads without
+     * synchronization).
+     *
+     * @since 4.12
+     */
+    @Documented
+    @Target(ElementType.TYPE)
+    @Retention(RetentionPolicy.RUNTIME)
+    public @interface ThreadSafe {
+    }
+}
diff --git a/src/main/java/org/junit/runner/notification/RunNotifier.java b/src/main/java/org/junit/runner/notification/RunNotifier.java
@@ -3,9 +3,9 @@
 import static java.util.Arrays.asList;
 
 import java.util.ArrayList;
-import java.util.Collections;
-import java.util.Iterator;
+import java.util.Collection;
 import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 import org.junit.internal.AssumptionViolatedException;
 import org.junit.runner.Description;
@@ -21,24 +21,39 @@
  * @since 4.0
  */
 public class RunNotifier {
-    private final List<RunListener> fListeners =
-            Collections.synchronizedList(new ArrayList<RunListener>());
+    private final List<RunListener> fListeners = new CopyOnWriteArrayList<RunListener>();
     private volatile boolean fPleaseStop = false;
 
     /**
      * Internal use only
      */
     public void addListener(RunListener listener) {
-        fListeners.add(listener);
+        if (listener == null) {
+            throw new NullPointerException("Cannot add a null listener");
+        }
+        fListeners.add(wrapIfNotThreadSafe(listener));
     }
 
     /**
      * Internal use only
      */
     public void removeListener(RunListener listener) {
-        fListeners.remove(listener);
+        if (listener == null) {
+            throw new NullPointerException("Cannot remove a null listener");
+        }
+        fListeners.remove(wrapIfNotThreadSafe(listener));
+    }
+
+    /**
+     * Wraps the given listener with {@link SynchronizedRunListener} if
+     * it is not annotated with {@link RunListener.ThreadSafe}.
+     */
+    RunListener wrapIfNotThreadSafe(RunListener listener) {
+        return listener.getClass().isAnnotationPresent(RunListener.ThreadSafe.class) ?
+                listener : new SynchronizedRunListener(listener, this);
     }
 
+
     private abstract class SafeNotifier {
         private final List<RunListener> fCurrentListeners;
 
@@ -51,21 +66,18 @@ private abstract class SafeNotifier {
         }
 
         void run() {
-            synchronized (fListeners) {
-                List<RunListener> safeListeners = new ArrayList<RunListener>();
-                List<Failure> failures = new ArrayList<Failure>();
-                for (Iterator<RunListener> all = fCurrentListeners.iterator(); all
-                        .hasNext(); ) {
-                    try {
-                        RunListener listener = all.next();
-                        notifyListener(listener);
-                        safeListeners.add(listener);
-                    } catch (Exception e) {
-                        failures.add(new Failure(Description.TEST_MECHANISM, e));
-                    }
+            int capacity = fCurrentListeners.size();
+            ArrayList<RunListener> safeListeners = new ArrayList<RunListener>(capacity);
+            ArrayList<Failure> failures = new ArrayList<Failure>(capacity);
+            for (RunListener listener : fCurrentListeners) {
+                try {
+                    notifyListener(listener);
+                    safeListeners.add(listener);
+                } catch (Exception e) {
+                    failures.add(new Failure(Description.TEST_MECHANISM, e));
                 }
-                fireTestFailures(safeListeners, failures);
             }
+            fireTestFailures(safeListeners, failures);
         }
 
         abstract protected void notifyListener(RunListener each) throws Exception;
@@ -80,8 +92,6 @@ public void fireTestRunStarted(final Description description) {
             protected void notifyListener(RunListener each) throws Exception {
                 each.testRunStarted(description);
             }
-
-            ;
         }.run();
     }
 
@@ -94,8 +104,6 @@ public void fireTestRunFinished(final Result result) {
             protected void notifyListener(RunListener each) throws Exception {
                 each.testRunFinished(result);
             }
-
-            ;
         }.run();
     }
 
@@ -114,8 +122,6 @@ public void fireTestStarted(final Description description) throws StoppedByUserE
             protected void notifyListener(RunListener each) throws Exception {
                 each.testStarted(description);
             }
-
-            ;
         }.run();
     }
 
@@ -133,14 +139,11 @@ private void fireTestFailures(List<RunListener> listeners,
         if (!failures.isEmpty()) {
             new SafeNotifier(listeners) {
                 @Override
-                protected void notifyListener(RunListener listener)
-                        throws Exception {
+                protected void notifyListener(RunListener listener) throws Exception {
                     for (Failure each : failures) {
                         listener.testFailure(each);
                     }
                 }
-
-                ;
             }.run();
         }
     }
@@ -158,8 +161,6 @@ public void fireTestAssumptionFailed(final Failure failure) {
             protected void notifyListener(RunListener each) throws Exception {
                 each.testAssumptionFailure(failure);
             }
-
-            ;
         }.run();
     }
 
@@ -190,8 +191,6 @@ public void fireTestFinished(final Description description) {
             protected void notifyListener(RunListener each) throws Exception {
                 each.testFinished(description);
             }
-
-            ;
         }.run();
     }
 
@@ -209,6 +208,9 @@ public void pleaseStop() {
      * Internal use only. The Result's listener must be first.
      */
     public void addFirstListener(RunListener listener) {
-        fListeners.add(0, listener);
+        if (listener == null) {
+            throw new NullPointerException("Cannot add a null listener");
+        }
+        fListeners.add(0, wrapIfNotThreadSafe(listener));
     }
-}
+}
diff --git a/src/main/java/org/junit/runner/notification/SynchronizedRunListener.java b/src/main/java/org/junit/runner/notification/SynchronizedRunListener.java
@@ -0,0 +1,103 @@
+package org.junit.runner.notification;
+
+import org.junit.runner.Description;
+import org.junit.runner.Result;
+
+/**
+ * Thread-safe decorator for {@link RunListener} implementations that synchronizes
+ * calls to the delegate.
+ *
+ * <p>This class synchronizes all listener calls on a RunNotifier instance. This is done because
+ * prior to JUnit 4.12, all listeners were called in a synchronized block in RunNotifier,
+ * so no two listeners were ever called concurrently. If we instead made the methods here
+ * sychronized, clients that added multiple listeners that called common code might see
+ * issues due to the reduced synchronization.
+ *
+ * @author Tibor Digana (tibor17)
+ * @author Kevin Cooney (kcooney)
+ * @since 4.12
+ *
+ * @see RunNotifier
+ */
+@RunListener.ThreadSafe
+final class SynchronizedRunListener extends RunListener {
+    private final RunListener fListener;
+    private final Object fMonitor;
+
+    SynchronizedRunListener(RunListener listener, Object monitor) {
+        fListener = listener;
+        fMonitor = monitor;
+    }
+
+    @Override
+    public void testRunStarted(Description description) throws Exception {
+        synchronized (fMonitor) {
+            fListener.testRunStarted(description);
+        }
+    }
+
+    @Override
+    public void testRunFinished(Result result) throws Exception {
+        synchronized (fMonitor) {
+            fListener.testRunFinished(result);
+        }
+    }
+
+    @Override
+    public void testStarted(Description description) throws Exception {
+        synchronized (fMonitor) {
+            fListener.testStarted(description);
+        }
+    }
+
+    @Override
+    public void testFinished(Description description) throws Exception {
+        synchronized (fMonitor) {
+            fListener.testFinished(description);
+        }
+    }
+
+    @Override
+    public void testFailure(Failure failure) throws Exception {
+        synchronized (fMonitor) {
+            fListener.testFailure(failure);
+        }
+    }
+
+    @Override
+    public void testAssumptionFailure(Failure failure) {
+        synchronized (fMonitor) {
+            fListener.testAssumptionFailure(failure);
+        }
+    }
+
+    @Override
+    public void testIgnored(Description description) throws Exception {
+        synchronized (fMonitor) {
+            fListener.testIgnored(description);
+        }
+    }
+
+    @Override
+    public int hashCode() {
+        return fListener.hashCode();
+    }
+
+    @Override
+    public boolean equals(Object other) {
+        if (this == other) {
+            return true;
+        }
+        if (!(other instanceof SynchronizedRunListener)) {
+            return false;
+        }
+        SynchronizedRunListener that = (SynchronizedRunListener) other;
+
+        return fListener.equals(that.fListener);
+    }
+
+    @Override
+    public String toString() {
+        return fListener.toString() + " (with synchronization wrapper)";
+    }
+}