Don't remove listeners, which throw exceptions (fixes #209 and #395)

src/main/java/org/junit/runner/notification/RunNotifier.java

@@ -1,5 +1,7 @@
 package org.junit.runner.notification;
 
+import static java.util.Arrays.asList;
+
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Iterator;
@@ -34,15 +36,30 @@ public void removeListener(RunListener listener) {
     }
 
 	private abstract class SafeNotifier {
+		private final List<RunListener> fCurrentListeners;
+
+		SafeNotifier() {
+			this(fListeners);
+		}
+
+		SafeNotifier(List<RunListener> currentListeners) {
+			fCurrentListeners= currentListeners;
+		}
+
 		void run() {
 			synchronized (fListeners) {
-				for (Iterator<RunListener> all= fListeners.iterator(); all.hasNext();)
+				List<RunListener> safeListeners= new ArrayList<RunListener>();
+				List<Failure> failures= new ArrayList<Failure>();
+				for (Iterator<RunListener> all= fCurrentListeners.iterator(); all
+						.hasNext();)
 					try {
-						notifyListener(all.next());
+						RunListener listener= all.next();
+						notifyListener(listener);
+						safeListeners.add(listener);
 					} catch (Exception e) {
-						all.remove(); // Remove the offending listener first to avoid an infinite loop
-						fireTestFailure(new Failure(Description.TEST_MECHANISM, e));
+						failures.add(new Failure(Description.TEST_MECHANISM, e));
 					}
+				fireTestFailures(safeListeners, failures);
 			}
 		}
 
@@ -93,13 +110,21 @@ protected void notifyListener(RunListener each) throws Exception {
 	 * Invoke to tell listeners that an atomic test failed.
 	 * @param failure the description of the test that failed and the exception thrown
 	 */
-	public void fireTestFailure(final Failure failure) {
-		new SafeNotifier() {
-			@Override
-			protected void notifyListener(RunListener each) throws Exception {
-				each.testFailure(failure);
-			};
-		}.run();
+	public void fireTestFailure(Failure failure) {
+		fireTestFailures(fListeners, asList(failure));
+	}
+
+	private void fireTestFailures(List<RunListener> listeners,
+			final List<Failure> failures) {
+		if (!failures.isEmpty())
+			new SafeNotifier(listeners) {
+				@Override
+				protected void notifyListener(RunListener listener)
+						throws Exception {
+					for (Failure each : failures)
+						listener.testFailure(each);
+				};
+			}.run();
 	}
 
 	/**

src/test/java/org/junit/runner/notification/RunNotifierTest.java

@@ -0,0 +1,49 @@
+package org.junit.runner.notification;
+
+import static org.junit.Assert.assertNotNull;
+import org.junit.Test;
+import org.junit.runner.Result;
+
+public class RunNotifierTest {
+
+	@Test
+	public void notifiesSecondListenerIfFirstThrowsException() {
+		FailureListener failureListener= new FailureListener();
+		RunNotifier notifier= new RunNotifier();
+		notifier.addListener(new CorruptListener());
+		notifier.addListener(failureListener);
+		notifier.fireTestFailure(new Failure(null, null));
+		assertNotNull("The FailureListener registered no failure.",
+				failureListener.failure);
+	}
+
+	@Test
+	public void hasNoProblemsWithFailingListeners() { // see issues 209 and 395
+		RunNotifier notifier= new RunNotifier();
+		notifier.addListener(new CorruptListener());
+		notifier.addListener(new FailureListener());
+		notifier.addListener(new CorruptListener());
+		notifier.fireTestRunFinished(new Result());
+	}
+
+	private static class CorruptListener extends RunListener {
+		@Override
+		public void testRunFinished(Result result) throws Exception {
+			throw new RuntimeException();
+		}
+
+		@Override
+		public void testFailure(Failure failure) throws Exception {
+			throw new RuntimeException();
+		}
+	}
+
+	private static class FailureListener extends RunListener {
+		private Failure failure;
+
+		@Override
+		public void testFailure(Failure failure) throws Exception {
+			this.failure= failure;
+		}
+	}
+}

src/test/java/org/junit/tests/listening/TestListenerTest.java

@@ -38,7 +38,8 @@ public void testRunStarted(Description description) throws Exception {
 		}
 	}
 
-	@Test public void removeFailingListeners() {
+	@Test
+	public void reportsFailureOfListener() {
 		JUnitCore core= new JUnitCore();
 		core.addListener(new ExceptionListener());
 
@@ -48,10 +49,6 @@ public void testRunStarted(Description description) throws Exception {
 		assertEquals(1, result.getFailureCount());
 		Failure testFailure= result.getFailures().get(0);
 		assertEquals(Description.TEST_MECHANISM, testFailure.getDescription());
-
-		count= 0;
-		core.run(OneTest.class);
-		assertEquals(0, count); // Doesn't change because listener was removed		
 	}
 
 	@Test public void freshResultEachTime() {