Using cucumber-spring does not share Spring context when using parallel (methods) surefire settings

[dom54]

Summary

I have 3 feature files, with a single StepDefinition. When running in single threaded mode, everything is perfect. When running with the maven surefire plugin, using parallel as methods, I get a spring context created per thread, which I cannot have (startup time of the app would mean no benefit to threading)

@WebAppConfiguration
@ContextHierarchy({
        @ContextConfiguration(classes = { TestConfiguration.class }, loader = AnnotationConfigWebContextLoader.class),
        @ContextConfiguration(classes = { ServletConfiguration.class }, loader = AnnotationConfigWebContextLoader.class)
})
public class DefaultSteps {

 <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-surefire-plugin</artifactId>
                <version>2.19.1</version>
                <configuration>
                    <parallel>methods</parallel>
                    <threadCount>8</threadCount>

I am working on a test case to share

Expected Behavior

The problem is the code uses a "TestContext" which binds a context to ThreadLocal.

So I worked around this by putting my own bootstrapper

@BootstrapWith(value = MultiThreadedAwareSpringBootstrapper.class)
public class DefaultSteps {

public class MultiThreadedAwareSpringBootstrapper extends org.springframework.test.context.web.WebTestContextBootstrapper {

    @Override
    public TestContext buildTestContext() {
        return new DefaultTestContext(getBootstrapContext().getTestClass(), buildMergedContextConfiguration(),
                new DefaultCacheAwareContextLoaderDelegate()); // dont pass in a context ,which will then use a static default cache of contexts, i.e. share between threads
    }

}

Current Behavior

This works, except once in a while it blows up (SpringFactory seems not to be thread safe)

org.springframework.beans.factory.NoUniqueBeanDefinitionException: No qualifying bean of type 'com.refacted.redacted.acceptance.glue.DefaultSteps' available: expected single matching bean but found 1: com.redacted.redacted.acceptance.glue.DefaultSteps

Possible Solution

Make SpringFactory threadsafe...still investigating.

In particular this line is not thread safe (if the context is shared)

context.getBeanFactory().registerScope(SCOPE_CUCUMBER_GLUE, new GlueCodeScope());

Steps to Reproduce (for bugs)

Working on it...

[mpkorstanje]

You are unfortunately not the first to have noticed this. If you want to help out you'd be most welcome but unfortunately it won't be trivial.

Have a look at #1583 and #1582 and #1470 for a possible solution.

I'll close this one.

[dom54]

Im not sure I understand why its not trivial. Adding a synchronized block seems to have fixed it for me.... (im using 4.8.0)

[mpkorstanje]

Unfortunately cucumber-spring puts step definitions instances into the application context as beans. Hence the occasional NoUniqueBeanDefinitionException when two scenarios execute concurrently.

The step definition instances contain the current state of a scenario. So swapping these out concurrently may result in leaking state between scenarios.

[dom54]

But looking at the code it is intending to put them in as "cucumber-scope" beans, which are ThreadLocal scoped, which it does do. So the instance of the bean is only ever in GlueCodeScope.

The method returns a bean and stores the bean in a thereadlocal context.

 @Override
    public Object get(String name, ObjectFactory<?> objectFactory) {
        GlueCodeContext context = GlueCodeContext.getInstance();
        Object obj = context.get(name);
        if (obj == null) {
            obj = objectFactory.getObject();
            context.put(name, obj);
        }

        return obj;
    }

[dom54]

(Im just trying to get to the bottom of why we think this is non trivial as the code looks to be doing everything required! Except for one or two lines of non-thread safe stuff in SpringFactory)

[mpkorstanje]

    @Override
    public <T> T getInstance(final Class<T> type) {
        try {
            return beanFactory.getBean(type);
        } catch (BeansException e) {
            throw new CucumberBackendException(e.getMessage(), e);
        }
    }

So you're saying that getInstance should pull the beans from the current glue scope?

[mpkorstanje]

Please feel free to send a PR!

[mpkorstanje]

I've been working with the assumption that there was only one cucumber-glue scope active at once. If that assumption is not correct then you are onto a much better fix then what I had in mind.

[dom54]

Yes, getInstance does pull out of current glue scope and current glue scope (of which there is one defined but delegates to a ThreadLocal) .

There is definitely a race condition but not sure where...ill carry on trying to diagnose, but I feel like it will be a small fix

[dom54]

Yes so I found the issue. I would say its more of a spring bug.

If two threads call (in DefaultListableBeanFactory)
public void registerBeanDefinition(String beanName, BeanDefinition beanDefinition)
There is a check to see if the definition already exists. If it doesnt exist, it replaces in a ConcurrentHashMap the definition (in a synchronized block as well) but it adds the bean name to a list without checking if it already exists

	synchronized (this.beanDefinitionMap) { //synchronized, but doesnt check if another thread already added it as the "exists" check is NOT synchronized 
					this.beanDefinitionMap.put(beanName, beanDefinition);
					List<String> updatedDefinitions = new ArrayList<>(this.beanDefinitionNames.size() + 1);
					**updatedDefinitions.addAll(this.beanDefinitionNames);**
					updatedDefinitions.add(beanName);
					this.beanDefinitionNames = updatedDefinitions;

So

[mpkorstanje]

Shouldn't be too much hardship to synchronize registerStepClassBeanDefinition and delete FixBootstrapUtils.

[mpkorstanje]

Ah. No that won't work. It's the underlying application context that is shared.

[dom54]

Away over Xmas so raised a quick PR which I believe will fix the issue0. Please note it doesnt change the fact that you still get a context per thread by default (but this is easy to override see below)

public class MultiThreadedAwareSpringBootstrapper extends org.springframework.test.context.web.WebTestContextBootstrapper {

    @Override
    public TestContext buildTestContext() {
        return new DefaultTestContext(getBootstrapContext().getTestClass(), buildMergedContextConfiguration(),
                new DefaultCacheAwareContextLoaderDelegate());
    }

[mpkorstanje]

If I am not mistaken that is caused by: https://github.com/cucumber/cucumber-jvm/blob/master/spring/src/main/java/io/cucumber/spring/FixBootstrapUtils.java#L47

Which is a class I'll enjoy deleting.

Anyway. Enjoy the holidays!

[dom54]

Yes you are right, but it might be desirable to have a context per feature , I am not sure really.. need to think about it.

Per thread is not very useful, per scenario might be useful... but if we are happy to get rid of "context per thread" as FixBootStrapUtils is doing, then yes we dont need that class anymore

[mpkorstanje]

There is no concept of features when executing cucumber. We only deal with pickles (compiled scenarios, and examples). Each of these is assumed to be independent from the others.

Ofc. with the spring context that doesn't really work out, but at this point it is up to the test author to ensure everything can be executed in parallel (much like JUnit).

[mpkorstanje]

And it is s a good thing we're working on major release now. So we can break everything - it just means writing more release notes. 😆

[dom54]

ok - updated the PR to default the behaviour using a single context (rather than one per thread)

[dom54]

#1848

I think its a bit tricky to have a deterministic test, what do you think?