Update to connectrpc.com/connect v1.14.0 and google.golang.org/protobuf v1.32.0 #111
Conversation
nicksnyder
changed the title
nicksnyder
force-pushed
the
upgrade-connect
branch
from
1f52247 to
1de4fca
Compare
|
@emcfarlane tests are failing, can you take a look? |
For GET request Content-Type is not required and is now rejected by connect.
With the new Send operation a bytes.Buffer will use the WriteTo method to write the response body. On a zero length payload Write will not be called. Make the body handling more robust by calling Write(nil) to trigger a body write.
Fallback to allow context cancel errors if the server errors.
|
@nicksnyder working. 3 different failure cases that I've corrected, left details in the commits. Will need review. |
|
Thanks! Will let @jhump review the substantive changes when he is back |
| @@ -1249,6 +1249,7 @@ func (w *responseWriter) close() { | |||
| w.WriteHeader(http.StatusOK) | |||
| } | |||
| if w.w != nil { | |||
| _, _ = w.w.Write(nil) // trigger any final writes | |||
There was a problem hiding this comment.
@emcfarlane, this makes me a little nervous. This smells like we inadvertently introduced a backwards-incompatible change in connect-go. Maybe you can elaborate a little on "the new Send operation", and why this change was really needed, and why that's not a bug in connect-go?
There was a problem hiding this comment.
Yep sure. The bytes.Buffer WriteTo won't Write to the io.Writer if the payload size is zero: https://cs.opensource.google/go/go/+/refs/tags/go1.21.5:src/bytes/buffer.go;l=260
Vanguard-go uses the Write to trigger the conversion of an empty protomessage (valid zero length payload) to a json message ({}). I do think this is an issue with how vanguard-go triggers conversions. It shouldn't be dependant on the write mechanism. This was the minimal change I could see to fix the issue but I would presume we wish to revisit later.
| } | ||
| } | ||
| default: | ||
| } else if serverInvoked { |
There was a problem hiding this comment.
Why all of these changes? The description on the individual commits don't really provide context. Was this necessary to fix something that changed in connect-go? We've actually relaxed the semantics now and allow a cancellation error on the server for all circumstances, when it was previously gated on expectServerCancel. That doesn't seem right.
There was a problem hiding this comment.
Yep this changed as context error was being set. I thought it was related to connectrpc/connect-go#643 but might have been too hastily in removing it for all cases. Here are the relevant test logs:
--- FAIL: TestTranscoder_BufferTooLargeFails/transforming_writer/must_buffer_response_stream/per_svc_options (0.00s)
vanguard_test.go:387: WrapStreamingHandler TestTranscoder_BufferTooLargeFails/transforming_writer/must_buffer_response_stream/per_svc_options
transcoder_test.go:534: RPC error: resource_exhausted: max buffer size (1024) exceeded
transcoder_test.go:534:
Error Trace: /Users/edward/src/github.com/connectrpc/vanguard-go/vanguard_test.go:932
/Users/edward/src/github.com/connectrpc/vanguard-go/transcoder_test.go:534
Error: Not equal:
expected: 0x8
actual : 0x1
Test: TestTranscoder_BufferTooLargeFails/transforming_writer/must_buffer_response_stream/per_svc_options
transcoder_test.go:534: resource_exhausted: buffer limit exceeded
transcoder_test.go:534: canceled: context canceled
--- FAIL: TestTranscoder_BufferTooLargeFails/transforming_writer/must_buffer_response_stream/default_svc_opts (0.00s)
vanguard_test.go:387: WrapStreamingHandler TestTranscoder_BufferTooLargeFails/transforming_writer/must_buffer_response_stream/default_svc_opts
transcoder_test.go:534: RPC error: resource_exhausted: max buffer size (1024) exceeded
transcoder_test.go:534:
Error Trace: /Users/edward/src/github.com/connectrpc/vanguard-go/vanguard_test.go:932
/Users/edward/src/github.com/connectrpc/vanguard-go/transcoder_test.go:534
Error: Not equal:
expected: 0x8
actual : 0x1
Test: TestTranscoder_BufferTooLargeFails/transforming_writer/must_buffer_response_stream/default_svc_opts
I've pushed up a change to add back expectServerCancel. Adding it to the out message passes this test. Does that look correct? It does look like the error should be a resource error though.
No description provided.