connectrpc / connect-go

The Go implementation of Connect: Protobuf RPC that works.
https://connectrpc.com
Apache License 2.0
2.94k stars 96 forks source link

Implement unary HTTP calls with retry support #649

Closed emcfarlane closed 8 months ago

emcfarlane commented 10 months ago

Internal changes to the HTTP client to remove some of the overhead required for streaming calls but unnecessary for non-streaming client requests. This PR changes both of the non-streaming client request types: unary and server streams. These types will now create the client request with the message payload upfront before calling client.Do. This removes the overhead of the io.Pipe to convert the body from a reader to a writer and the extra goroutine required to write to the pipe asynchronously. From this we get improved functionality for non-streaming requests by setting the Content-Length header and implementing GetBody function to enable retries.

Implementation details

The buffer reuse semantics are kept the same for both unary and stream requests. This allows for simple error handling with a w.bufferPool.Get() matched to a defer w.bufferPool.Put(). To enforce this behaviour for the new unary calls we must wait for transport to be done with the request body ensuring no reads occur after a return from Send. Under testing a client.Do will almost always drain the body before returning the response. However, we need to enforce this behaviour.

To safely reuse the payload buffer a new type payloadCloser is added to implement the required HTTP body semantics. On receiving a response the request may still be read up and until the response body is closed. See the documentation on RoundTripper for details. To ensure the request body is safe to release we need to wait for a Close on the body. We can eagerly move this check forward by instead waiting for a complete read and then returning io.EOF on subsequent reads. Retries may read, close or rewind the body multiple times before a response is returned and this behaviour is supported. On detecting the payload has been drained or closed the payloadCloser releases the reference to the messagePayload ensuring it's safe to be reused.

Changes

Impact

Benchmarking with -bench=BenchmarkConnect//unary shows a small improvement in throughput and memory use for unary calls (~ -5% sec/op, -5% B/op, -3% allocs/op).

Fix for #541 and #609.

mattrobenolt commented 10 months ago

👀 I am going to look at this over the weekend!

mattrobenolt commented 9 months ago

If you'd like, I can get this into a staging environment for some more thorough testing on my end. This will take a bit, but I think it'll be worth it. This is quite a core change, and I'd rather not push it through too fast. But I'm happy to get this into some chaos monkey on our side.

(We currently don't leverage GetBody, but would like to! It just didn't exist before.)

emcfarlane commented 9 months ago

@mattrobenolt that would be incredibly helpful! Will wait to see how this PR resolves and try do the same testing in staging.

emcfarlane commented 9 months ago

Hey @mattrobenolt! I think this PR is in a good state for more testing. Would you like to run it through some chaos testing on your side?

mattrobenolt commented 9 months ago

Hey @mattrobenolt! I think this PR is in a good state for more testing. Would you like to run it through some chaos testing on your side?

Yeah! Lemme construct some tests this weekend.

jhump commented 8 months ago

@mattrobenolt, we're going to go ahead and merge this. We are happy to iterate on this more post-merge if you see anything, whenever you get a chance to play with it in a staging environment.