Implement unary HTTP calls with retry support #649
Conversation
|
👀 I am going to look at this over the weekend! |
Changes the internal duplexHTTPCall to switch on non streaming client requests to block and wait for the response. This avoids the need to convert the reader to a writer with io.Pipe and the go routine to run asynchronously. On unary requests we are now able to set the `Content-Length` header and the `GetBody` function for retries. 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 is body is closed. To ensure the request body is safe to releasee we wait for a complete read or close on the request body. Retries may read, close or rewind the body multiple times before a response is returned.
emcfarlane
force-pushed
the
unaryCalls
branch
from
ee09163
to
f85fce8
Compare
Ensure we set GetBody on all zero length requests.
There was a problem hiding this comment.
The blocking Wait makes me slightly nervous. An alternative might be a way to have the call invalidate the payload closer -- so instead of waiting for it to asynchronously complete, we force it complete synchronously. Any concurrent reader/interaction will suddenly get EOF after it's invalidated. That seems much more predictable and safer since it still prevents buffer use-after-release but does so without non-deterministic latency/blocking.
|
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.) |
|
@mattrobenolt that would be incredibly helpful! Will wait to see how this PR resolves and try do the same testing in staging. |
Removes the wait and instead simplifies to dropping the payload when the response is received. Should be valid for all unary servers.
|
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. |
|
@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. |
Fixes the LICENSE header for new test file introduced in #649 . Causing CI failure on main: https://github.com/connectrpc/connect-go/actions/runs/7616456681/job/20743252336
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 theio.Pipeto 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 theContent-Lengthheader and implementingGetBodyfunction 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 adefer 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 fromSend. Under testing aclient.Dowill 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
payloadCloseris 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 onRoundTripperfor details. To ensure the request body is safe to release we need to wait for aCloseon the body. We can eagerly move this check forward by instead waiting for a complete read and then returningio.EOFon 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 thepayloadCloserreleases the reference to themessagePayloadensuring it's safe to be reused.Changes
Content-Lengthheader set for single messages client requests.GetBodyfunction for retries of single message client requests.messages.Impact
Benchmarking with
-bench=BenchmarkConnect//unaryshows 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.