fix_early_postprocessor_bug #7752
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7752 +/- ##
============================================
+ Coverage 59.23% 59.92% +0.69%
- Complexity 530 544 +14
============================================
Files 1076 1090 +14
Lines 43540 43843 +303
Branches 6363 6403 +40
============================================
+ Hits 25789 26271 +482
+ Misses 14888 14625 -263
- Partials 2863 2947 +84
Continue to review full report at Codecov.
|
|
LGTM. Would you please add some unit test cases for this? |
|
why not use BeanFactoryAware ？ |
|
DubboEarlyApplicationContext implements BeanFactoryAware or DubboConfigEarlyInitializationPostProcessor implements BeanFactoryAware？ |
|
DubboConfigEarlyInitializationPostProcessor need early add in BeanPostProcessor. |
|
it is like other early PostProcessor. see ImportAwareBeanPostProcessor |
|
@owen200008 This test case is not a normal user scenario, and it seems difficult to understand. |
|
I don't understand why the registration of |
|
it means maybe aop is not available。 |
|
early beanprocess shoud not registry into beanDefinition. |
|
is my fault. english is not so good. make puzzle. |
| @EnableDubboConfig | ||
| @PropertySource("classpath:/META-INF/issue-7752-test.properties") | ||
| @DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_EACH_TEST_METHOD) | ||
| public class Issue7752Test { |
There was a problem hiding this comment.
Can you provide a test for a general application scenario?
There was a problem hiding this comment.
This will make it clearer what problem is solved, and it will be easier to understand.
|
If you just want to register DubboConfigEarlyInitializationPostProcessor earlier, you only need to modify the place where it is registered. It is not recommended to add a wrapper, which makes the registration logic difficult to understand. For example, register it in BeanDefinitionRegistryPostProcessor#postProcessBeanDefinitionRegistry. |
|
The processing of DubboConfigEarlyInitializationPostProcessor is not elegant enough. |
|
Please follow @kylixs 's instructions to further enhance this patch. |
|
@kylixs @chickenlj DubboConfigEarlyInitializationPostProcessor must not be implements BeanDefinitionRegistryPostProcessor#postProcessBeanDefinitionRegistry. |
|
spring code is not elegant enough. the DubboConfigEarlyInitializationPostProcessor in the getBeanPostProcessorCount and in the postProcessorNames. so the size > real BeanPostProcessor size |
|
I saw this error elsewhere. It should be that new beans were created and registered during the execution of registerBeanPostProcessors(). These new beans cannot be processed by all BeanPostProcessors because there are still some BeanPostProcessors that are not registered. BeanPostProcessorChecker is used to detect this problem. So, I think it can be broken down into several things:
|
|
@kylixs you are right. |
|
@owen200008 I tested this pr locally. First, I removed the code related to
|
|
This problem was reproduced in Two registered locations:
private void initBeanFactory() {
if (beanFactory != null) {
// Register itself
...
beanFactory.addBeanPostProcessor(this);
}
}
//org.springframework.context.support.PostProcessorRegistrationDelegate
public static void registerBeanPostProcessors(
ConfigurableListableBeanFactory beanFactory, AbstractApplicationContext applicationContext) {
String[] postProcessorNames = beanFactory.getBeanNamesForType(BeanPostProcessor.class, true, false);
// Register BeanPostProcessorChecker that logs an info message when
// a bean is created during BeanPostProcessor instantiation, i.e. when
// a bean is not eligible for getting processed by all BeanPostProcessors.
int beanProcessorTargetCount = beanFactory.getBeanPostProcessorCount() + 1 + postProcessorNames.length;
beanFactory.addBeanPostProcessor(new BeanPostProcessorChecker(beanFactory, beanProcessorTargetCount));
....
}The same bean registered twice will only keep one: // org.springframework.beans.factory.support.AbstractBeanFactory#addBeanPostProcessor
public void addBeanPostProcessor(BeanPostProcessor beanPostProcessor) {
Assert.notNull(beanPostProcessor, "BeanPostProcessor must not be null");
// Remove from old position, if any
this.beanPostProcessors.remove(beanPostProcessor);
// Track whether it is instantiation/destruction aware
if (beanPostProcessor instanceof InstantiationAwareBeanPostProcessor) {
this.hasInstantiationAwareBeanPostProcessors = true;
}
if (beanPostProcessor instanceof DestructionAwareBeanPostProcessor) {
this.hasDestructionAwareBeanPostProcessors = true;
}
// Add to end of list
this.beanPostProcessors.add(beanPostProcessor);
}Solution: So the solution is keep the first registration, but remove the second registration, that is not to register it as a common bean to The improvement of this PR is generally right, but the name of Also add a use case to reproduce the problem, it is estimated that a common dubbo application example is enough. |
|
#9397 fix it |
What is the purpose of the change
fix bug
early post processor must not Registry beanDefinition
BeanPostProcessorChecker check error, because it think the beanprocess size is less than create. so must have some beanprocess not deal with the bean
see spring error:
same
apache/dubbo-spring-boot-project#786
Brief changelog
Verifying this change
[Dubbo-XXX] Fix UnknownException when host config not exist #XXX. Each commit in the pull request should have a meaningful subject line and body.mvn clean install -DskipTests=false&mvn clean test-compile failsafe:integration-testto make sure unit-test and integration-test pass.