Concurrency issue with resolving parameters in test methods using the SpringExtension

[mdolinin]

Concurrency issues with resolving parameters in test methods

Steps to reproduce

• Gradle java project with Junit5 tests that use SpringExtension
• Have at least one method @Test/@BeforeEach with parameter annotated with spring @Autowired
• Run all test with enabled default parallel strategy junit.jupiter.execution.parallel.enabled= true
• And with default "per-method" test instance lifecycle

Expected behavior

• Test passed

Actual behavior

• Some times get concurrency issues for example

Caused by: java.lang.IllegalArgumentException: 
Given parameter [com.example.project.Calculator arg0] 
does not match any parameter in the declaring executable

Example project

• That reproduce issue https://github.com/mdolinin/gradle-junit5-spring-example

Context

• Used versions : Jupiter 5.3.1 /Platform 1.3.1/ Spring Test 5.1.2
• Build Tool/IDE: gradle 4.10.2

Posible rootcause

• After some investigation found that this exceptions is thrown by Spring from org.springframework.core.MethodParameter

protected static int findParameterIndex(Parameter parameter) {
	Executable executable = parameter.getDeclaringExecutable();
	Parameter[] allParams = executable.getParameters();
	for (int i = 0; i < allParams.length; i++) {
		if (parameter == allParams[i]) {
			return i;
		}
	}
	throw new IllegalArgumentException("Given parameter [" + parameter +
			"] does not match any parameter in the declaring executable");
}

• This could happen because how JUnit create this parameter in
  https://github.com/junit-team/junit5/blob/5caf217676e85a51aabec736fce7cadf687a299d/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/execution/ExecutableInvoker.java#L160
• If run this in parallel two or more thread will get here with same link to one executable method
• And because inside Executable class parameters are stored in private transient volatile Parameter[] parameters;

private Parameter[] privateGetParameters() {
        // Use tmp to avoid multiple writes to a volatile.
        Parameter[] tmp = parameters;

        if (tmp == null) {

            // Otherwise, go to the JVM to get them
            try {
                tmp = getParameters0();
            } catch(IllegalArgumentException e) {
                // Rethrow ClassFormatErrors
                throw new MalformedParametersException("Invalid constant pool index");
            }

            // If we get back nothing, then synthesize parameters
            if (tmp == null) {
                hasRealParameterData = false;
                tmp = synthesizeAllParams();
            } else {
                hasRealParameterData = true;
                verifyParameters(tmp);
            }

            parameters = tmp;
        }

        return tmp;
    } 

• First thread could get parameters array from second one

Posible solutions

• Synchronize calls to executable
• Or provide clone of executable to each thread

[marcphilipp]

If I understood you correctly, the issue is the == comparison in Spring's findParameterIndex?

[mdolinin]

Yes. But this is fine from Spring perspective.
The actual reason is because JUnit provide broken param object to Spring

[mdolinin]

In my fork of JUnit5 project is just synchronize calls to executable in ExecutableInvoker

private Object[] resolveParameters(Executable executable, Optional<Object> target, Object outerInstance,
			ExtensionContext extensionContext, ExtensionRegistry extensionRegistry) {

		Preconditions.notNull(target, "target must not be null");

		Parameter[] parameters;
		synchronized (this) {
			parameters = executable.getParameters();
		}
		Object[] values = new Object[parameters.length];

		int start = 0;

And this solves issue. But I am sure that this is not the right way to fix this.

[marcphilipp]

Why does that fix the issue?

[marcphilipp]

Given the code in Executable, I think it's safe to say that it isn't safe to compare Parameter instances with ==. They have a pretty simple equals() implementation.

I think this would be quite easy to fix in Spring. Instead of calling SynthesizingMethodParameter.forParameter(parameter), using SynthesizingMethodParameter.forParameter(executable, parameterIndex) in https://github.com/spring-projects/spring-framework/blob/d553ddc5b3a657adebad04d9f3c7d466fbdd7b05/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/ParameterAutowireUtils.java#L121 should do it.

@sbrannen WDYT?

[marcphilipp]

Why does that fix the issue?

Nevermind, I got it. 🙂

[mdolinin]

I am not sure, but my guess is because if we have two concurrently running test methods.
Both of them get same Executable object (Method object).
When first one will call getParameters() it will receive value of Parameter[] tmp array and assign it to volatile Parameter[] parameters field inside Executable.
If at the same time second thread do the same it also receive value of Parameter[] tmp and reassign
volatile Parameter[] parameters field inside Executable to new values.
And as result of this when first will pass this parameter to Spring to resolve it will try to compare it with values from volatile Parameter[] parameters field that was reassigned by second thread.