Wrap errors with context cancellation codes #659
Conversation
This PR wraps errors with the appropariate connect code of Cancelled or DeadlineExceeded if the context error is not nil. Improves error handling for some well known error cases that do not surface context.Cancelled errors. For example HTTP2 "client disconnect" string errors are now raised with a Cancelled code not an Unknwon. This lets handlers check the error code for better handling and reporting of errors.
|
Edit: can't reproduce test flakes. Looks to be solid now. |
| func asMaxBytesError(err error, tmpl string, args ...any) *Error { | ||
| // wrapIfMaxBytesError wraps errors returned reading from a http.MaxBytesHandler | ||
| // whose limit has been exceeded. | ||
| func wrapIfMaxBytesError(err error, tmpl string, args ...any) error { |
There was a problem hiding this comment.
Converted wrapIfMaxBytesError to the same style as other error handling for consistency.
envelope.go
Outdated
| @@ -117,6 +118,7 @@ func (e *envelope) Len() int { | |||
| } | |||
|
|
|||
| type envelopeWriter struct { | |||
| ctx context.Context | |||
There was a problem hiding this comment.
Storing the ctx in the envelope reader is a little odd, but would otherwise be needed to be stored on the stream interceptors and then passed down which makes this problem worse. Would be nice to find a better solution though!
|
So what's the status of this PR now? Did you still want to merge it, or is it no longer needed? |
|
@chrispine I think this behaviour is still wanted to fix the issue linked. PR needs review. |
envelope.go
Outdated
| err = wrapIfContextError(err) | ||
| err = wrapWithContextError(w.ctx, err) |
There was a problem hiding this comment.
Perhaps this would be better to push the error classification into sender.Send? The sender (the duplexHTTPCall) has a reference to the request, which should also include the context.
Also, under what conditions would we ever call only one or the other of those context-wrap functions? Feels like they want to be a single function -- maybe even a method on duplexHTTPCall (so it can pull out the request context for the comparisons in the latter functions).
There was a problem hiding this comment.
We call wrapWithContext more liberally to convert context errors but not to wrap errors with context codes. We will always call wrapIfContext with wrapWithContext but still need wrapIfContext separate so left them split out. A method on duplexHTTPCall would need a corresponding method on the handler side. The marshallers are used on both client/server so can handle it uniformly there.
There was a problem hiding this comment.
We will always call
wrapIfContextwithwrapWithContextbut still needwrapIfContextseparate so left them split out.
Why do they need to be split? Under what cases do we care about the error being a context error but if the context is actually done (or vice versa)? Also, the names are rather confusing. Just reading the above sentence is really not clear as to which is which.
There was a problem hiding this comment.
We check for context errors on non IO related errors like the wrapper to the stream handlers. I've merged the call to the wrapIfContextError from wrapIfContextDone.
There was a problem hiding this comment.
https://github.com/emcfarlane/connect-go/blob/c3324072f50f3882a98e4ee846ee7511d3911efa/error.go#L279
Which we don't currently have an easy way to access the context from.
There was a problem hiding this comment.
Let's at least make the names more clear. I think wrapIfContextError is reasonably clear. Maybe the other could be called wrapIfContextDone? Although I'm still not convinced these should be separate - it seems like wherever we're doing one, we'd likely want to do both.
envelope.go
Outdated
| err = wrapIfContextError(err) | ||
| err = wrapWithContextError(w.ctx, err) |
There was a problem hiding this comment.
We will always call
wrapIfContextwithwrapWithContextbut still needwrapIfContextseparate so left them split out.
Why do they need to be split? Under what cases do we care about the error being a context error but if the context is actually done (or vice versa)? Also, the names are rather confusing. Just reading the above sentence is really not clear as to which is which.
This PR wraps errors with the appropriate connect code of Cancelled or DeadlineExceeded if the context error is not nil.
Improves error handling for some well known error cases that do not surface context.Cancelled errors. For example HTTP2 "client disconnect" string errors are now raised with a Cancelled code not an Unknown. This lets handlers check the error code for better handling and reporting of errors.
Fix for #645