Skip to content

Commit d7c07f3

Browse files
sbabcocsbabcoc
authored
Merge pull request #23 from Nordstrom/pr/fix-intercept-issue
Re-implement thread safety to avoid loading watchers repeatedly
2 parents 5c5749e + 3d32c8b commit d7c07f3

File tree

4 files changed

+105
-39
lines changed

4 files changed

+105
-39
lines changed

‎src/main/java/com/nordstrom/automation/junit/CreateTestClass.java‎

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,18 @@
2323
*/
2424
@SuppressWarnings("squid:S1118")
2525
public class CreateTestClass {
26+
private static final ServiceLoader<TestClassWatcher> classWatcherLoader;
27+
private static final ServiceLoader<TestClassWatcher2> classWatcher2Loader;
2628
private static final Logger LOGGER = LoggerFactory.getLogger(CreateTestClass.class);
2729
private static final Map<TestClass, Object> TESTCLASS_TO_RUNNER = new ConcurrentHashMap<>();
2830
private static final Map<Object, TestClass> METHOD_TO_TESTCLASS = new ConcurrentHashMap<>();
2931

30-
/**
32+
static {
33+
classWatcherLoader = ServiceLoader.load(TestClassWatcher.class);
34+
classWatcher2Loader = ServiceLoader.load(TestClassWatcher2.class);
35+
}
36+
37+
/**
3138
* Interceptor for the {@link org.junit.runners.ParentRunner#createTestClass createTestClass} method.
3239
*
3340
* @param runner underlying test runner
@@ -45,11 +52,15 @@ public static TestClass intercept(@This final Object runner, @SuperCall final Ca
4552
METHOD_TO_TESTCLASS.put(method, testClass);
4653
}
4754

48-
for (TestClassWatcher watcher : ServiceLoader.load(TestClassWatcher.class)) {
49-
watcher.testClassCreated(testClass, runner);
55+
synchronized(classWatcherLoader) {
56+
for (TestClassWatcher watcher : classWatcherLoader) {
57+
watcher.testClassCreated(testClass, runner);
58+
}
5059
}
51-
for (TestClassWatcher2 watcher : ServiceLoader.load(TestClassWatcher2.class)) {
52-
watcher.testClassCreated(testClass, runner);
60+
synchronized(classWatcher2Loader) {
61+
for (TestClassWatcher2 watcher : classWatcher2Loader) {
62+
watcher.testClassCreated(testClass, runner);
63+
}
5364
}
5465

5566
attachRunnerScheduler(testClass, runner);
@@ -86,11 +97,15 @@ private static RunnerScheduler createRunnerScheduler(final TestClass testClass,
8697

8798
public void schedule(Runnable childStatement) {
8899
if (scheduled.compareAndSet(false, true)) {
89-
for (TestClassWatcher watcher : ServiceLoader.load(TestClassWatcher.class)) {
90-
watcher.testClassStarted(testClass);
100+
synchronized(classWatcherLoader) {
101+
for (TestClassWatcher watcher : classWatcherLoader) {
102+
watcher.testClassStarted(testClass);
103+
}
91104
}
92-
for (TestClassWatcher2 watcher : ServiceLoader.load(TestClassWatcher2.class)) {
93-
watcher.testClassStarted(testClass, runner);
105+
synchronized(classWatcher2Loader) {
106+
for (TestClassWatcher2 watcher : classWatcher2Loader) {
107+
watcher.testClassStarted(testClass, runner);
108+
}
94109
}
95110
}
96111

@@ -106,11 +121,15 @@ public void schedule(Runnable childStatement) {
106121
}
107122

108123
public void finished() {
109-
for (TestClassWatcher watcher : ServiceLoader.load(TestClassWatcher.class)) {
110-
watcher.testClassFinished(testClass);
124+
synchronized(classWatcherLoader) {
125+
for (TestClassWatcher watcher : classWatcherLoader) {
126+
watcher.testClassFinished(testClass);
127+
}
111128
}
112-
for (TestClassWatcher2 watcher : ServiceLoader.load(TestClassWatcher2.class)) {
113-
watcher.testClassFinished(testClass, runner);
129+
synchronized(classWatcher2Loader) {
130+
for (TestClassWatcher2 watcher : classWatcher2Loader) {
131+
watcher.testClassFinished(testClass, runner);
132+
}
114133
}
115134
}
116135
};

‎src/main/java/com/nordstrom/automation/junit/LifecycleHooks.java‎

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -126,9 +126,16 @@ static synchronized JUnitConfig getConfig() {
126126
*/
127127
@SuppressWarnings("squid:S1118")
128128
public static class Run {
129+
private static final ServiceLoader<RunListener> runListenerLoader;
130+
private static final ServiceLoader<RunnerWatcher> runnerWatcherLoader;
129131
private static final Set<RunNotifier> NOTIFIERS = new CopyOnWriteArraySet<>();
130132
private static final Map<Object, Object> CHILD_TO_PARENT = new ConcurrentHashMap<>();
131133

134+
static {
135+
runListenerLoader = ServiceLoader.load(RunListener.class);
136+
runnerWatcherLoader = ServiceLoader.load(RunnerWatcher.class);
137+
}
138+
132139
/**
133140
* Interceptor for the {@link org.junit.runners.ParentRunner#run run} method.
134141
*
@@ -147,20 +154,26 @@ public static void intercept(@This final Object runner, @SuperCall final Callabl
147154

148155
if (NOTIFIERS.add(notifier)) {
149156
Description description = invoke(runner, "getDescription");
150-
for (RunListener listener : ServiceLoader.load(RunListener.class)) {
151-
notifier.addListener(listener);
152-
listener.testRunStarted(description);
157+
synchronized(runListenerLoader) {
158+
for (RunListener listener : runListenerLoader) {
159+
notifier.addListener(listener);
160+
listener.testRunStarted(description);
161+
}
153162
}
154163
}
155164

156-
for (RunnerWatcher watcher : ServiceLoader.load(RunnerWatcher.class)) {
157-
watcher.runStarted(runner);
165+
synchronized(runnerWatcherLoader) {
166+
for (RunnerWatcher watcher : runnerWatcherLoader) {
167+
watcher.runStarted(runner);
168+
}
158169
}
159170

160171
callProxy(proxy);
161172

162-
for (RunnerWatcher watcher : ServiceLoader.load(RunnerWatcher.class)) {
163-
watcher.runFinished(runner);
173+
synchronized(runnerWatcherLoader) {
174+
for (RunnerWatcher watcher : runnerWatcherLoader) {
175+
watcher.runFinished(runner);
176+
}
164177
}
165178
}
166179

@@ -182,8 +195,13 @@ static Object getParentOf(Object child) {
182195
@SuppressWarnings("squid:S1118")
183196
public static class CreateTest {
184197

198+
private static final ServiceLoader<TestObjectWatcher> objectWatcherLoader;
185199
private static final Map<Object, TestClass> TARGET_TO_TESTCLASS = new ConcurrentHashMap<>();
186200

201+
static {
202+
objectWatcherLoader = ServiceLoader.load(TestObjectWatcher.class);
203+
}
204+
187205
/**
188206
* Interceptor for the {@link org.junit.runners.BlockJUnit4ClassRunner#createTest createTest} method.
189207
*
@@ -199,8 +217,10 @@ public static Object intercept(@This final Object runner,
199217
TARGET_TO_TESTCLASS.put(testObj, getTestClassOf(runner));
200218
applyTimeout(testObj);
201219

202-
for (TestObjectWatcher watcher : ServiceLoader.load(TestObjectWatcher.class)) {
203-
watcher.testObjectCreated(testObj, TARGET_TO_TESTCLASS.get(testObj));
220+
synchronized(objectWatcherLoader) {
221+
for (TestObjectWatcher watcher : objectWatcherLoader) {
222+
watcher.testObjectCreated(testObj, TARGET_TO_TESTCLASS.get(testObj));
223+
}
204224
}
205225

206226
return testObj;

‎src/main/java/com/nordstrom/automation/junit/RetryHandler.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,13 @@
2121
*/
2222
public class RetryHandler {
2323

24+
private static final ServiceLoader<JUnitRetryAnalyzer> retryAnalyzerLoader;
2425
private static final Logger LOGGER = LoggerFactory.getLogger(RetryHandler.class);
2526

27+
static {
28+
retryAnalyzerLoader = ServiceLoader.load(JUnitRetryAnalyzer.class);
29+
}
30+
2631
private RetryHandler() {
2732
throw new AssertionError("RetryHandler is a static utility class that cannot be instantiated");
2833
}
@@ -124,9 +129,11 @@ static int getMaxRetry(Object runner, final FrameworkMethod method) {
124129
* @return {@code true} if test should be retried; otherwise {@code false}
125130
*/
126131
static boolean isRetriable(final FrameworkMethod method, final Throwable thrown) {
127-
for (JUnitRetryAnalyzer analyzer : ServiceLoader.load(JUnitRetryAnalyzer.class)) {
128-
if (analyzer.retry(method, thrown)) {
129-
return true;
132+
synchronized(retryAnalyzerLoader) {
133+
for (JUnitRetryAnalyzer analyzer : retryAnalyzerLoader) {
134+
if (analyzer.retry(method, thrown)) {
135+
return true;
136+
}
130137
}
131138
}
132139
return false;

‎src/main/java/com/nordstrom/automation/junit/RunReflectiveCall.java‎

Lines changed: 34 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,17 @@
2727
@SuppressWarnings("squid:S1118")
2828
public class RunReflectiveCall {
2929

30+
private static final ServiceLoader<MethodWatcher> methodWatcherLoader;
31+
private static final ServiceLoader<RunWatcher> runWatcherLoader;
32+
3033
private static final Map<FrameworkMethod, Object> METHOD_TO_TARGET = new ConcurrentHashMap<>();
3134
private static final Map<TestClass, AtomicTest> TESTCLASS_TO_ATOMICTEST = new ConcurrentHashMap<>();
3235

36+
static {
37+
methodWatcherLoader = ServiceLoader.load(MethodWatcher.class);
38+
runWatcherLoader = ServiceLoader.load(RunWatcher.class);
39+
}
40+
3341
/**
3442
* Interceptor for the {@link org.junit.internal.runners.model.ReflectiveCallable#runReflectiveCall
3543
* runReflectiveCall} method.
@@ -69,17 +77,21 @@ public static Object intercept(@This final Object callable, @SuperCall final Cal
6977

7078
Object result = null;
7179
Throwable thrown = null;
72-
for (MethodWatcher watcher : ServiceLoader.load(MethodWatcher.class)) {
73-
watcher.beforeInvocation(target, method, params);
80+
synchronized(methodWatcherLoader) {
81+
for (MethodWatcher watcher : methodWatcherLoader) {
82+
watcher.beforeInvocation(target, method, params);
83+
}
7484
}
7585

7686
try {
7787
result = LifecycleHooks.callProxy(proxy);
7888
} catch (Throwable t) {
7989
thrown = t;
8090
} finally {
81-
for (MethodWatcher watcher : ServiceLoader.load(MethodWatcher.class)) {
82-
watcher.afterInvocation(target, method, thrown);
91+
synchronized(methodWatcherLoader) {
92+
for (MethodWatcher watcher : methodWatcherLoader) {
93+
watcher.afterInvocation(target, method, thrown);
94+
}
8395
}
8496
}
8597

@@ -100,8 +112,10 @@ public static Object intercept(@This final Object callable, @SuperCall final Cal
100112
static void fireTestStarted(TestClass testClass, Runnable runnable) {
101113
AtomicTest atomicTest = createAtomicTest(testClass, runnable);
102114
if (atomicTest != null) {
103-
for (RunWatcher watcher : ServiceLoader.load(RunWatcher.class)) {
104-
watcher.testStarted(atomicTest.getIdentity(), atomicTest.getTestClass());
115+
synchronized(runWatcherLoader) {
116+
for (RunWatcher watcher : runWatcherLoader) {
117+
watcher.testStarted(atomicTest.getIdentity(), atomicTest.getTestClass());
118+
}
105119
}
106120
}
107121
}
@@ -114,9 +128,11 @@ static void fireTestStarted(TestClass testClass, Runnable runnable) {
114128
static void fireTestFinished(TestClass testClass) {
115129
AtomicTest atomicTest = TESTCLASS_TO_ATOMICTEST.get(testClass);
116130
if (atomicTest != null) {
117-
for (RunWatcher watcher : ServiceLoader.load(RunWatcher.class)) {
118-
notifyIfTestFailed(watcher, atomicTest);
119-
watcher.testFinished(atomicTest.getIdentity(), atomicTest.getTestClass());
131+
synchronized(runWatcherLoader) {
132+
for (RunWatcher watcher : runWatcherLoader) {
133+
notifyIfTestFailed(watcher, atomicTest);
134+
watcher.testFinished(atomicTest.getIdentity(), atomicTest.getTestClass());
135+
}
120136
}
121137
}
122138
}
@@ -147,8 +163,10 @@ private static void notifyIfTestFailed(RunWatcher watcher, AtomicTest atomicTest
147163
*/
148164
static void fireTestIgnored(Object runner, FrameworkMethod method) {
149165
TestClass testClass = getTestClassOf(runner);
150-
for (RunWatcher watcher : ServiceLoader.load(RunWatcher.class)) {
151-
watcher.testIgnored(method, testClass);
166+
synchronized(runWatcherLoader) {
167+
for (RunWatcher watcher : runWatcherLoader) {
168+
watcher.testIgnored(method, testClass);
169+
}
152170
}
153171
}
154172

@@ -175,9 +193,11 @@ public static Object getTargetFor(FrameworkMethod method) {
175193
public static Optional<MethodWatcher> getAttachedWatcher(
176194
Class<? extends MethodWatcher> watcherType) {
177195
Objects.requireNonNull(watcherType, "[watcherType] must be non-null");
178-
for (MethodWatcher watcher : ServiceLoader.load(MethodWatcher.class)) {
179-
if (watcher.getClass() == watcherType) {
180-
return Optional.of(watcher);
196+
synchronized(methodWatcherLoader) {
197+
for (MethodWatcher watcher : methodWatcherLoader) {
198+
if (watcher.getClass() == watcherType) {
199+
return Optional.of(watcher);
200+
}
181201
}
182202
}
183203
return Optional.empty();

0 commit comments

Comments
 (0)