fix: Handle panics in the rpc server #1330
Conversation
AndrewSisley
added
area/p2p
go.mod
Outdated
| @@ -87,6 +87,7 @@ require ( | |||
| github.com/google/gopacket v1.1.19 // indirect | |||
| github.com/google/pprof v0.0.0-20221203041831-ce31453925ec // indirect | |||
| github.com/google/uuid v1.3.0 // indirect | |||
| github.com/grpc-ecosystem/go-grpc-middleware v1.4.0 // indirect | |||
There was a problem hiding this comment.
todo: Don't think this should be indirect. Try running make tidy.
There was a problem hiding this comment.
good spot - thanks Shahzad - I think I'm guilty of skimming over this file :)
- make tidy
There was a problem hiding this comment.
You're right Shahzad. This probably needs to change.
Codecov Report
@@ Coverage Diff @@
## develop #1330 +/- ##
===========================================
+ Coverage 70.52% 70.60% +0.08%
===========================================
Files 184 184
Lines 17792 17801 +9
===========================================
+ Hits 12547 12568 +21
+ Misses 4286 4278 -8
+ Partials 959 955 -4
|
AndrewSisley
force-pushed
the
sisley/fix/I1298-handle-rpc-panics
branch
from
1855f92
to
9b30b7a
Compare
| MaxConnectionIdle: rpcTimeoutDuration, | ||
| })) | ||
| server := grpc.NewServer( | ||
| grpc.UnaryInterceptor( |
There was a problem hiding this comment.
question: What was the reason for Unary over Stream?
There was a problem hiding this comment.
I believe it would be because we don't use rpc streaming.
There was a problem hiding this comment.
was easy to test that - I had no idea which one we used and added the streaming one first - to no effect on the CLI. If we end up using it at some point we can add in the middleware for it then IMO
There was a problem hiding this comment.
Thanks for the explanation :)
| @@ -20,6 +20,8 @@ import ( | |||
| "strings" | |||
|
|
|||
| badger "github.com/dgraph-io/badger/v3" | |||
| grpc_middleware "github.com/grpc-ecosystem/go-grpc-middleware" | |||
There was a problem hiding this comment.
todo: If you don't mind, there is v2 that is now available. We should probably use that instead.
There was a problem hiding this comment.
V2 is not released yet (pre-release build only)
| MaxConnectionIdle: rpcTimeoutDuration, | ||
| })) | ||
| server := grpc.NewServer( | ||
| grpc.UnaryInterceptor( |
There was a problem hiding this comment.
I believe it would be because we don't use rpc streaming.
AndrewSisley
dismissed
fredcarle’s stale review
requested change is not possible, and I'd like to merge this and another PR before logging off today before the release
Relevant issue(s)
Resolves #1298
Description
Adds middleware to handle panics in the rpc server.
Currently panics via the server will result in the full defradb instance crashing, this PR adds middleware that converts the panics into errors.
Tested by hacking in a panic and calling into the server via the CLI to see if it results in an error or a panic/crash.