[Spring] Share application context #1848
Conversation
|
making this the default - share context between all threads |
| @@ -26,8 +26,6 @@ | |||
| import java.util.Set; | |||
|
|
|||
| import static io.cucumber.spring.CucumberTestContext.SCOPE_CUCUMBER_GLUE; | |||
| import static io.cucumber.spring.FixBootstrapUtils.createBootstrapContext; | |||
| import static io.cucumber.spring.FixBootstrapUtils.resolveTestContextBootstrapper; | |||
There was a problem hiding this comment.
Removed the FixBootstrapUtils. Wasn't needed any more.
| } else { | ||
| if (beanFactory == null) { | ||
| beanFactory = createFallbackContext(); | ||
| synchronized (monitor) { |
There was a problem hiding this comment.
Replaced the SpringFactory.class with monitor. The class is accessible by any one. And while I don't expect any one to use it to synchronize on it, there also no need to do so.
There was a problem hiding this comment.
Also explained the why of it all.
|
|
||
| @Test | ||
| void test() { | ||
| @RepeatedTest(100) |
There was a problem hiding this comment.
We're running 100 repetitions with 100 concurrent scenarios. Should tone this down a bit for CI but good enough.
There was a problem hiding this comment.
[INFO] Cucumber-JVM: Spring ............................... SUCCESS [02:12 min]
🤣
mpkorstanje
changed the title
mpkorstanje
changed the title
|
To keep the history readable PRs are squashed when merging. I've drafted a commit message in your original description of the PR. You should be able to edit it. |
|
looks good to me |
mpkorstanje
changed the title
|
Hi @dom54, Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾 In return for this generous offer we hope you will:
On behalf of the Cucumber core team, |
|
Aright then we're done! Cheers! |
When executing Cucumber in parallel in combination with
cucumber-springusers would often encounter a cryptic error message:No qualifying bean of type 'Steps' available: expected single matching bean but found 1(#1823).This was initially fixed by ensuring that the Spring application context was not shared between threads (#1153, #1148). This however has the downside that the application context is not shared with JUnit tests either (#1583) and comes at significant performance penalty (#1846).
The root cause however was a race condition in
DefaultListableBeanFactory.registerBeanDefinition(String beanName, BeanDefinition beanDefinition). There is a check to see if the definition already exists. If it does not exist, it replaces the bean definition in aConcurrentHashMap(in a synchronized block as well) but then adds the bean name to a list without checking if it already exists.By synchronizing inside
SpringFactory.startwe prevent concurrent modification of the application context bycucumber-springThis in turn makes it possible to share the application context between threads and with JUnit tests.Fixes: #1846
Fixes: #1583
Closes: #1848
Closes: #1582