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

[spring-projects-issues]

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:

• deadlock.txt (65.53 kB)

Issue Links:

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

Backported to: 3.2.17

[spring-projects-issues]

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-projects-issues]

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-projects-issues]

Kevin Richards commented

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

[asgs]

Hello, @jhoeller

We're on Spring 4.3.2.RELEASE and experiencing the same deadlock issue. How do we know if this fix is available in this one? Please let know if you need more details