Junit5: 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.

[sbrannen]

@mdolinin & @marcphilipp, thank you for the great detective work! 👍

I've confirmed that this is in fact a bug in Spring. I already have the fix in place locally and will be pushing to master soon (after first opening a JIRA ticket 😉). So that should be available in the upcoming Spring Framework 5.1.3 release.

[sbrannen]

Yes. But this is fine from Spring perspective.

Actually, the identity check (==) in Spring is the core underlying issue due to the implementation of java.lang.reflect.Executable#getParameters() in the JDK.

Of course, one could potentially argue that the implementation in the JDK is _buggy_ since it is not actually thread-safe, but we cannot change the JDK. 😉

The actual reason is because JUnit provide broken param object to Spring

I disagree: JUnit provides a perfectly valid Parameter object to Spring, and Spring uses that Parameter object according to the published APIs of the JDK. The reason that Spring uses identity checks instead of equality checks is for performance. Note, however, that the Javadoc for java.lang.reflect.Executable#getParameters() unfortunately does not state that it is not thread-safe.

So the best solution here is to improve the implementation in Spring.

[sbrannen]

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 agree and have taken note of that for the improvements for Spring.

I think this would be quite easy to fix in Spring. Instead of calling SynthesizingMethodParameter.forParameter(parameter), using SynthesizingMethodParameter.forParameter(executable, parameterIndex)

Almost!

That last one has to be forExecutable() and then it works. 😉

[sbrannen]

In case you're interested, here's the first JIRA issue for Spring, specifically for the SpringExtension.

https://jira.spring.io/browse/SPR-17533

[marcphilipp]

Thanks, @sbrannen!

That last one has to be forExecutable() and then it works.

That's what happens when you try to write code in a GitHub issue comment. 😉

[mdolinin]

Thanks @marcphilipp and @sbrannen for this super fast response. Great work.

[sbrannen]

And here's the second JIRA issue for Spring Core.

https://jira.spring.io/browse/SPR-17534

[sbrannen]

Already fixed within the SpringExtension (in Spring Framework 5.1.3):

https://github.com/spring-projects/spring-framework/commit/aa7f69a5d1790b2a2bc9e82e70ead060763c4ec9

[sbrannen]

Fixed in the SpringExtension in Spring Framework 5.0.11 as well: https://github.com/spring-projects/spring-framework/commit/cd67b285e1038323fd4e8434ec48e40190d63236

[sbrannen]

And now fixed in Spring Core for Spring Framework 5.0.11 and 5.1.3.

• https://github.com/spring-projects/spring-framework/commit/f0e69e06b7c0b4b1c8cbf7c17cfc21ab92ce983c
• https://github.com/spring-projects/spring-framework/commit/81fde5ec4103e3db28bf79073691938a4743b121

So... all good!

[sbrannen]

I am closing this issue since it is not a bug in JUnit Jupiter.

[sbrannen]

Thanks @marcphilipp and @sbrannen for this super fast response. Great work.

You're very welcome.

Thanks again for bringing this to our attention and for all of the very helpful detective work!