Reproduce issue with client error handling + fix #799
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| // Copyright 2021-2024 The Connect Authors | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package connect | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "net/http" | ||
| "testing" | ||
|
|
||
| "connectrpc.com/connect/internal/assert" | ||
| pingv1 "connectrpc.com/connect/internal/gen/connect/ping/v1" | ||
| "connectrpc.com/connect/internal/memhttp/memhttptest" | ||
| ) | ||
|
|
||
| var ErrOKToIgnore = errors.New("some error that is ok to ignore") | ||
|
|
||
| func TestClientErrorHandling(t *testing.T) { | ||
| t.Parallel() | ||
| mux := http.NewServeMux() | ||
| mux.Handle("/connect.ping.v1.PingService/Ping", NewUnaryHandler( | ||
| "/connect.ping.v1.PingService/Ping", | ||
| func(ctx context.Context, r *Request[pingv1.PingRequest]) (*Response[pingv1.PingResponse], error) { | ||
| return nil, NewError(CodeCanceled, ErrOKToIgnore) | ||
| }, | ||
| )) | ||
| server := memhttptest.NewServer(t, mux) | ||
|
|
||
| client := NewClient[pingv1.PingRequest, pingv1.PingResponse]( | ||
| server.Client(), | ||
| server.URL()+"/connect.ping.v1.PingService/Ping", | ||
| WithInterceptors( | ||
| errorHidingInterceptor{}, | ||
| ), | ||
| ) | ||
| ctx := context.Background() | ||
|
|
||
| _, err := client.CallUnary(ctx, NewRequest[pingv1.PingRequest](nil)) | ||
| assert.Nil(t, err) | ||
| } | ||
|
|
||
| // errorHidingInterceptor is a simple interceptor that hides errors based on | ||
| // some criteria (in this case, if the error is a CodeCanceled error). It is | ||
| // used to reproduce an issue with error handling in the client, where the | ||
| // type information is lost between unaryFunc and client.callUnary. | ||
| type errorHidingInterceptor struct{} | ||
|
|
||
| func (e errorHidingInterceptor) WrapStreamingClient(StreamingClientFunc) StreamingClientFunc { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| func (e errorHidingInterceptor) WrapStreamingHandler(StreamingHandlerFunc) StreamingHandlerFunc { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| func (e errorHidingInterceptor) WrapUnary(next UnaryFunc) UnaryFunc { | ||
| return func(ctx context.Context, req AnyRequest) (_ AnyResponse, retErr error) { | ||
| res, err := next(ctx, req) | ||
| if CodeOf(err) == CodeCanceled { // some criteria for ignored errors | ||
| return res, nil | ||
|
There was a problem hiding this comment. This causes the interceptor to return This could be done dynamically by inspecting We could also potentially have connect-go's internal implementation of if respTyped, ok := req.(interface {
// Could instantiate the correct response type and then run
// any configured initializer.
NewResponse() any
}); ok {
return respTyped.NewResponse(), nil
}But, now that I've suggested it, I don't love that. It is much less intuitive of course. But it also just feels like a terrible pattern to support -- always returning a zero value response seems wrong and is almost certain to violate any expected RPC contract about what the method is supposed to return. This just doesn't seem like an appropriate function for an interceptor. It would be much better to inject this error handling much deeper -- like into the actual response handlers, so they can return a valid response value if the application wants to ignore a particular error. Another possibility would be to use a switch on the schema type or the actual procedure name so that it always returns a valid response (which may not necessarily be empty). There was a problem hiding this comment. The client interceptor returning an actual, non-nil response value is reasonable. However, it also doesn't seem right that interceptors lose all information about the expected response in an error condition. I don't think smuggling the type via AnyRequest would work either -- there's no reason (other than convention, I suppose) that the same request type isn't used by multiple service methods returning different response types. Short of smuggling the type information through the Error struct, another option is that // Any returns the concrete response message as an empty interface, so that
// *Response implements the [AnyResponse] interface. If the response message is
// nil, Any returns a new zero value of the message type.
func (r *Response[T]) Any() any {
if r.Msg == nil {
return new(T)
}
return r.Msg
}But this means the framework would need to ensure that resp is never nil before being passed to the next interceptor. |
||
| } | ||
| return res, err | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right. A
nil, nilresponse isn't really valid. What is the framework expected to do with this when returning a value to the caller?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #799 (comment)