Make RetryExtension more parallel-safe (#1701)

Fixes #1700

spock-core/src/main/java/org/spockframework/runtime/extension/builtin/RetryIterationInterceptor.java

@@ -16,22 +16,26 @@
 
 package org.spockframework.runtime.extension.builtin;
 
-import org.spockframework.runtime.extension.*;
+import groovy.lang.Closure;
+import org.opentest4j.MultipleFailuresError;
+import org.spockframework.runtime.extension.IMethodInterceptor;
+import org.spockframework.runtime.extension.IMethodInvocation;
+import org.spockframework.runtime.extension.IStore.Namespace;
 import org.spockframework.runtime.model.MethodInfo;
+import org.spockframework.util.ThreadSafe;
 import spock.lang.Retry;
 
-import java.util.*;
-
-import groovy.lang.Closure;
-import org.opentest4j.MultipleFailuresError;
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * @author Leonard Brünings
  * @since 1.2
  */
 public class RetryIterationInterceptor extends RetryBaseInterceptor implements IMethodInterceptor {
-
-  private final IterationState iterationState = new IterationState();
+  private static final Namespace NAMESPACE = Namespace.create(RetryIterationInterceptor.class);
+  private static final String ITERATION_STATE = "iterationState";
 
   public RetryIterationInterceptor(Retry retry, MethodInfo featureMethod) {
     super(retry);
@@ -40,7 +44,8 @@ public RetryIterationInterceptor(Retry retry, MethodInfo featureMethod) {
 
   @Override
   public void intercept(IMethodInvocation invocation) throws Throwable {
-    iterationState.startIteration();
+    IterationState iterationState = new IterationState();
+    invocation.getStore(NAMESPACE).put(ITERATION_STATE, iterationState);
     for (int i = 0; i <= retry.count(); i++) {
       iterationState.setRetryAttempt(i);
       invocation.proceed();
@@ -60,9 +65,10 @@ public InnerRetryInterceptor(Retry retry, Closure condition) {
 
     @Override
     public void intercept(IMethodInvocation invocation) throws Throwable {
+      IterationState iterationState = invocation.getStore(NAMESPACE).get(ITERATION_STATE);
       try {
         invocation.proceed();
-        iterationState.startIteration();
+        iterationState.resetFailures();
       } catch (Throwable e) {
         if (isExpected(invocation, e)) {
           iterationState.failIteration(e);
@@ -73,27 +79,28 @@ public void intercept(IMethodInvocation invocation) throws Throwable {
     }
   }
 
+  @ThreadSafe
   private class IterationState {
-    private final ThreadLocal<Boolean> finalIteration = ThreadLocal.withInitial(() -> false);
-    private final ThreadLocal<List<Throwable>> throwables = new ThreadLocal<>();
+    private final AtomicBoolean finalIteration = new AtomicBoolean(false);
+    private final List<Throwable> throwables = new CopyOnWriteArrayList<>();
 
-    void startIteration() {
-      throwables.set(new ArrayList<>());
+    void resetFailures() {
+      throwables.clear();
     }
 
     void setRetryAttempt(int retryAttempt) {
       finalIteration.set(retryAttempt == retry.count());
     }
 
     void failIteration(Throwable failure) {
-      throwables.get().add(failure);
+      throwables.add(failure);
       if (finalIteration.get()) {
-        throw new MultipleFailuresError("Retries exhausted", throwables.get());
+        throw new MultipleFailuresError("Retries exhausted", throwables);
       }
     }
 
     boolean notFailed() {
-      return throwables.get().isEmpty();
+      return throwables.isEmpty();
     }
   }
 }

spock-specs/src/test/groovy/org/spockframework/smoke/extension/RetryFeatureExtensionSpec.groovy

@@ -2,8 +2,14 @@ package org.spockframework.smoke.extension
 
 import org.spockframework.EmbeddedSpecification
 import org.spockframework.runtime.ConditionNotSatisfiedError
+import org.spockframework.runtime.extension.ExtensionAnnotation
+import org.spockframework.runtime.extension.IAnnotationDrivenExtension
+import org.spockframework.runtime.extension.builtin.RetryIterationInterceptor
+import org.spockframework.runtime.model.FeatureInfo
 import spock.lang.*
 
+import java.lang.annotation.Retention
+import java.lang.annotation.RetentionPolicy
 import java.util.concurrent.atomic.AtomicInteger
 
 import org.opentest4j.MultipleFailuresError
@@ -130,6 +136,47 @@ class Foo extends Specification {
     Retry.Mode.SETUP_FEATURE_CLEANUP.name() || 4             || true
   }
 
+  def "@Retry mode SETUP_FEATURE_CLEANUP is safe against offloading iteration execution to a different thread"() {
+    given:
+    runner.throwFailure = true
+    runner.addClassImport(ChangeThread)
+
+    when:
+    runner.runSpecBody("""
+      @Retry(mode = Retry.Mode.SETUP_FEATURE_CLEANUP)
+      @ChangeThread
+      def foo() {
+        expect: false
+      }
+    """)
+
+    then:
+    MultipleFailuresError e = thrown()
+    e.failures.size() == 4
+    e.failures.collect { it.getClass() } =~ [ConditionNotSatisfiedError]
+  }
+
+  def "@Retry mode SETUP_FEATURE_CLEANUP is safe against offloading iteration execution to a different thread in data driven feature"() {
+    given:
+    runner.addClassImport(ChangeThread)
+
+    when:
+    def result = runner.runSpecBody("""
+      @Retry(mode = Retry.Mode.SETUP_FEATURE_CLEANUP)
+      @ChangeThread
+      def foo() {
+        expect: false
+        where: i << (1..2)
+      }
+    """)
+
+    then:
+    result.failures.size() == 2
+    result.failures.collect { it.exception.getClass() } =~ [MultipleFailuresError]
+    result.failures.collect { it.exception.failures.size() } =~ [4]
+    result.failures.collectMany { it.exception.failures*.getClass() } =~ [ConditionNotSatisfiedError]
+  }
+
   def "@Retry mode #mode executes setup and cleanup #expectedCount times for @Unroll'ed feature (parallel: #parallel)"(String mode, int expectedCount) {
     given:
     withParallelExecution(parallel)
@@ -735,4 +782,21 @@ class Foo extends Specification {
     }
   }
 
+  static class ChangeThreadExtension implements IAnnotationDrivenExtension<ChangeThread> {
+    @Override
+    void visitFeatureAnnotation(ChangeThread annotation, FeatureInfo feature) {
+      assert feature.iterationInterceptors.any { it instanceof RetryIterationInterceptor }: "RetryIterationInterceptor must be added first"
+      feature.addIterationInterceptor { invocation ->
+        new Thread({ invocation.proceed() }).tap {
+          it.start()
+          it.join()
+        }
+      }
+    }
+  }
+}
+
+@Retention(RetentionPolicy.RUNTIME)
+@ExtensionAnnotation(RetryFeatureExtensionSpec.ChangeThreadExtension)
+@interface ChangeThread {
 }