New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve thread safety in Spring JUnit integration [SPR-12421] #17028

Closed
spring-issuemaster opened this Issue Nov 12, 2014 · 11 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

spring-issuemaster commented Nov 12, 2014

Bastian Voigt opened SPR-12421 and commented

Status Quo

When using the maven surefire setting parallel=methods, JUnit still creates only one runner instance per test class, i.e. one runner instance is used by multiple threads in parallel.

SpringJUnit4ClassRunner uses a single TestContextManager which stores the current TestContext in an instance field.

Proposal

SpringJUnit4ClassRunner should use a ThreadLocal instead of a single TestContextManager instance field.

Deliverables

  1. Improve thread safety for the SpringRunner for JUnit 4.
  2. Improve thread safety for the SpringClassRule and SpringMethodRule for JUnit 4.
  3. Improve thread safety for the SpringExtension for JUnit Jupiter (in JUnit 5).

Affects: 3.0 GA

Reference URL: http://stackoverflow.com/questions/26882936/why-does-springjunit4classrunner-not-work-with-surefire-parallel-methods

Issue Links:

  • #10532 Allow for concurrent test execution in the TestContext framework ("duplicates")

3 votes, 5 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Nov 12, 2014

Bastian Voigt commented

I wrote a patch:

BastianVoigt@a5e466e

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Apr 18, 2015

Sam Brannen commented

Bastian Voigt, please submit an official pull request and make sure you have signed (and acknowledged in your pull request) the Contributor License Agreement.

For details, consult Spring's CONTRIBUTING page.

Thanks!

Sam

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Apr 19, 2015

Sam Brannen commented

Please note that this issue is closely related to (and potentially a duplicate of) #10532.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Aug 25, 2016

Marten Deinum commented

Pull Request available: #1140

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Aug 25, 2016

Marten Deinum commented

Also wondering if this applies to the JUnit5 test support or maybe that already works?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Aug 25, 2016

Sam Brannen commented

Marten Deinum, thanks for submitting the PR.

My major concern here is whether storing the TestContextManager in a ThreadLocal is sufficient for covering more advanced use cases such as the one outlined in #10532.

Have you put any thought into that?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Aug 25, 2016

Sam Brannen commented

Also wondering if this applies to the JUnit5 test support or maybe that already works?

Good question!

The SpringExtension for JUnit Jupiter (i.e., JUnit 5) relies on the org.junit.jupiter.api.extension.ExtensionContext.Store for storing the TestContextManager between invocations of the extension. So thread safety is ultimately determined by the implementation of Store. Store is unfortunately not thread-safe at the moment, so I'll open an issue to address that in JUnit Jupiter.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Aug 25, 2016

Sam Brannen commented

Also, please note that creating new TestContextManager per thread is an expensive task since it executes the entire bootstrapping mechanism (i.e., all search algorithms for locating annotations, loading TestExecutionListeners, loading context customizers, building the MergedContextConfiguration, etc.). And... the bootstrapping process in Spring Boot 1.4 is even more exhaustive.

In contrast, although it is considerably more complex, the general idea behind the proposal in #10532 is to only load the TestContext once and make it thread-safe as opposed to making the TestContextManager thread-safe.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Aug 26, 2016

Marten Deinum commented

I applied this patch as this was the most easy way to achieve this. Also I wondered what would happen if @DirtiesContext was to be used and tests where run in parallel, if there is a shared context one might get some weird issues with that.

However I have to agree that reloading everything per thread might be a bit heavy and with Spring Boot one might run into other issues.

I'll have a stab at integrating the proposed changes from #10532 and related issues. Will create a new branch for that (easier to start with a clean slate I guess).

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Aug 26, 2016

Sam Brannen commented

I applied this patch as this was the most easy way to achieve this.

I assumed that was the case. ;)

Also I wondered what would happen if @DirtiesContext was to be used and tests where run in parallel, if there is a shared context one might get some weird issues with that.

Yes, I assume horribly unpredictable things might happen in such scenarios.

I'll have a stab at integrating the proposed changes from #10532 and related issues. Will create a new branch for that (easier to start with a clean slate I guess).

Sounds good!

And yes... we will definitely have to start from scratch due to all of the changes in recent years to the internals of the TCF. Plus, AFAIK, it is no longer permissible to accept external code other than via GitHub PRs where the contributor has signed the CLA. The same holds true for code in the test suite. So please keep that in mind.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Sep 1, 2016

Sam Brannen commented

After careful consideration, I have decided to resolve this issue as a duplicate of #10532, with the rationale that the correct places to ensure thread safety are the TestContext (i.e., its mutable state) and ApplicationContext loading and eviction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment