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

fix #3386 #3395

Merged
merged 2 commits into from
May 29, 2024
Merged

fix #3386 #3395

merged 2 commits into from
May 29, 2024

Conversation

dowenliu-xyz
Copy link
Contributor

Describe what this PR does / why we need it

This PR contains fixes for #3386

Does this pull request fix one issue?

Fixes #3386

Describe how you did it

Combine origin method's parameter type canonical names into register map key.

Describe how to verify it

See bug #3386 to repreduce and verify.

Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented May 12, 2024

CLA assistant check
All committers have signed the CLA.

@LearningGp
Copy link
Collaborator

这看起来确实是个问题,但是要不要在运行时支持这种同名我觉得可能得再讨论下,因为这样做会引入一些不必要的额外的复杂度,但其实只要不重名就不会有问题,另外 Issue 中的这个插件看起来是个不错的解决方案

@dowenliu-xyz
Copy link
Contributor Author

#3397 也要改这一处。我觉得改 #3397 还得把返回值类型信息也加到 key 里,复杂度会更高些 😂

@dowenliu-xyz
Copy link
Contributor Author

dowenliu-xyz commented May 21, 2024

从概率论上讲,代码里出现重名导致这个bug是一定会出现的。所以要讨论的是怎么处理这个bug。
要么就这么放着不处理,相当于认为是快速失败,那么遇到这个问题的团队能不能避免生产事故就要看研发有没有了解到这个bug、测试有没有测到这个场景了;否则就进行修复。
修复方案我的想法是既要避免报错,也保证流控不失效(不打破研发设计预期)。
关于复杂度,我没觉得这是个问题,而且我变更的代码我自己觉得还挺可控的,不复杂吧。😂
@LearningGp 你有其他的修复思路吗?

P.S. 欢迎试用插件。如果觉得不错,可以留言评论+分享。插件已设置开源项目 free 🤝。

Copy link

codecov bot commented May 24, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 45.85%. Comparing base (dbd3c06) to head (945468f).
Report is 1 commits behind head on 1.8.

Files Patch % Lines
...nterceptor/AbstractSentinelInterceptorSupport.java 0.00% 8 Missing ⚠️
...l/annotation/aspectj/ResourceMetadataRegistry.java 84.61% 1 Missing and 1 partial ⚠️
...tion/cdi/interceptor/ResourceMetadataRegistry.java 84.61% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                1.8    #3395      +/-   ##
============================================
- Coverage     45.90%   45.85%   -0.06%     
  Complexity     2148     2148              
============================================
  Files           431      431              
  Lines         12906    12932      +26     
  Branches       1728     1728              
============================================
+ Hits           5925     5930       +5     
- Misses         6281     6301      +20     
- Partials        700      701       +1     
Components Coverage Δ
sentinel-adapter 43.22% <ø> (ø)
sentinel-cluster 23.71% <ø> (-0.08%) ⬇️
sentinel-core 59.64% <ø> (+0.01%) ⬆️
sentinel-extension 45.88% <71.42%> (-0.27%) ⬇️
sentinel-logging 54.54% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LearningGp
Copy link
Collaborator

如果方便的话,麻烦在sentinel-demo-annotation-spring-aop模块中完善下Demo以体现新的特性

@dowenliu-xyz
Copy link
Contributor Author

如果方便的话,麻烦在sentinel-demo-annotation-spring-aop模块中完善下Demo以体现新的特性

OK,我这两天看时间搞下。

Run module sentinel-demo-annotation-spring-aop, and run following
requests (in curl format) to verify that fallbacks won't overlap
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
@dowenliu-xyz
Copy link
Contributor Author

Done.

Run module sentinel-demo-annotation-spring-aop, and run following
requests (in curl format) to verify that fallbacks won't overlap
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

@dowenliu-xyz
Copy link
Contributor Author

另外昨天还看了下 resilience4j 的做法。
https://github.com/resilience4j/resilience4j/blob/eeaf57a8217ded7fe14bad511454f263f9e6f06d/resilience4j-spring6/src/main/java/io/github/resilience4j/spring6/fallback/FallbackMethod.java#L85
他们的 MethodMeta 结构我觉得值得参考。(可以直接照抄😅)

@GetMapping("/bar")
public String apiBar(@RequestParam(required = false) String t) {
service.test();
return service.hello(t);

Check warning

Code scanning / CodeQL

Cross-site scripting Medium

Cross-site scripting vulnerability due to a
user-provided value
.
Copy link
Collaborator

@LearningGp LearningGp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LearningGp LearningGp merged commit 568e2df into alibaba:1.8 May 29, 2024
10 checks passed
@LearningGp
Copy link
Collaborator

另外昨天还看了下 resilience4j 的做法。 https://github.com/resilience4j/resilience4j/blob/eeaf57a8217ded7fe14bad511454f263f9e6f06d/resilience4j-spring6/src/main/java/io/github/resilience4j/spring6/fallback/FallbackMethod.java#L85 他们的 MethodMeta 结构我觉得值得参考。(可以直接照抄😅)

感谢贡献🚀,欢迎后续进行进一步优化

@dowenliu-xyz dowenliu-xyz deleted the fix_3386 branch May 29, 2024 12:44
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 this pull request may close these issues.

Same name fallback/blockHandler with different parameter types cause reflect exception
3 participants