KentBeck/junit — Issue #395: Potential ConcurrentModificationException in RunNotifier.SafeNotifier
Author: Stephan202
Created: 2012-03-06 14:52:55+00:00
State: open

The class `org.junit.runner.notification.RunNotifier` contains the following code:

private final List fListeners=
Collections.synchronizedList(new ArrayList());
...
private abstract class SafeNotifier {
void run() {
synchronized (fListeners) {
for (Iterator all= fListeners.iterator(); all.hasNext();)
try {
notifyListener(all.next());
} catch (Exception e) {
all.remove(); // Remove the offending listener first to avoid an infinite loop
fireTestFailure(new Failure(Description.TEST_MECHANISM, e));
}
}
}

abstract protected void notifyListener(RunListener each) throws Exception;
}
...
public void fireTestFailure(final Failure failure) {
new SafeNotifier() {
@Override
protected void notifyListener(RunListener each) throws Exception {
each.testFailure(failure);
};
}.run();
}

The synchronization on `fListeners` is insufficient to avoid a `ConcurrentModificationException` because of the inherent recursion of the construct. The following unit test demonstrates the issue:

final RunListener decentListener = new RunListener();
final RunListener faultyListener = new RunListener() {
public void testFailure(Failure failure) {
throw new RuntimeException();
}
};

final RunNotifier notifier = new RunNotifier();
notifier.addListener(faultyListener);
notifier.addListener(decentListener);
notifier.addListener(faultyListener);
notifier.fireTestFailure(null);

(Note that this will throw an `IllegalStateException`, because the `ConcurrentModificationException` is caught and followed by yet another call to `all.remove()`.)

N.B.: I did see [Issue 209](https://github.com/KentBeck/junit/issues/209), but filed a separate ticket because this problem seems to be sufficiently distinct.