gRPC server supports PanicHandler
#319
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Supports `PanicHandler` for the gRPC server to allow users to catch and log the panic at the client side. Currently, when the plugin panics, the panic is captured only by the stderr rpc channel, which is then logged by the client side with "DEBUG" log level. In most cases, the panic information should be of the "ERROR" log level by the definition of log level. This `PanicHandler` can help with that. In some other cases, this hook allows users to do other handling when the provider crashes.
robmonte
reviewed
Oct 17, 2024
| @@ -73,6 +78,21 @@ func (s *GRPCServer) Init() error { | |||
| if s.TLS != nil { | |||
| opts = append(opts, grpc.Creds(credentials.NewTLS(s.TLS))) | |||
| } | |||
|
|
|||
| if s.PanicHandler != nil { | |||
There was a problem hiding this comment.
It seems to me like the go-grpc-middleware recovery interceptor still provides the ability to return these panics as errors even if you don't want to provide a recovery handler function, so this nil check may not be necessary if I am understanding this correctly: https://github.com/grpc-ecosystem/go-grpc-middleware/blob/main/interceptors/recovery/interceptors.go#L52
What do you think?
There was a problem hiding this comment.
If we don't do nil check here, and don't provide a PanicHandler, the behavior changes from panicing (the current behavior), to returning an error. Would this be a breaking change?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Supports
PanicHandlerfor the gRPC server to allow users to handle the panic at the plugin side instead of directly down.Currently, when the plugin panics, the panic is captured only by the stderr rpc channel, which is then logged by the client side with "DEBUG" log level. In most cases, the panic information should be of the "ERROR" log level by the definition of log level. This
PanicHandlercan help with that. In some other cases, this hook allows users to do other handling when the provider crashes.