fix: EXPOSED-1028 Return defensive copy from ExposedSpringTransactionAttributeSource#2812
Open
Yuya Urano (yurano) wants to merge 2 commits into
Conversation
…nsactionAttributeSource
The default delegate (AnnotationTransactionAttributeSource) caches the
RuleBasedTransactionAttribute it returns per (method, targetClass).
ExposedSpringTransactionAttributeSource currently mutates that shared
instance, which is a data race across concurrent callers and was observed
in production as a NullPointerException in ArrayList.toArray (unsafe
publication of the new ArrayList through the non-volatile rollbackRules
field).
These tests document the regression. The deterministic invariant test
('delegate's cached attribute is not mutated by source') fails against the
current implementation; the stress test ('concurrent getTransactionAttribute
calls are thread-safe') guards against future races.
…AttributeSource The delegate (typically AnnotationTransactionAttributeSource) caches the RuleBasedTransactionAttribute it returns. Mutating its rollbackRules in place was a data race across concurrent callers and caused NullPointerException in ArrayList.toArray under load (unsafe publication of the new ArrayList through the non-volatile rollbackRules field). Use the RuleBasedTransactionAttribute(other) copy constructor and only modify the copy. The delegate's cached instance is no longer touched. Applied identically to spring-transaction (Spring 6) and spring7-transaction (Spring 7).
Author
|
Chantal Loncle (@bog-walk) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summary of the change:
ExposedSpringTransactionAttributeSourcenow returns a defensive copy of the delegate'sRuleBasedTransactionAttributeinstead of mutating the shared, cached instance, fixing a thread-safety bug that causedNullPointerExceptionunder concurrent load.Detailed description:
AnnotationTransactionAttributeSource, caches theRuleBasedTransactionAttributeit returns per(method, targetClass). The previous implementation readattr.rollbackRules.toMutableList(), mutated the list, and assigned it back toattr.rollbackRules— a read-modify-write on shared state across concurrent callers. In production we observedNullPointerExceptionthrown fromArrayList.toArrayinsidekotlin.collections.toMutableList: a reader thread observed the reference to the freshly-publishedArrayListwhile itselementDatafield was still unset (classic unsafe publication through the non-volatilerollbackRulesfield). The stack trace and full analysis are in YouTrack EXPOSED-1028.getTransactionAttributenow copies the delegate's attribute via theRuleBasedTransactionAttribute(other)copy constructor and only mutates the copy. The delegate's cached instance is no longer touched. The change is applied identically tospring-transaction(Spring 6) andspring7-transaction(Spring 7).RuleBasedTransactionAttribute(other)produces an independent attribute with its ownrollbackRuleslist. The configurablerollbackExceptionsconstructor parameter is preserved, and the returned attribute remains semantically equivalent to the previous implementation (same propagation, isolation, timeout, read-only flag, and full set of rollback rules including the configured exceptions). Spring'sTransactionAttributeSourcecontract does not promise instance identity, andAbstractFallbackTransactionAttributeSourceitself returns copies in similar scenarios, so this is a non-breaking change.Commits are split for clarity: the first adds failing regression tests (deterministic invariant + stress), the second applies the fix and turns them green.
Type of Change
Please mark the relevant options with an "X":
Updates/remove existing public API methods:
Affected databases:
Checklist
Related Issues