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

Deadlock possible with AspectJ aspects and multi-threading [SPR-14241] #18814

Closed
spring-issuemaster opened this issue May 3, 2016 · 3 comments
Closed
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented May 3, 2016

Kevin Richards opened SPR-14241 and commented

When AspectJ style aspects are used, the aspects are eagerly converted to Spring Advisors, but the AspectJ aspect object is not eagerly created. It is lazily created at the time of its first use. This can be seen in ReflectiveAspectJAdvisorFactory's use of LazySingletonAspectInstanceFacotryDecorator.

When a singleton Spring bean is created, it grabs the lock of singleton bean map. If, inside its constructor, it calls a function that has an uninitialized aspect wrapped around it, it will then grab the lock in LazySingletonAspectInstanceFactoryDecorator.getAspectInstance.

If, in a different thread (started after some other bean finishes construction), a call is made that invokes the same aspect, which has not yet been initialized, the lock in LazySingletonAspectInstanceFactoryDecorator.getAspectInstance will be acquired and then the singleton aspect bean will be instantiated. This requires going through the singleton bean instantiation code, which acquires the singleton bean map lock.

This leads to a deadlock. I've attached a stack trace from jstack of this situation occurring. I think one potential solution would be to eagerly instantiate the singleton AspectJ aspects, not just the advisors that wrap them.


Affects: 3.2.16, 4.1.7

Attachments:

Issue Links:

  • #17336 Deadlock publishing event while creating listener bean
  • #10339 avoid synchronization when AspectJExpressionPointcut.getShadowMatch hits cache
  • #18896 Async advisor retrieval blocks when triggered by singleton init method
  • #18961 Deadlock while creating a new thread on bean initialization with transactional code invocation

Backported to: 3.2.17

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 3, 2016

Juergen Hoeller commented

Indeed, the use of separate locks leads to quite some deadlock potential here. We opted for a shared-lock approach in other places (see #17336), and this also seems to work here: LazySingletonAspectInstanceFactoryDecorator needs to use ConfigurableBeanFactory.getSingletonMutex() in order to preserve lazy-init characteristics while avoiding a deadlock with other factory-calling threads.

In 4.3, we can have first-class support for this in the MetadataAwareAspectInstanceFactory abstraction. However, for 4.2.6, we'll rather have to do some conditional downcasting to BeanFactoryAspectInstanceFactory... And for 3.2.17, which is unfortunately also affected, we'll have to do even more downcasting since ConfigurableBeanFactory doesn't expose getSingletonMutex() there yet.

As a side note, this also reveals that LazySingletonAspectInstanceFactoryDecorator.getAspectInstance() has an outdated synchronized marker at the method level. #10339 (back in 2009!) was meant to avoid contention through the use of double-checked locking there, but with method-level synchronization, that change had no actual benefits. We'll properly address this in the course of our fix here now.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 4, 2016

Juergen Hoeller commented

The suggested fix will be available in the upcoming 4.3.0.BUILD-SNAPSHOT as well as 4.2.6.BUILD-SNAPSHOT in about half an hour. Please give it a try if you have the chance...

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 4, 2016

Kevin Richards commented

Thanks for addressing this so quickly. When the 4.2.6 GA is released, I'll upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.