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

Concurrency Exception during bean configuration related to not thread safe getBeanPostProcessor access [SPR-17286] #21819

Closed
spring-projects-issues opened this issue Sep 18, 2018 · 10 comments
Assignees
Labels
in: core status: backported type: bug
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

Sebastien Fuchs opened SPR-17286 and commented

Happens during initialisation of projects using Akka actors enriched by Spring beans ex:

Spring service -> akka actor created and autowired during postConstruct.

Log:

[INFO] F8: Caused by: java.util.ConcurrentModificationException: null
[INFO] F8: 	at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:909)
[INFO] F8: 	at java.util.ArrayList$Itr.next(ArrayList.java:859)
[INFO] F8: 	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.predictBeanType(AbstractAutowireCapableBeanFactory.java:614)
[INFO] F8: 	at org.springframework.beans.factory.support.AbstractBeanFactory.isTypeMatch(AbstractBeanFactory.java:555)
[INFO] F8: 	at org.springframework.beans.factory.support.DefaultListableBeanFactory.doGetBeanNamesForType(DefaultListableBeanFactory.java:432)
[INFO] F8: 	at org.springframework.beans.factory.support.DefaultListableBeanFactory.getBeanNamesForType(DefaultListableBeanFactory.java:395)
[INFO] F8: 	at org.springframework.beans.factory.support.DefaultListableBeanFactory.getBeanNamesForType(DefaultListableBeanFactory.java:389)
[INFO] F8: 	at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveNamedBean(DefaultListableBeanFactory.java:1002)
[INFO] F8: 	at org.springframework.beans.factory.support.DefaultListableBeanFactory.getBean(DefaultListableBeanFactory.java:345)
[INFO] F8: 	at org.springframework.beans.factory.support.DefaultListableBeanFactory.getBean(DefaultListableBeanFactory.java:340)
[INFO] F8: 	at org.springframework.context.support.AbstractApplicationContext.getBean(AbstractApplicationContext.java:1092)
[INFO] F8: 	at com.asaoweb.exmessaging.server.actors.SpringActorProducer.produce(SpringActorProducer.java:36)
[INFO] F8: 	at akka.actor.Props.newActor(Props.scala:212)
[INFO] F8: 	at akka.actor.ActorCell.newActor(ActorCell.scala:624)
[INFO] F8: 	at akka.actor.ActorCell.create(ActorCell.scala:650)

 


Affects: 4.3.19

Issue Links:

  • #17034 Concurrent registration/iteration in PropertySourcesPropertyResolver
  • #21980 StandardEvaluationContext does not support concurrent variable access

Referenced from: commits d15abfd, fa06faa, 1756f83, 0d1bf52, 4642c32

Backported to: 5.0.10

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

Juergen Hoeller commented

So is there a BeanPostProcessor getting registered in parallel, leading to a race condition? We generally recommend full post-processor setup in the bootstrap thread, not expecting concurrent modificaitons to the post-processor list thereafter.

Also, any idea why this only happens in 4.3.x but not in 5.x anymore, since the general post-processor management is largely the same?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

Sebastien Fuchs commented

I'm preparing a simple fix of AbstractAutowireCapableBeanFactory (using ReentrantLock).

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

Sebastien Fuchs commented

Sorry about the version. Did not test enough out of 4.3.x. My mistake.

The problem comes from the way Akka creates actors: through its own dispatcher threads. So the actor beans are getting autowired from those threads and not the bootstrap thread.

Code derived from: https://github.com/typesafehub/activator-akka-java-spring/tree/master/src/main/java/sample

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

Juergen Hoeller commented

Even in such an environment, I wonder where the late registration of BeanPostProcessor instances comes from. The initial ApplicationContext refresh should still happen in a bootstrap thread there, with the context instance only accessed after the refresh finished, for the purposes of autowiring application beans in other threads... with no further post-processors getting registered at that point.

If we have to be defensive about BeanPostProcessor registration there, I'd rather use a CopyOnWriteArrayList for the beanPostProcessors field (and also for embeddedValueResolvers since it could suffer from the same late-registration effect). This also covers the direct exposure of that List through getBeanPostProcessors and allows for concurrent post-processor invocations on multiple beans.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

Sebastien Fuchs commented

Fix submitted: #1962

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

Juergen Hoeller commented

Thanks for the suggested patch! However, as mentioned above, a CopyOnWriteArrayList declaration seems like a better trade-off here. I've locally tried it and it seems to work fine so far.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

Sebastien Fuchs commented

And it's an inner bean if that helps.

Could it be PostConstruct of the bean itself (the actor then) that modifies the beanPostProcessors ?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

Sebastien Fuchs commented

CopyOnWriteArrayList is surely much safer. But much slower.

Just tell me if you prefer a new PR using CopyOnWriteArrayList.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

Juergen Hoeller commented

Along the lines of my previous comment and an earlier change in #17034, I went with a CopyOnWriteArrayList for the beanPostProcessors and embeddedValueResolvers fields. This fits our usage profile quite well (a few modifications on startup where CopyOnWriteArrayList adds some overhead, then just iteration where it is very efficient) and covers external access to getBeanPostProcessor() as well (where all we expose is a List that we can't guard any further). I'll backport this change to 5.0.10 and 4.3.20 ASAP.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 14, 2018

Juergen Hoeller commented

I eventually reverted the fix in 4.3.x since it turned out to cause subtle side effects for callers to getBeanPostProcessors(), expecting Iterator.remove to work and the returned List to remain a stable reference. This was easy enough to fix in 5.x where those callers use List.removeIf in the meantime, and where we can generally make a stronger push towards callers to adapt if necessary.

All in all, this is addressed in 5.1 and also 5.0.10 now. For any concurrent BeanPostProcessor registration needs, please upgrade to 5.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core status: backported type: bug
Projects
None yet
Development

No branches or pull requests

2 participants