Skip to content
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

Fix transaction aspect's incorrect state after creating new context #23650

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SammyVimes
Copy link
Contributor

@SammyVimes SammyVimes commented Sep 17, 2019

As aspects are singletons, transactionManagerCache and transactionManager in TransactionAspectSupport are persistent. It's ok if context is not changing, but if new contexts gets created (in tests, for example), new transaction manager should be used.

I faced it using Spring Data with repository's @Transactional set to MANDATORY:

  1. Service's Transactional method is called
  2. Transaction is created in aspect
  3. Transaction manager is put to cache
  4. Transaction is used in TransactionInterceptor of SpringData repository
  5. New context got created for another test
  6. Service's Transactional method is called
  7. Transaction is created in old transaction manager, that is cached inside aspect
  8. TransactionInterceptor in SpringData repository fails to find transaction, because it uses different (new) transaction manager

I fixed it with clearing cache every time aspect's bean is created (and also remove a null check for txManager parameter, because if it's null, it should clear txManager field and also setter's parameter marked Nullable) and created tests for that.

I also had to change gradle settings for integration tests, because they were not processed by aspectj AND that fixed one strange test where aspects were not found in classpath.

To demonstrate the issue, I also created this repository, where the test which is run second always fails.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 17, 2019
@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) in: test Issues in the test module labels Sep 24, 2019
@sbrannen
Copy link
Member

Potential fix for #11019

@sbrannen sbrannen added this to the 5.x Backlog milestone Sep 24, 2019
@sbrannen sbrannen added status: pending-design-work Needs design work before any code can be developed and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 24, 2019
@sbrannen
Copy link
Member

Assigned to the 5.x backlog for further investigation

@SammyVimes
Copy link
Contributor Author

By the way, I assume there would be similar cases for all aspects. If the assumption will be confirmed, I would gladly fix other aspects as well.

@sbrannen sbrannen marked this pull request as draft September 28, 2023 08:22
@sbrannen sbrannen removed the status: pending-design-work Needs design work before any code can be developed label Sep 28, 2023
@jhoeller jhoeller modified the milestones: 6.x Backlog, General Backlog Dec 23, 2023
@jhoeller jhoeller added the type: enhancement A general enhancement label Jan 11, 2024
@jhoeller jhoeller modified the milestones: General Backlog, 6.x Backlog Jan 11, 2024
@jhoeller jhoeller added in: data Issues in data modules (jdbc, orm, oxm, tx) and removed in: core Issues in core modules (aop, beans, core, context, expression) labels Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) in: test Issues in the test module type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants