Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reproduce issue with client error handling + fix #799

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Reproduce issue with client error handling + fix #799

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
facbe19
Reproduce issue with client error handling + fix
jamesrom Nov 28, 2024
9bfd649
Alternative fix: keep back compat with interceptor contract
jamesrom Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func NewClient[Req, Res any](httpClient HTTPClient, url string, options ...Clien
response, err := receiveUnaryResponse[Res](conn, config.Initializer)
if err != nil {
_ = conn.CloseResponse()
return nil, err
return response, err
jamesrom marked this conversation as resolved.
Show resolved Hide resolved
}
return response, conn.CloseResponse()
})
Expand Down
76 changes: 76 additions & 0 deletions client_error_handling_test.go
View file
Open in desktop
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
// use 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes the interceptor to return nil, nil. I don't think that should be legal. If an interceptor is "hiding" an error, it must synthesize an actual, non-nil response value.

This could be done dynamically by inspecting req.Spec.Schema(). If that is unset then a dynamic response is not possible.

We could also potentially have connect-go's internal implementation of AnyRequest implement an optional method that could help. For example, maybe something like so:

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).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 resp.Any() could do a nil check:

// 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
}
}