Alanxtl commented on code in PR #3064:
URL: https://github.com/apache/dubbo-go/pull/3064#discussion_r2469417558
##########
protocol/triple/triple_protocol/handler_compat.go:
##########
@@ -25,6 +25,8 @@ import (
)
import (
+ "github.com/dubbogo/gost/log/logger"
Review Comment:
use dubbogo's logger, do not use gost's logger barely
```suggestion
"dubbo.apache.org/dubbo-go/v3/common/logger"
```
##########
protocol/triple/triple_protocol/handler_compat.go:
##########
@@ -64,13 +66,16 @@ func (t *tripleCompatInterceptor)
compatUnaryServerInterceptor(ctx context.Conte
}
dubbo3RespRaw, err := handler(ctx, typed.Any())
if dubbo3RespRaw == nil && err == nil {
- // This is going to panic during serialization.
Debugging is much easier
- // if we panic here instead, so we can include the
procedure name.
- panic(fmt.Sprintf("%s returned nil resp and nil error",
t.procedure)) //nolint: forbidigo
+ // Returning nil response and nil error is invalid.
+ // Previously we panicked here for easier debugging,
+ // now we log and return an Internal error instead.
+ logger.Errorf("%s returned nil resp and nil error",
t.procedure)
+ return nil, errorf(CodeInternal, "%s returned nil resp
and nil error", t.procedure)
Review Comment:
```suggestion
logger.Errorf("Procedure %s unexpectedly returned both
nil response and nil error, which should not happen", t.procedure)
return nil, errorf(CodeInternal, "Procedure %s
unexpectedly returned both nil response and nil error, which should not
happen", t.procedure)
```
##########
protocol/triple/triple_protocol/handler_compat.go:
##########
@@ -64,13 +66,16 @@ func (t *tripleCompatInterceptor)
compatUnaryServerInterceptor(ctx context.Conte
}
dubbo3RespRaw, err := handler(ctx, typed.Any())
if dubbo3RespRaw == nil && err == nil {
- // This is going to panic during serialization.
Debugging is much easier
- // if we panic here instead, so we can include the
procedure name.
- panic(fmt.Sprintf("%s returned nil resp and nil error",
t.procedure)) //nolint: forbidigo
+ // Returning nil response and nil error is invalid.
+ // Previously we panicked here for easier debugging,
+ // now we log and return an Internal error instead.
Review Comment:
just delete the comments
```suggestion
```
##########
protocol/triple/triple_protocol/handler_compat.go:
##########
@@ -64,13 +66,16 @@ func (t *tripleCompatInterceptor)
compatUnaryServerInterceptor(ctx context.Conte
}
dubbo3RespRaw, err := handler(ctx, typed.Any())
if dubbo3RespRaw == nil && err == nil {
- // This is going to panic during serialization.
Debugging is much easier
- // if we panic here instead, so we can include the
procedure name.
- panic(fmt.Sprintf("%s returned nil resp and nil error",
t.procedure)) //nolint: forbidigo
+ // Returning nil response and nil error is invalid.
+ // Previously we panicked here for easier debugging,
+ // now we log and return an Internal error instead.
+ logger.Errorf("%s returned nil resp and nil error",
t.procedure)
+ return nil, errorf(CodeInternal, "%s returned nil resp
and nil error", t.procedure)
}
dubbo3Resp, ok := dubbo3RespRaw.(*dubbo_protocol.RPCResult)
if !ok {
- panic(fmt.Sprintf("%+v is not of type *RPCResult",
dubbo3RespRaw))
+ logger.Errorf("%s returned non-RPCResult type %T",
t.procedure, dubbo3RespRaw)
+ return nil, errorf(CodeInternal, "%s returned
non-RPCResult type %T", t.procedure, dubbo3RespRaw)
Review Comment:
```suggestion
logger.Errorf("Procedure %s returned an unexpected
response type. Expected *dubbo_protocol.RPCResult, but got %T", t.procedure,
dubbo3RespRaw)
return nil, errorf(CodeInternal, "Procedure %s returned
an unexpected response type. Expected *dubbo_protocol.RPCResult, but got %T",
t.procedure, dubbo3RespRaw)
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
