Skip to content

Commit c8be9e7

Browse files
authored
Connect unary should return unavailable instead of unimplemented for io.EOF errors from the transport (#776)
This adds a test that can reproduce the issue where unary RPCs would return an `unimplemented` code when an `unavailable` code was more appropriate. Basically, it was mistakenly interpreting network EOF errors as "the server did not send us any response messages which is a cardinality violation" instead of "the server connection broke so the server response is unavailable". There are still cases where an `io.EOF` could likely be misinterpreted as a cardinality issue: if it the connection is severed _after_ the headers have been received but _before_ the first byte of the first envelope has been received. But the main case where this issue has been reported was for unary RPCs in the Connect protocol, which does not suffer from that issue, since it does not use envelopes.
1 parent 99d6b9c commit c8be9e7

File tree

3 files changed

+64
-3
lines changed

3 files changed

+64
-3
lines changed

.golangci.yml

-2
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,6 @@ linters-settings:
1919
importas:
2020
no-unaliased: true
2121
alias:
22-
- pkg: connectrpc.com/connect
23-
alias: connect
2422
- pkg: connectrpc.com/connect/internal/gen/connect/ping/v1
2523
alias: pingv1
2624
varnamelen:

client_ext_test.go

+59-1
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ import (
3131
"testing"
3232
"time"
3333

34-
connect "connectrpc.com/connect"
34+
"connectrpc.com/connect"
3535
"connectrpc.com/connect/internal/assert"
3636
pingv1 "connectrpc.com/connect/internal/gen/connect/ping/v1"
3737
"connectrpc.com/connect/internal/gen/connect/ping/v1/pingv1connect"
@@ -227,6 +227,58 @@ func TestGetNoContentHeaders(t *testing.T) {
227227
assert.Equal(t, http.MethodGet, unaryReq.HTTPMethod())
228228
}
229229

230+
func TestConnectionDropped(t *testing.T) {
231+
t.Parallel()
232+
ctx := context.Background()
233+
for _, protocol := range []string{connect.ProtocolConnect, connect.ProtocolGRPC, connect.ProtocolGRPCWeb} {
234+
var opts []connect.ClientOption
235+
switch protocol {
236+
case connect.ProtocolGRPC:
237+
opts = []connect.ClientOption{connect.WithGRPC()}
238+
case connect.ProtocolGRPCWeb:
239+
opts = []connect.ClientOption{connect.WithGRPCWeb()}
240+
}
241+
t.Run(protocol, func(t *testing.T) {
242+
t.Parallel()
243+
httpClient := httpClientFunc(func(_ *http.Request) (*http.Response, error) {
244+
return nil, io.EOF
245+
})
246+
client := pingv1connect.NewPingServiceClient(
247+
httpClient,
248+
"http://1.2.3.4",
249+
opts...,
250+
)
251+
t.Run("unary", func(t *testing.T) {
252+
t.Parallel()
253+
req := connect.NewRequest[pingv1.PingRequest](nil)
254+
_, err := client.Ping(ctx, req)
255+
assert.NotNil(t, err)
256+
if !assert.Equal(t, connect.CodeOf(err), connect.CodeUnavailable) {
257+
t.Logf("err = %v\n%#v", err, err)
258+
}
259+
})
260+
t.Run("stream", func(t *testing.T) {
261+
t.Parallel()
262+
req := connect.NewRequest[pingv1.CountUpRequest](nil)
263+
svrStream, err := client.CountUp(ctx, req)
264+
if err == nil {
265+
t.Cleanup(func() {
266+
assert.Nil(t, svrStream.Close())
267+
})
268+
if !assert.False(t, svrStream.Receive()) {
269+
return
270+
}
271+
err = svrStream.Err()
272+
}
273+
assert.NotNil(t, err)
274+
if !assert.Equal(t, connect.CodeOf(err), connect.CodeUnavailable) {
275+
t.Logf("err = %v\n%#v", err, err)
276+
}
277+
})
278+
})
279+
}
280+
}
281+
230282
func TestSpecSchema(t *testing.T) {
231283
t.Parallel()
232284
mux := http.NewServeMux()
@@ -762,3 +814,9 @@ func addUnrecognizedBytes[M proto.Message](msg M, data []byte) M {
762814
msg.ProtoReflect().SetUnknown(data)
763815
return msg
764816
}
817+
818+
type httpClientFunc func(*http.Request) (*http.Response, error)
819+
820+
func (fn httpClientFunc) Do(req *http.Request) (*http.Response, error) {
821+
return fn(req)
822+
}

duplex_http_call.go

+5
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,11 @@ func (d *duplexHTTPCall) makeRequest() {
308308
// pipe. Write's check for io.ErrClosedPipe and will convert this to io.EOF.
309309
response, err := d.httpClient.Do(d.request) //nolint:bodyclose
310310
if err != nil {
311+
if errors.Is(err, io.EOF) {
312+
// We use io.EOF as a sentinel in many places and don't want this
313+
// transport error to be confused for those other situations.
314+
err = io.ErrUnexpectedEOF
315+
}
311316
err = wrapIfContextError(err)
312317
err = wrapIfLikelyH2CNotConfiguredError(d.request, err)
313318
err = wrapIfLikelyWithGRPCNotUsedError(err)

0 commit comments

Comments
 (0)