[WIP] feat(gRPC): optimize gRPC error handling #1549

DMwangnima · 2024-09-13T09:00:10Z

What type of PR is this?

feat

Check the PR title.

This PR title match the format: <type>(optional scope): <description>
The description of this PR title is user-oriented and clear enough for others to understand.
Attach the PR updating the user documentation if the current PR requires user awareness at the usage level. User docs repo

(Optional) Translate the PR title into Chinese.

(Optional) More detailed description for this PR(en: English/zh: Chinese).

en:

Optimising the gRPC client side call to streaming.Send to return ```the stream is done`` now allows you to get the real reason why the stream is being shut down
Optimise the error returned when a connection is closed on the gRPC client/server side by uniformly converting ConnectionError to code = 14, msg = ConnectionError.Desc status
Optimise the error returned by gRPC server-side calls to streaming.Recv/Send, so that when the stream is closed we can get the real reason instead of "context is canceled".
For the streaming call string scenario A -> B -> C, an error between A and B causes the ctx to be cancelled, making the call between B and C go wrong. Add ‘[triggered by handler side]’, '[triggered by remote service]' to the error string to better alert the user.
Add log in the scenario where an RstStream Frame is sent under an exception on the gRPC server side.

zh(optional):

kerrors 定义用户可以感知到的 Streaming Error
gerrors 定义 gRPC 相关的协议错误，统一继承 kerrors.ErrStreamingProtocol
gRPC 支持注册 HTTP2 ErrCode 与 kerrors/gerrors 错误之间的映射，让 RstStream Frame 也能传递更为详细的信息
gRPC Status Error 统一携带 kerrors/gerrors 中的错误，用于 errors.Is 感知与内部错误码扩展
优化 gRPC client 侧调用 streaming.Send 返回the stream is done，现在可以拿到流被关闭的真正原因
优化 gRPC client/server 侧连接被关闭时返回的错误，统一将ConnectionError转化为code = 14, msg = ConnectionError.Desc的 status
优化 gRPC server 侧调用 streaming.Recv/Send 返回的错误，当流被关闭时可以获取到真正的原因而不是"context is canceled"
对于流式调用串联场景 A -> B -> C，A B 间的错误让 ctx 被 cancel，使得 B C 间的调用出错。给错误字符串加上 "[triggered by handler side]"、"[triggered by remote service]" 来更好地提示用户
为 gRPC 异常情况下发送/接收 RstStream Frame 的场景加上日志

(Optional) Which issue(s) this PR fixes:

(optional) The PR that updates user documentation:

xiaost · 2024-09-16T09:35:51Z

pkg/remote/trans/nphttp2/status/status.go

 // Error wraps a pointer of a status proto. It implements error and Status,
 // and a nil *Error should never be returned by this package.
 type Error struct {
-	e *spb.Status
+	e                   *spb.Status
+	triggeredByUpstream bool


这个有点丑，我们要不要通过 code 编码来区别呀？另外streaming里没有什么upstream的概念。。。代码里可以都叫 remote err

更好的办法确实是 code 编码，但是对于 gRPC-python 之类的服务可能不认。
叫 upstream 主要是因为 A -> B -> C，A B 出错导致 B C 调用失败，不提示 upstream 的话很容易以为是 B C 间的问题。。

移除了，现在 http2Server 流/连接被关闭抛出的错误都会带上"[triggered by {remote service}]"后缀

pkg/remote/trans/nphttp2/grpc/transport_test.go

xiaost · 2024-09-20T08:25:02Z

pkg/remote/trans/nphttp2/grpc/controlbuf.go

@@ -451,19 +451,23 @@ func (c *controlBuffer) get(block bool) (interface{}, error) {
 		select {
 		case <-c.ch:
 		case <-c.done:


done是外部传入的，如果close了的吧，而c.err没数据，会导致返回 nil

c.finish(errStatusConnClosing) 已经注入了 err 了(在 c.err 没数据的情况下)

finish是不是直接return err呀，这样外面不用老是加锁再取一次。

xiaost · 2024-09-20T08:30:00Z

pkg/remote/trans/nphttp2/grpc/error_prompt.go

+	sendRSTStreamFrameSuffix = "[send RST Stream Frame]"
+)
+
+func remoteErrMsg(msg string) string {


这些方法实际上也没有多大的意义。。。感觉不如直接写到klog.CtxErrorf，一般用于日志的话，不要把格式化放在外层做。因为如果关闭了日志，这些逻辑都不应该被执行。

xiaost · 2024-09-20T08:31:19Z

pkg/remote/trans/nphttp2/grpc/http2_server.go

 		t.controlBuf.put(&cleanupStream{
 			streamID: streamID,
 			rst:      true,
 			rstCode:  http2.ErrCodeRefusedStream,
 			onWrite:  func() {},
 		})
-		s.cancel(errMaxStreamsExceeded)


这个变更是为啥？

xiaost · 2024-09-20T08:33:10Z

pkg/remote/trans/nphttp2/grpc/transport.go

+	return errStatusStreamDone
+}
+
+func (s *Stream) SetSourceService(svc string) {


这是公开的方法，不应该这样直接放出来。

xiaost · 2024-09-20T08:34:03Z

pkg/remote/trans/nphttp2/server_handler.go

@@ -185,6 +185,7 @@ func (t *svrTransHandler) handleFunc(s *grpcTransport.Stream, svrTrans *SvrTrans
 			return
 		}
 	}
+	s.SetSourceService(ri.From().ServiceName())


还是用一个更通用的方式去存储这类数据吧，不然后面要加一堆的公开接口。

DMwangnima · 2024-11-14T02:10:11Z

pkg/streamx/provider/grpc/gerrors/gerrors.go

@@ -0,0 +1,62 @@
+/*


外部统一使用 streamx 下暴露的定义

DMwangnima · 2024-11-14T02:11:36Z

pkg/remote/trans/nphttp2/status/status.go

 // Error wraps a pointer of a status proto. It implements error and Status,
 // and a nil *Error should never be returned by this package.
 type Error struct {
 	e *spb.Status
+	// kerr is the Kitex custom error that status maps to
+	kerr error


携带 kerrors 和 gerrors 中定义的错误，用于 errors.Is 错误处理与内部错误码上报

这个预计会怎么用？ fmt.Errorf %w + errors.As 的方式不就够了么，为什么还要封装？

DMwangnima · 2024-11-14T02:15:31Z

pkg/remote/trans/nphttp2/grpc/http_util.go

+// e.g. RegisterCustomRstCode(1000, kerrors.ErrGracefulShutdown)
+// When Kitex receives a RST_STREAM frame with error code 1000, it will inject kerrors.ErrGracefulShutdown into
+// *status.Error that users will conceive
+func RegisterCustomRstCode(rstCode uint32, mappingErr error) {


注册 HTTP2 ErrCode 与 kerrors/gerrors 错误之间的映射，使得 RstStream Frame 可以传递信息

xiaost · 2024-11-14T03:21:13Z

pkg/remote/trans/nphttp2/grpc/transport.go

+)
+
+// SetServiceMeta is used to inject service-related metadata
+func (s *Stream) SetServiceMeta(key svcMetaKey, val interface{}) error {


这个方法到底是对内还是对外的？对内的话，不用暴露出来，对外的话，interface里也没有，保证不了用户套了一层，我们用不了。另外key这类，不要用string

这个方法是对内的，但是不暴露的话，没法在 nphttp2/server_handler.go 中直接使用。
用户即使套了也没有意义，执行 SetServiceMeta 的时候用户还没法感知 Stream

xiaost · 2024-11-14T03:23:11Z

pkg/remote/trans/nphttp2/status/status.go

@@ -158,29 +160,54 @@ func (s *Status) Details() []interface{} {
 	return details
 }

+// WithMappingErr creates a new Status and injects Kitex mapping err
+func (s *Status) WithMappingErr(kerr error) *Status {


这个预计会怎么用？ fmt.Errorf %w + errors.As 的方式不就够了么，为什么还要封装？

xiaost · 2024-11-14T05:01:20Z

pkg/kerrors/streaming_errors.go

+)
+
+// IsStreamingError reports whether the given err is a streaming err
+func IsStreamingError(err error) bool {


这个如果在热路径上判断会比较低效，会随着错误变多，每个都要判断一次。加个 streamingError 类型？

xiaost · 2024-11-14T05:05:54Z

pkg/remote/trans/nphttp2/errors/errors.go

+// This package contains all the errors suitable for Kitex errors model.
+// These errors should not be used by user directly.
+// If users need to perceive these errors, the pkg/streamx/provider/grpc/gerrors package should be used.
+var (


如果用户不需要用的话，就不用定义到这里。哪里返回的话，定义到哪里。反正也不会拿来做逻辑，只是判断、日志，不需要export

内部需要拿到这个类型获取相应错误码，如果不暴露的话，得修改 *status.Error 的 Code() 方法返回值，或者加个 TypeID() 方法，内部错误码定义放在开源库里。

xiaost · 2024-11-14T05:08:58Z

pkg/remote/trans/nphttp2/errors/errors.go

+	basic error
+}
+
+func newErrType(message string) *errType {


这个实现成 fmt.Errorf("%w - %s", kerrors.ErrStreamingProtocol, message) 好像跟你的一样。

主要是考虑有可能会对 errType 做扩展，用 fmt.Errorf 确实最快

pkg/remote/trans/nphttp2/grpc/http_util.go

DMwangnima requested review from a team as code owners September 13, 2024 09:00

DMwangnima force-pushed the feat/new_grpc_error_handling branch 2 times, most recently from 20064bb to ed1b013 Compare September 16, 2024 06:26

DMwangnima changed the title ~~feat: optimize gRPC server side error handling~~ feat: optimize gRPC error handling Sep 16, 2024

DMwangnima force-pushed the feat/new_grpc_error_handling branch 3 times, most recently from 012de39 to 8683021 Compare September 16, 2024 08:20

xiaost reviewed Sep 16, 2024

View reviewed changes

pkg/remote/trans/nphttp2/grpc/transport_test.go Outdated Show resolved Hide resolved

DMwangnima force-pushed the feat/new_grpc_error_handling branch 4 times, most recently from 62e6e92 to ba48fa1 Compare September 18, 2024 13:22

DMwangnima changed the title ~~feat: optimize gRPC error handling~~ feat(gRPC): optimize gRPC error handling Sep 19, 2024

DMwangnima force-pushed the feat/new_grpc_error_handling branch 2 times, most recently from 3087213 to 94687b7 Compare September 20, 2024 08:17

xiaost reviewed Sep 20, 2024

View reviewed changes

DMwangnima force-pushed the feat/new_grpc_error_handling branch 3 times, most recently from 7688c77 to 67c9a2d Compare September 27, 2024 01:47

DMwangnima force-pushed the feat/new_grpc_error_handling branch from 36b87c4 to a7622dc Compare October 22, 2024 12:32

DMwangnima force-pushed the feat/new_grpc_error_handling branch 3 times, most recently from 963f2d1 to c7bb7cd Compare November 11, 2024 03:08

DMwangnima force-pushed the feat/new_grpc_error_handling branch 7 times, most recently from 9bd9abe to 95da68d Compare November 14, 2024 01:42

DMwangnima commented Nov 14, 2024

View reviewed changes

DMwangnima force-pushed the feat/new_grpc_error_handling branch from 95da68d to 0f7b799 Compare November 14, 2024 03:16

xiaost reviewed Nov 14, 2024

View reviewed changes

DMwangnima force-pushed the feat/new_grpc_error_handling branch 2 times, most recently from be89793 to 3cc884a Compare November 15, 2024 08:17

joway previously approved these changes Nov 19, 2024

View reviewed changes

pkg/remote/trans/nphttp2/grpc/http_util.go Outdated Show resolved Hide resolved

DMwangnima dismissed joway’s stale review via e153228 November 21, 2024 02:57

joway previously approved these changes Nov 21, 2024

View reviewed changes

DMwangnima added 2 commits November 22, 2024 16:02

feat: optimize gRPC error handling

1f6b631

rebase and resolve conflict

a9dee0c

DMwangnima dismissed joway’s stale review via a9dee0c November 22, 2024 09:40

DMwangnima force-pushed the feat/new_grpc_error_handling branch from 9d514fa to a9dee0c Compare November 22, 2024 09:40

DMwangnima added 4 commits November 22, 2024 17:44

gofumpt format

3d80c26

add recvDownStream graceful shutdown

5cd3a14

remove special status code for graceful shutdown

001a3d9

better log format

26b653b

DMwangnima changed the title ~~feat(gRPC): optimize gRPC error handling~~ [WIP] feat(gRPC): optimize gRPC error handling Nov 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] feat(gRPC): optimize gRPC error handling #1549

[WIP] feat(gRPC): optimize gRPC error handling #1549

DMwangnima commented Sep 13, 2024 •

edited

Loading

xiaost Sep 16, 2024 •

edited

Loading

DMwangnima Sep 16, 2024

DMwangnima Sep 17, 2024 •

edited

Loading

xiaost Sep 20, 2024

DMwangnima Sep 20, 2024

xiaost Nov 14, 2024

DMwangnima Nov 21, 2024

xiaost Sep 20, 2024

xiaost Sep 20, 2024

xiaost Sep 20, 2024

xiaost Sep 20, 2024

DMwangnima Nov 14, 2024 •

edited

Loading

DMwangnima Nov 14, 2024

xiaost Nov 14, 2024

DMwangnima Nov 14, 2024

xiaost Nov 14, 2024

DMwangnima Nov 14, 2024

xiaost Nov 14, 2024

xiaost Nov 14, 2024

DMwangnima Nov 21, 2024

xiaost Nov 14, 2024

DMwangnima Nov 14, 2024

xiaost Nov 14, 2024 •

edited

Loading

DMwangnima Nov 14, 2024

[WIP] feat(gRPC): optimize gRPC error handling #1549

Are you sure you want to change the base?

[WIP] feat(gRPC): optimize gRPC error handling #1549

Conversation

DMwangnima commented Sep 13, 2024 • edited Loading

What type of PR is this?

Check the PR title.

(Optional) Translate the PR title into Chinese.

(Optional) More detailed description for this PR(en: English/zh: Chinese).

(Optional) Which issue(s) this PR fixes:

(optional) The PR that updates user documentation:

xiaost Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DMwangnima Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DMwangnima Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaost Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DMwangnima commented Sep 13, 2024 •

edited

Loading

xiaost Sep 16, 2024 •

edited

Loading

DMwangnima Sep 17, 2024 •

edited

Loading

DMwangnima Nov 14, 2024 •

edited

Loading

xiaost Nov 14, 2024 •

edited

Loading