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

AbstractBeanFactory's interaction with BeanPostProcessorCacheAwareList is not fully thread-safe #29299

Closed
xaviertesch opened this issue Oct 11, 2022 · 3 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: regression A bug that is also a regression
Milestone

Comments

@xaviertesch
Copy link

xaviertesch commented Oct 11, 2022

Affects: 5.3.x


Hi,

AbstractBeanFactory implementation is causing issues in our application since version 5.3. The application randomly failed to initialize with the following error :

WARN o.s.b.w.s.c.AnnotationConfigServletWebServerApplicationContext - Exception encountered during context initialization - cancelling refresh attempt: org.springframework.context.ApplicationContextException: Unable to start web server; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'errorPageFilterRegistration' defined in org.springframework.boot.web.servlet.support.Error PageFilterConfiguration: Unsatisfied dependency expressed through method 'errorPageFilterRegistration' parameter 0; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'errorPageFilter' defined in org.springframework.boot.web.servlet.support.ErrorPageFilterConfiguration: Initialization of bean failed; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'org.spring framework.boot.autoconfigure.web.servlet.error.ErrorMvcAutoConfiguration': Instantiation of bean failed; nested exception is org.springframework.beans.BeanInstantiationException: Failed to instantiate [org.springframework.boot.autoconfigu re.web.servlet.error.ErrorMvcAutoConfiguration]: No default constructor found; nested exception is java.lang.NoSuchMethodException: org.springframework.boot.autoconfigure.web.servlet.error.ErrorMvcAutoConfiguration.<init>() []

I finally managed to understand what is happening and believe this can and should be fixed in spring-beans.

BeanPostProcessorCacheAwareList was introduced in AbstractBeanFactory in 5.3.

BeanPostProcessorCacheAwareList and accesses to the beanPostProcessors instance are not Thread safe. If multiple Threads are running during initialization and a Thread calls getBeanPostProcessorCache() while another Thread is calling addBeanPostProcessors, you can end up with a cache which does not contain all BeanPostProcessor instances and thus doesn't find the appropriate constructor.

Making BeanPostProcessorCacheAwareList and its usages in AbstractBeanFactory thread safe is relatively straightforward with synchronized blocks and would prevent such very obscure initialization issues.

Kind regards,
Xavier

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 11, 2022
@jhoeller jhoeller self-assigned this Oct 11, 2022
@jhoeller jhoeller added type: regression A bug that is also a regression in: core Issues in core modules (aop, beans, core, context, expression) and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Oct 11, 2022
@jhoeller jhoeller added this to the 5.3.24 milestone Oct 11, 2022
@jhoeller jhoeller changed the title Thread safety issues in AbstractBeanFactory BeanPostProcessorCacheAwareList is not fully thread-safe Oct 11, 2022
@xaviertesch
Copy link
Author

xaviertesch commented Oct 12, 2022

Hi @jhoeller,
thanks for looking into this !
Considering the title update, I would simply like to note that the problem is not only in BeanPostProcessorCacheAwareList, but also in AbstractBeanFactory, as even with a fully thread safe BeanPostProcessorCacheAwareList, at least the two methods named addBeanPostProcessors(...) and addBeanPostProcessor(...) would still not be thread safe. At least unless you would group the remove + add calls in one method in BeanPostProcessorCacheAwareList.

Kind regards,
Xavier

@jhoeller jhoeller changed the title BeanPostProcessorCacheAwareList is not fully thread-safe AbstractBeanFactory's interaction with BeanPostProcessorCacheAwareList is not fully thread-safe Oct 12, 2022
@jhoeller
Copy link
Contributor

Good catch. I'm aware of that remove/add part being the problem, we're going to see what we can do about it, maybe providing dedicated operations for those steps in BeanPostProcessorCacheAwareList indeed.

A general note: It is not common to perform post-processor registration concurrently. A typical Spring application startup is primarily single-threaded, at least during the post-processor registration phase.

Out of curiosity, what kind of concurrent startup arrangement are you using there?

@xaviertesch
Copy link
Author

On our side, this concurrent initialization was not on purpose. A Thread was started during the initialization phase, using a spring bean and triggering initialization of dependent beans in the Thread. All was working fine until 5.3 when we started to experience Exceptions in a small percentage of our application startups.

Clearly our code wasn't optimal but I thought there was no obvious reason not to make this possible in spring, especially considering the time lost on our side investigating this problem.

Note that we are considering concurrent initialization to improve startup times in the future, so if supported and useful, this might become critical for us again. In the meantime, I made sure not to use a different thread during the initialization phase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

3 participants