You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by GitBox <gi...@apache.org> on 2022/04/26 12:45:46 UTC
[GitHub] [dubbo-go] sheny1xuan opened a new pull request, #1866: fix: complete grpc based protocol panic recover handle.
sheny1xuan opened a new pull request, #1866:
URL: https://github.com/apache/dubbo-go/pull/1866
Signed-off-by: stonex <14...@qq.com>
<!-- Thanks for sending a pull request!
Read https://github.com/apache/dubbo-go/blob/master/CONTRIBUTING.md before commit pull request.
-->
**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](https://github.com/dubbogo/tools#5-how-to-get-imports-formatter) to run 'imports-formatter .' in project root, to format your import blocks, mentioned in [CONTRIBUTING.md](https://github.com/apache/dubbo-go/blob/master/CONTRIBUTING.md) above)
- [ ] Your new-created file needs to have [apache license](https://raw.githubusercontent.com/dubbogo/resources/master/tools/license/license.txt) 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](https://github.com/apache/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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] sanxun0325 commented on a diff in pull request #1866: fix: complete grpc based protocol panic recover handle.
Posted by GitBox <gi...@apache.org>.
sanxun0325 commented on code in PR #1866:
URL: https://github.com/apache/dubbo-go/pull/1866#discussion_r870872248
##########
common/proxy/proxy_factory/pass_through.go:
##########
@@ -106,11 +108,28 @@ func (pi *PassThroughProxyInvoker) Invoke(ctx context.Context, invocation protoc
in = append(in, reflect.ValueOf(args))
in = append(in, reflect.ValueOf(invocation.Attachments()))
- returnValues := method.Method().Func.Call(in)
-
+ var replyv reflect.Value
var retErr interface{}
- replyv := returnValues[0]
- retErr = returnValues[1].Interface()
+ func() {
+ // Handle invoke exception in user func.
+ defer func() {
Review Comment:
和default 是一样的defer func, 提取出一个共用的func吧
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] AlexStocks merged pull request #1866: fix: complete grpc based protocol panic recover handle.
Posted by GitBox <gi...@apache.org>.
AlexStocks merged PR #1866:
URL: https://github.com/apache/dubbo-go/pull/1866
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] AlexStocks commented on a diff in pull request #1866: fix: complete grpc based protocol panic recover handle.
Posted by GitBox <gi...@apache.org>.
AlexStocks commented on code in PR #1866:
URL: https://github.com/apache/dubbo-go/pull/1866#discussion_r867472025
##########
common/proxy/proxy_factory/default.go:
##########
@@ -84,8 +85,8 @@ type ProxyInvoker struct {
}
// Invoke is used to call service method by invocation
-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{}
Review Comment:
不要在返回值处定义变量，容易造成 shadow var。原来的实现挺好的。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] sheny1xuan commented on a diff in pull request #1866: fix: complete grpc based protocol panic recover handle.
Posted by GitBox <gi...@apache.org>.
sheny1xuan commented on code in PR #1866:
URL: https://github.com/apache/dubbo-go/pull/1866#discussion_r867475050
##########
common/proxy/proxy_factory/default.go:
##########
@@ -84,8 +85,8 @@ type ProxyInvoker struct {
}
// Invoke is used to call service method by invocation
-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{}
Review Comment:
这里是为了在出现调用的exception时候，返回result要设置error，要不然没办法返回RPCResult的调用错误信息了。有没有其他方法可以在defer的recover过程中修改返回的结果呢。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] sheny1xuan commented on a diff in pull request #1866: fix: complete grpc based protocol panic recover handle.
Posted by GitBox <gi...@apache.org>.
sheny1xuan commented on code in PR #1866:
URL: https://github.com/apache/dubbo-go/pull/1866#discussion_r870453249
##########
common/proxy/proxy_factory/default.go:
##########
@@ -84,8 +85,8 @@ type ProxyInvoker struct {
}
// Invoke is used to call service method by invocation
-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{}
Review Comment:
改成了在匿名函数里面用defer来获取异常，cc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] codecov-commenter commented on pull request #1866: fix: complete grpc based protocol panic recover handle.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1866:
URL: https://github.com/apache/dubbo-go/pull/1866#issuecomment-1119676528
# [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1866?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1866](https://codecov.io/gh/apache/dubbo-go/pull/1866?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (753ab63) into [3.0](https://codecov.io/gh/apache/dubbo-go/commit/84a131cb4a4e615ff9a4f905d7469971c503f5f5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (84a131c) will **decrease** coverage by `1.90%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## 3.0 #1866 +/- ##
==========================================
- Coverage 46.78% 44.88% -1.91%
==========================================
Files 295 297 +2
Lines 17173 17290 +117
==========================================
- Hits 8034 7760 -274
- Misses 8286 8710 +424
+ Partials 853 820 -33
```
| [Impacted Files](https://codecov.io/gh/apache/dubbo-go/pull/1866?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [common/proxy/proxy\_factory/default.go](https://codecov.io/gh/apache/dubbo-go/pull/1866/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3Byb3h5L3Byb3h5X2ZhY3RvcnkvZGVmYXVsdC5nbw==) | `18.42% <0.00%> (-3.12%)` | :arrow_down: |
| [remoting/getty/listener.go](https://codecov.io/gh/apache/dubbo-go/pull/1866/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVtb3RpbmcvZ2V0dHkvbGlzdGVuZXIuZ28=) | `26.42% <ø> (-0.50%)` | :arrow_down: |
| [registry/nacos/listener.go](https://codecov.io/gh/apache/dubbo-go/pull/1866/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvbmFjb3MvbGlzdGVuZXIuZ28=) | `0.00% <0.00%> (-78.27%)` | :arrow_down: |
| [remoting/nacos/builder.go](https://codecov.io/gh/apache/dubbo-go/pull/1866/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVtb3RpbmcvbmFjb3MvYnVpbGRlci5nbw==) | `0.00% <0.00%> (-73.92%)` | :arrow_down: |
| [registry/etcdv3/registry.go](https://codecov.io/gh/apache/dubbo-go/pull/1866/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvZXRjZHYzL3JlZ2lzdHJ5Lmdv) | `3.07% <0.00%> (-70.77%)` | :arrow_down: |
| [metadata/report/etcd/report.go](https://codecov.io/gh/apache/dubbo-go/pull/1866/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWV0YWRhdGEvcmVwb3J0L2V0Y2QvcmVwb3J0Lmdv) | `1.11% <0.00%> (-61.12%)` | :arrow_down: |
| [registry/etcdv3/listener.go](https://codecov.io/gh/apache/dubbo-go/pull/1866/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvZXRjZHYzL2xpc3RlbmVyLmdv) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [config\_center/nacos/listener.go](https://codecov.io/gh/apache/dubbo-go/pull/1866/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29uZmlnX2NlbnRlci9uYWNvcy9saXN0ZW5lci5nbw==) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [remoting/etcdv3/listener.go](https://codecov.io/gh/apache/dubbo-go/pull/1866/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVtb3RpbmcvZXRjZHYzL2xpc3RlbmVyLmdv) | `3.53% <0.00%> (-43.37%)` | :arrow_down: |
| [config\_center/nacos/impl.go](https://codecov.io/gh/apache/dubbo-go/pull/1866/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29uZmlnX2NlbnRlci9uYWNvcy9pbXBsLmdv) | `40.59% <0.00%> (-34.66%)` | :arrow_down: |
| ... and [10 more](https://codecov.io/gh/apache/dubbo-go/pull/1866/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1866?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1866?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [84a131c...753ab63](https://codecov.io/gh/apache/dubbo-go/pull/1866?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] sheny1xuan commented on pull request #1866: fix: complete grpc based protocol panic recover handle.
Posted by GitBox <gi...@apache.org>.
sheny1xuan commented on PR #1866:
URL: https://github.com/apache/dubbo-go/pull/1866#issuecomment-1123923283
> proxy 有default和PassThrough 都需要考虑一下，是不是将defer放在DefaultProxyImplementFunc中的makeDubboCallProxy里更好一些
这个makeDubboCallProxy好像是客户端用的，服务端应该直接调用的是Invoke，我在两个proxy里面都加了异常处理，cc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org