fix synchronized problem, issues #26377
Comment From: pivotal-cla
@jeooica Please sign the Contributor License Agreement!
Click here to manually synchronize the status of this Pull Request.
See the FAQ for frequently asked questions.
Comment From: flyhenanren
We need it in 5.3.x branch.
Comment From: pivotal-cla
@jeooica Thank you for signing the Contributor License Agreement!
Comment From: ryl2018
good fix
Comment From: leexgone
Great!
Comment From: jhoeller
After reviewing our arrangement there, it turns out that the calculateArgumentBindings synchronization is effectively unnecessary these days: We always create AspectJ advice instances with an early call to calculateArgumentBindings in ReflectiveAspectJAdvisorFactory, not requiring any synchronization for that standard scenario. The implicit lazy calculation of argument bindings is only really there for test scenarios or other custom arrangements, and there are no guarantees for thread safety defined there. Given the commonality of the standard scenario, I'll optimize for that, recommending custom scenarios to also perform an early calculateArgumentBindings call if necessary.
From that perspective, the approach taken in this PR won't be needed anymore. I'll reopen #26377 accordingly. Thanks for the PR, in any case!
For a review, even if we are not using that approach: The implementation in the PR seems incomplete in terms of its memory barriers. At the very least, argumentsIntrospected would have to be volatile for such a nested synchronization approach to work. Even the data structures populated by bindArgumentsByName would have to be guarded for thread-safe publication through the use of concurrent data structures, since otherwise it is not guaranteed that their state is visible to other threads when argumentsIntrospected is true in that early check before the synchronized block.