issue: Component scan two times for @Service

[zhangjiezhang]

Add cache to solve this issue

[kylixs]

Repeated scan should be a problem in the implementation logic. There are two ways to register a scanner, one is to annotate @DubboComponentScan/@EnableDubbo, and the other is to define the configuration: dubbo.scan.base-packages .
A better solution is to register a single scanner (prefer to ServiceClassPostProcessor), then collect all the package names to be scanned from the annotations and configuration, filter the duplicate packages and sub-packages, and then scan.

[chickenlj]

Please check @kylixs 's comments.

[zhangjiezhang]

Please check @kylixs 's comments.

My project uses ServiceClassPostProcessor and only scans once for Component's(DubboService, org.apache.dubbo.config.annotation.Service, com.alibaba.dubbo.config.annotation.Service).

org.apache.dubbo.config.spring.beans.factory.annotation.ServiceClassPostProcessor is used to run org.springframework.context.annotation.ClassPathScanningCandidateComponentProvider.findCandidateComponents(String basePackage) twice:
1.scanner.scan(packageToScan) -> scanner.doScan(basePackages) -> scanner.findCandidateComponents(basePackage): add DubboService lost by AnnotationConfigApplicationContext scan to spring container.
2.ServiceClassPostProcessor.findServiceBeanDefinitionHolders(scanner, packageToScan, registry, beanNameGenerator) -> scanner.findCandidateComponents(packageToScan): get All DubboService.

[kylixs]

Duplicated scanning is resolved in Dubbo 3 :

dubbo/dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/beans/factory/annotation/ServiceAnnotationPostProcessor.java

Line 162 in bdb2d31

