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

defaultFallback annotation attribute on class cause defaultFallback handling conflicts on methods with different return types. #3397

Open
dowenliu-xyz opened this issue May 19, 2024 · 4 comments · May be fixed by #3415

Comments

@dowenliu-xyz
Copy link
Contributor

dowenliu-xyz commented May 19, 2024

Issue Description

Type: bug report

Describe what happened

The defaultFallback handling on methods with different return types is depend on calling order when defaultFallback is defined in annotation on class, like the issue #3386 .

defaultFallback 由类上的注解定义时,不同返回值类型的方法的defaultFallback 处理结果与实际方法的被调用顺序相关,这和问题 #3386 类似。

Describe what you expected to happen

The defaultFallback of each method of return type should be correctly handled.

各类型方法的 defaultFallback 可以被正确的处理。

How to reproduce it (as minimally and precisely as possible)

Minimium reproduce demo: https://github.com/dowenliu-xyz/sentinel-default-fallback-on-class-conflict/

  • Minimium dependencies: only spring-boot starter and sentinel-annotation-aspectj:
dependencies {
    implementation 'org.springframework.boot:spring-boot-starter'
    implementation 'com.alibaba.csp:sentinel-annotation-aspectj:1.8.7'
    testImplementation 'org.springframework.boot:spring-boot-starter-test'
    testRuntimeOnly 'org.junit.platform:junit-platform-launcher'
}
  • One additial SentinelResourceAspect bean definition:
    @Bean
    public SentinelResourceAspect sentinelResourceAspect() {
        return new SentinelResourceAspect();
    }
  • Resource/Biz bean definition:
@Component
@SentinelResource(defaultFallback = "df")
public class Biz {
    @SentinelResource
    public int doubled(int a) {
        if (a == 0) {
            throw new IllegalArgumentException("a should not be 0");
        }
        return a * 2;
    }
    public String df() {
        return "fallback";
    }
    @SentinelResource
    public String doubled(String a) {
        if (a == null || a.isEmpty()) {
            throw new IllegalArgumentException("a should not be empty");
        }
        return a + a;
    }
}
  • Method calls
@Component
public class RunBiz implements CommandLineRunner {
    private static final Logger LOG = LoggerFactory.getLogger(RunBiz.class);
    @Resource
    private Biz biz;

    @Override
    public void run(String... args) throws Exception {
        //*/
        // 先执行实际 defaultFallback 不存在的方法, 会导致后面有 defaultFallback 的方法不执行 defaultFallback
        try {
            LOG.error("[1] should not output: {}", biz.doubled(0)); // no output
        } catch (IllegalArgumentException e) {
            LOG.info("[2] exception: {}", e.getMessage()); // go here
        }
        try {
            LOG.info("[3] expect fallback: {}" , biz.doubled("")); // no output
        } catch (IllegalArgumentException e) {
            LOG.error("[4] IllegalArgumentException: {}", e.getMessage()); // go here, an IllegalArgumentException
        }
        /*/
        // 先执行实际 defaultFallback 存在的方法, 会导致后面无回调的方法报错 (报错与业务方法内报错无关,业务异常信息丢失)
        try {
            LOG.info("[5] fallback: {}", biz.doubled("")); // fallback
        } catch (Exception e) {
            LOG.error("[6] should not output: {}", e.getMessage()); // no output
        }
        try {
            LOG.error("[7] should not output: {}", biz.doubled(0)); // no output
        } catch (ClassCastException e) {
            LOG.error("[8] unexpected ClassCastException: {}", e.getMessage()); // go here, a ClassCastException
        } catch (IllegalArgumentException e) {
            LOG.info("[9] expected exception: {}", e.getMessage()); // no output
        }
        //*/
    }
}

Tell us your environment

MacOS 14.3
Temurin 17.0.10
Gradle wrapper 8.7

Anything else we need to know?

Additionally, I really don't understand what the design intent is and what the usage scenarios are for supporting annotations at the class level.

另外,我实在不明白在类级别支持注解是什么设计意图、有什么使用场景。

@dowenliu-xyz
Copy link
Contributor Author

这个问题的修复似乎与 #3386 类似,也要修改 ResourceMetadataRegistry 这个类。我现在不能确定是否代码冲突。等到 #3395 被合并或其他什么方式解决了 #3386 再处理这个 Bug。

我会在这几天先尝试完成 idea 插件对类级别注解的支持。

@dowenliu-xyz
Copy link
Contributor Author

仔细分析了这个bug,不只是 class 级 annotation 会导致这个情况。
只要有多个不同返回值类型的方法的 blockHandlerfallbackdefaultFallback 尝试使用同一处理类的相同名称的处理方法,都有可能会触发这个 bug 里。
具体来说:

  • defaultFallback 不同的方法尝试使用相同处理类中同名的处理方法,只要有一个方法的处理器方法能成功解析就必定触发这个 bug
  • fallbackblockHandler 不同的方法如果方法返回值不同(至少没有继承关系),如果尝试使用同一类中同名处理方法,只要有一个方法的处理器能成功解析就会触发这个 bug。

    如果有多个解析成功,且它们的参数类型列表不同,会触发 Same name fallback/blockHandler with different parameter types cause reflect exception #3386

  • 不论是 blockHandlerfallback 还是 defaultFallback ,如果声明了值,但全都解析失败,找不到实际可用的处理器方法,会巧合的避过这个 bug。

另外,因为类级别注解的查找、非 static 处理方法的查找还会跨继承关系查找,可能实际会有更复杂的冲突情况

@dowenliu-xyz
Copy link
Contributor Author

插件 Alibaba Sentinel Annotation Support 版本已更新,对类注解提供了相关支持,同时对该bug情形进行了检查报错:

Kapture.2024-05-23.at.16.04.07.mp4

@dowenliu-xyz
Copy link
Contributor Author

这个问题的修复似乎与 #3386 类似,也要修改 ResourceMetadataRegistry 这个类。我现在不能确定是否代码冲突。等到 #3395 被合并或其他什么方式解决了 #3386 再处理这个 Bug。

我会在这几天先尝试完成 idea 插件对类级别注解的支持。

有谁能给我讲下,在类上使用注解的设计意图、实际场景吗?我在想是修复这个bug,还是提议干掉类注解。

dowenliu-xyz added a commit to dowenliu-xyz/Sentinel that referenced this issue Jun 8, 2024
An spring-aop example that fits alibaba#3397 case is also provided:

Run module sentinel-demo-annotation-spring-aop, and run following
requests (in curl format) to verify that handlers won't conflict with
each other. Once the module started, you can call requests in any
sequence.

curl -XGET localhost:19966/bar?t=
curl -XGET localhost:19966/foo?t=-1
curl -XGET localhost:19966/count
@dowenliu-xyz dowenliu-xyz linked a pull request Jun 8, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant