From 60d6bfb6a9d04ef101f8bf59555ae27f4dfdbd24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Kautler?= Date: Mon, 4 Mar 2024 22:15:24 +0100 Subject: [PATCH] Make RetryExtension more parallel-safe (#1701) Fixes #1700 --- .../builtin/RetryIterationInterceptor.java | 39 ++++++----- .../RetryFeatureExtensionSpec.groovy | 64 +++++++++++++++++++ 2 files changed, 87 insertions(+), 16 deletions(-) diff --git a/spock-core/src/main/java/org/spockframework/runtime/extension/builtin/RetryIterationInterceptor.java b/spock-core/src/main/java/org/spockframework/runtime/extension/builtin/RetryIterationInterceptor.java index 6850380a86..7feaa27b63 100644 --- a/spock-core/src/main/java/org/spockframework/runtime/extension/builtin/RetryIterationInterceptor.java +++ b/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,12 +79,13 @@ public void intercept(IMethodInvocation invocation) throws Throwable { } } + @ThreadSafe private class IterationState { - private final ThreadLocal finalIteration = ThreadLocal.withInitial(() -> false); - private final ThreadLocal> throwables = new ThreadLocal<>(); + private final AtomicBoolean finalIteration = new AtomicBoolean(false); + private final List throwables = new CopyOnWriteArrayList<>(); - void startIteration() { - throwables.set(new ArrayList<>()); + void resetFailures() { + throwables.clear(); } void setRetryAttempt(int retryAttempt) { @@ -86,14 +93,14 @@ void setRetryAttempt(int retryAttempt) { } 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(); } } } diff --git a/spock-specs/src/test/groovy/org/spockframework/smoke/extension/RetryFeatureExtensionSpec.groovy b/spock-specs/src/test/groovy/org/spockframework/smoke/extension/RetryFeatureExtensionSpec.groovy index 2628dbdf00..b3c446e24b 100644 --- a/spock-specs/src/test/groovy/org/spockframework/smoke/extension/RetryFeatureExtensionSpec.groovy +++ b/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 { + @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 { }