private void scanServiceBeans(Set<String> packagesToScan, BeanDefinitionRegistry registry) {

[zhangjiezhang]

Duplicated scanning is resolved in Dubbo 3 :

Still scan twice!!!
The scan results of the first step should be cached for the second step of query。

[kylixs]

First, avoid scan duplicated packages or sub-package:

  // avoid duplicated scans
  if (servicePackagesHolder.isPackageScanned(packageToScan)) {
      if (logger.isInfoEnabled()) {
          logger.info("Ignore package who has already bean scanned: " + packageToScan);
      }
      continue;
  }

Then, exclude scanned classes before generate BeanDefinitions:

ScanExcludeFilter scanExcludeFilter = new ScanExcludeFilter();        
scanner.addExcludeFilter(scanExcludeFilter);

private class ScanExcludeFilter implements TypeFilter {

    private int excludedCount;

    @Override
    public boolean match(MetadataReader metadataReader, MetadataReaderFactory metadataReaderFactory) throws IOException {
        String className = metadataReader.getClassMetadata().getClassName();
        boolean excluded = servicePackagesHolder.isClassScanned(className);
        if (excluded) {
            excludedCount ++;
        }
        return excluded;
    }

    public int getExcludedCount() {
        return excludedCount;
    }
}

[zhangjiezhang]

Then, exclude scanned classes before generate BeanDefinitions:

1.the first scan: scanner.scan(packageToScan)
2.the second scan: findServiceBeanDefinitionHolders(scanner, packageToScan, registry, beanNameGenerator)
You just add classfile to servicePackagesHolder after the second scan, and you cannot avoid called scanner.findCandidateComponents(String basePackage) twice for same class.
Because the classfile scanned for the first time has not been added to servicePackagesHolder.

[kylixs]

1.the first scan: scanner.scan(packageToScan)
2.the second scan: findServiceBeanDefinitionHolders(scanner, packageToScan, registry, beanNameGenerator)

Oh, you're right, I miss it.

The first scan is to register the bean definition of the service implementation class, and the second scan is to register the bean definition of the dubbo ServiceBean later. So, I think the second scan is unnecessary, just use the results of the first scan directly.

Some thing like this:

private void scanServiceBeans(Set<String> packagesToScan, BeanDefinitionRegistry registry) {
   ...
    for (String packageToScan : packagesToScan) {

        // avoid duplicated scans
        if (servicePackagesHolder.isPackageScanned(packageToScan)) {
            if (logger.isInfoEnabled()) {
                logger.info("Ignore package who has already bean scanned: " + packageToScan);
            }
            continue;
        }

        // Scan and registers as Spring @Service Bean first
        Set<BeanDefinitionHolder> beanDefinitionHolders = scanner.doScan(packageToScan);

        // Second, register bean definition of dubbo ServiceBean for each service
        if (!CollectionUtils.isEmpty(beanDefinitionHolders)) {
            ...
            for (BeanDefinitionHolder beanDefinitionHolder : beanDefinitionHolders) {
                processScannedBeanDefinition(beanDefinitionHolder, registry, scanner);
                servicePackagesHolder.addScannedClass(beanDefinitionHolder.getBeanDefinition().getBeanClassName());
            }
        } else {
            if (logger.isWarnEnabled()) {
                logger.warn("No class annotated by Dubbo @Service was found under package ["
                        + packageToScan + "], ignore re-scanned classes: " + scanExcludeFilter.getExcludedCount());
            }
        }
        servicePackagesHolder.addScannedPackage(packageToScan);
    }
}

However, if the @Service annotation of Spring is scanned first, and the @DubboService annotation of dubbo is scanned secondly, the bean of the service implementation class has already been registered, it will be ignored.
So it is necessary to re-implement the logic of doScan, regardless of whether the beanDefinition already exists, or create a bean definition to register the dubbo ServiceBean.

[kylixs]

super.doScan(basePackage): will do first scanning
findCandidateComponents() : will do second scanning
This did not reduce scanning.

[zhangjiezhang]

super.doScan(basePackage): Call method findCandidateComponents() for first time run, this method will cache scan result.
findCandidateComponents(basePackage): this method run for the second time and will use cached results instead of scanning.
So this reduce scanning for the second time.

[zhangjiezhang]

I rewrite method findCandidateComponents for DubboClassPathBeanDefinitionScanner.

[kylixs]

I think just cache Set<BeanDefinitionHolder> in findCandidateComponents method is enough, do not need add new doScan method

[zhangjiezhang]

Solve the cause of the problem

[zhangjiezhang]

I rewrite method findCandidateComponents for DubboClassPathBeanDefinitionScanner.

[kylixs]

Another way is to cache the beanName and BeanDefinition in the postProcessBeanDefinition method, so that there is no need to regenerate the beanName, which is more reliable.

public class DubboClassPathBeanDefinitionScanner extends ClassPathBeanDefinitionScanner {

    private Set<BeanDefinitionHolder> beanDefinitionHolders = new LinkedHashSet<>();
    ....

    @Override
    protected void postProcessBeanDefinition(AbstractBeanDefinition beanDefinition, String beanName) {
        beanDefinitionHolders.add(new BeanDefinitionHolder(beanDefinition, beanName));
        super.postProcessBeanDefinition(beanDefinition, beanName);
    }

    public Set<BeanDefinitionHolder> getBeanDefinitionHolders() {
        return beanDefinitionHolders;
    }
}

// ServiceAnnotationPostProcessor

    private void scanServiceBeans(Set<String> packagesToScan, BeanDefinitionRegistry registry) {
      ...
        for (String packageToScan : packagesToScan) {
            ...
            // Scan and registers as Spring @Service Bean first
            // if the service class is already scanned and registered by @ComponentScan, the bean definitions will be ignored.
            scanner.scan(packageToScan);

            Set<BeanDefinitionHolder> beanDefinitionHolders = scanner.getBeanDefinitionHolders();

            // Second, register bean definition of dubbo ServiceBean for each service
            if (!CollectionUtils.isEmpty(beanDefinitionHolders)) {
            ....
      ....
}

[zhangjiezhang]

Another way is to cache the beanName and BeanDefinition in the postProcessBeanDefinition method, so that there is no need to regenerate the beanName, which is more reliable.

public class DubboClassPathBeanDefinitionScanner extends ClassPathBeanDefinitionScanner {

    private Set<BeanDefinitionHolder> beanDefinitionHolders = new LinkedHashSet<>();
    ....

    @Override
    protected void postProcessBeanDefinition(AbstractBeanDefinition beanDefinition, String beanName) {
        beanDefinitionHolders.add(new BeanDefinitionHolder(beanDefinition, beanName));
        super.postProcessBeanDefinition(beanDefinition, beanName);
    }

    public Set<BeanDefinitionHolder> getBeanDefinitionHolders() {
        return beanDefinitionHolders;
    }
}

// ServiceAnnotationPostProcessor

    private void scanServiceBeans(Set<String> packagesToScan, BeanDefinitionRegistry registry) {
      ...
        for (String packageToScan : packagesToScan) {
            ...
            // Scan and registers as Spring @Service Bean first
            // if the service class is already scanned and registered by @ComponentScan, the bean definitions will be ignored.
            scanner.scan(packageToScan);

            Set<BeanDefinitionHolder> beanDefinitionHolders = scanner.getBeanDefinitionHolders();

            // Second, register bean definition of dubbo ServiceBean for each service
            if (!CollectionUtils.isEmpty(beanDefinitionHolders)) {
            ....
      ....
}

Yes, you're right.👍
Either way is OK.

[kylixs]

@zhangjiezhang Please improve it and resubmit pr.

[zhangjiezhang]

@zhangjiezhang Please improve it and resubmit pr.

Which branch to modify?

[kylixs]

Create two pull requests separately based on master and 3.0 branches, please use concise code whenever possible.