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: complete grpc based protocol panic recover handle. #1866

Merged
merged 7 commits into from
May 14, 2022

Conversation

sheny1xuan
Copy link
Contributor

Signed-off-by: stonex 1479765922@qq.com

What this PR does:

  • Dubbo3.0 and grpc based service in dubbo-golang will crash if user throw exception in their code, only getty based protocol catch these exception in method OnMessage.
  • Catch user's exception in Invoke to prevent the server from hanging up due to the exception .

You should pay attention to items below to ensure your pr passes our ci test
We do not merge pr with ci tests failed

  • All ut passed (run 'go test ./...' in project root)
  • After go-fmt ed , run 'go fmt project' using goland.
  • Golangci-lint passed, run 'sudo golangci-lint run' in project root.
  • After import formatted, (using imports-formatter to run 'imports-formatter .' in project root, to format your import blocks, mentioned in CONTRIBUTING.md above)
  • Your new-created file needs to have apache license at the top, like other existed file does.
  • All integration test passed. You can run integration test locally (with docker env). Clone our dubbo-go-samples project and replace the go.mod to your dubbo-go, and run 'sudo sh start_integration_test.sh' at root of samples project root. (M1 Slice is not Support)

stonex added 3 commits April 26, 2022 20:38
Signed-off-by: stonex <1479765922@qq.com>
Signed-off-by: stonex <1479765922@qq.com>
Copy link
Member

@binbin0325 binbin0325 left a comment

Choose a reason for hiding this comment

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

proxy 有default和PassThrough 都需要考虑一下,是不是将defer放在DefaultProxyImplementFunc中的makeDubboCallProxy里更好一些

@codecov-commenter
Copy link

codecov-commenter commented May 6, 2022

Codecov Report

Merging #1866 (d2b0ce8) into 3.0 (84a131c) will decrease coverage by 1.97%.
The diff coverage is 0.00%.

❗ Current head d2b0ce8 differs from pull request most recent head 9abcc65. Consider uploading reports for the commit 9abcc65 to get more accurate results

@@            Coverage Diff             @@
##              3.0    #1866      +/-   ##
==========================================
- Coverage   46.78%   44.80%   -1.98%     
==========================================
  Files         295      283      -12     
  Lines       17173    16943     -230     
==========================================
- Hits         8034     7591     -443     
- Misses       8286     8548     +262     
+ Partials      853      804      -49     
Impacted Files Coverage Δ
common/proxy/proxy_factory/default.go 18.18% <0.00%> (-3.36%) ⬇️
common/proxy/proxy_factory/pass_through.go 25.00% <0.00%> (-6.92%) ⬇️
remoting/getty/listener.go 26.42% <ø> (-0.50%) ⬇️
registry/nacos/listener.go 0.00% <0.00%> (-78.27%) ⬇️
remoting/nacos/builder.go 0.00% <0.00%> (-73.92%) ⬇️
registry/etcdv3/registry.go 3.07% <0.00%> (-70.77%) ⬇️
metadata/report/etcd/report.go 1.11% <0.00%> (-61.12%) ⬇️
registry/etcdv3/listener.go 0.00% <0.00%> (-50.00%) ⬇️
config_center/nacos/listener.go 0.00% <0.00%> (-50.00%) ⬇️
remoting/etcdv3/listener.go 3.53% <0.00%> (-43.37%) ⬇️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84a131c...9abcc65. Read the comment docs.

func (pi *ProxyInvoker) Invoke(ctx context.Context, invocation protocol.Invocation) protocol.Result {
result := &protocol.RPCResult{}
func (pi *ProxyInvoker) Invoke(ctx context.Context, invocation protocol.Invocation) (result protocol.Result) {
result = &protocol.RPCResult{}
Copy link
Contributor

Choose a reason for hiding this comment

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

不要在返回值处定义变量,容易造成 shadow var。原来的实现挺好的。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里是为了在出现调用的exception时候,返回result要设置error,要不然没办法返回RPCResult的调用错误信息了。有没有其他方法可以在defer的recover过程中修改返回的结果呢。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

改成了在匿名函数里面用defer来获取异常,cc

@sheny1xuan
Copy link
Contributor Author

proxy 有default和PassThrough 都需要考虑一下,是不是将defer放在DefaultProxyImplementFunc中的makeDubboCallProxy里更好一些

这个makeDubboCallProxy好像是客户端用的,服务端应该直接调用的是Invoke,我在两个proxy里面都加了异常处理,cc

retErr = returnValues[1].Interface()
func() {
// Handle invoke exception in user func.
defer func() {
Copy link
Member

Choose a reason for hiding this comment

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

和default 是一样的defer func, 提取出一个共用的func吧

@AlexStocks AlexStocks merged commit dba1d8e into apache:3.0 May 14, 2022
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.

5 participants