RSDK-9316 Prefer ErrDisconnected over closed pipe in client code #4571
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
| @@ -80,12 +80,12 @@ require ( | |||
| go.uber.org/zap v1.27.0 | |||
| go.viam.com/api v0.1.357 | |||
| go.viam.com/test v1.2.3 | |||
| go.viam.com/utils v0.1.112 | |||
| go.viam.com/utils v0.1.116 | |||
There was a problem hiding this comment.
This quadruple bump could be trouble? Certainly comes with other direct and indirect transient bumps.
| @@ -99,6 +99,11 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
| h.ServeHTTP(w, r) | |||
| } | |||
|
|
|||
| // Stats is unsupported. | |||
| func (s *Server) Stats() any { | |||
There was a problem hiding this comment.
Same fix from #4566 @dgottlieb.
| @@ -130,13 +128,6 @@ func skipConnectionCheck(method string) bool { | |||
| return exemptFromConnectionCheck[method] | |||
| } | |||
|
|
|||
| func isClosedPipeError(err error) bool { | |||
There was a problem hiding this comment.
Spoke with @dgottlieb a bit offline. We were relying on io.ErrClosedPipe to tell us if a client was disconnected in any of our gRPC calls within robot/client/client.go. We now explicitly rely on rpc.ErrDisconnected, which should be returned any time a gRPC call tries to open a new stream on a closed channel (we send a gRPC request over a closed connection.)
| @@ -161,7 +152,7 @@ func (rc *RobotClient) handleUnaryDisconnect( | |||
| err := invoker(ctx, method, req, reply, cc, opts...) | |||
| // we might lose connection before our background check detects it - in this case we | |||
| // should still surface a helpful error message. | |||
| if isClosedPipeError(err) { | |||
There was a problem hiding this comment.
These "if closed pipe" conditionals were a little abstract before. Can we tighten up the documentation here? I'd love if we could list out exactly which errors bubble up here (obvious ErrDisconnected is one). And how this conditional changes the behavior in a desirable way.
It's hard to claim "surface a helpful error message" is an improvement over some unstated error message.
There was a problem hiding this comment.
Spoke a bit with @dgottlieb offline. I did some sleuthing into what errors can bubble up to this point; I do not think we're catching them all. Left a TODO with a reference to https://viam.atlassian.net/browse/RSDK-9333 and a NOTE explaining my findings/thoughts.
For now, I think we should capture both rpc.ErrDisconnected and io.ErrClosedPipe as possible disconnect manifestations, so we are not less sensitive than before. Per RSDK-9333, we should likely capture more than that and transform the errors into something more informative than an Unavailable gRPC error.
The test that was failing in a previous CI run was indeed "injecting" an io.ErrClosedPipe and expecting us to transform it into a gRPC error with code Unavailable. Left that test as-is.
viambot
added
safe to test
benjirewis
force-pushed
the
handle-err-disconnected
branch
from
969a383
to
1b5f485
Compare
viambot
added
safe to test
Sensitivity to changes from viamrobotics/goutils#394.