Atomic operations for BeanDefinitionProducer #11570
Conversation
Replace the BeanDefinitionProducer API with atomic operations. This removes patterns like `if (producer.isReferenceEnabled()) producer.getReference()` which could cause TOCTOU concurrency issues. Should fix #11569
There was a problem hiding this comment.
Looks OK though it'd be good to have a comment justifying this sort of very fine grained parallelism with performance numbers. There are some unintuitive states possible here (e.g. reference == null whilst definition is not the sentinel). I can see why bean management would be performance sensitive, but how much difference does all this really make?
| @@ -2078,7 +2080,7 @@ protected void initializeContext( | |||
| if (CollectionUtils.isNotEmpty(parallelBeans)) { | |||
| processParallelBeans(parallelBeans); | |||
| } | |||
| ForkJoinPool.commonPool().execute(() -> beanDefinitionsClasses.forEach(p -> p.isReferenceEnabled(this))); | |||
| ForkJoinPool.commonPool().execute(() -> beanDefinitionsClasses.forEach(p -> p.getReferenceIfEnabled(this))); | |||
There was a problem hiding this comment.
What is this line trying to do originally? It looks a bit odd.
There was a problem hiding this comment.
I think it's kicking off async loading of all bean definitions that were not initialized by the previous code
inject/src/main/java/io/micronaut/context/DefaultBeanContext.java
Outdated
Show resolved
Hide resolved
|
|
Is it going to affect the startup performance? |
|
@sdelamo i don't know how to reproduce the original bug, it's some weird race condition |
|
@dstepanov I think it should be no worse than before, there's no additional locks or extra work. maybe it's even slightly faster |
|
@dstepanov If you are ok with this, lets merge it and release 4.8.1 core |
Replace the BeanDefinitionProducer API with atomic operations. This removes patterns like
if (producer.isReferenceEnabled()) producer.getReference()which could cause TOCTOU concurrency issues.Should fix #11569