Special case unary HTTP calls #611
Conversation
There was a problem hiding this comment.
This didn't actually add a separate entry point to protocolClient as previously discussed. That's a bit awkward since it means the NewConn still has to special-case unary in ways that cause the shape of the abstractions to be more complicated than needed. For example, connectUnaryClientConn seems to be pure bloat. We could get rid of it, and then also improve the unaryHTTPCall's interface if we separate unary from stream calls higher up the stack.
See #609 for a little more context.
|
The only thing I wanted to add to this right now is this should in theory be able to be applied just fine for server streaming as well, and I'd suspect some optimizations for client streaming? The only special case needing the io.Pipe should be for bidi. So if we can swallow up "everything except bidi", that'd be my dream implementation. 🙏 Only commenting now since I'd hate to be hyper specific on unary and not catch server stream, at minimum. |
Hmmm, that feels like hyper-specialization. I think it's worth it for unary since they are the overwhelming majority of RPCs, and the flow of a unary RPC is straight-forward enough that a specialized flow would maximize readability. But also separately optimizing the other streaming types feel like it will either (1) lead to too many permutations (we suddenly have four specializations to maintain instead of two) or (2) require substantial re-architecting of That's not really an objective that has been discussed before now. Maybe we should take this discussion to the issue (#609), to better nail down what stakeholders are expecting?
I don't see how this is true. How does client-streaming work without an |
|
I understand I'm not the one implementing this, so I don't fully know the context of everything here, but there are two sides to this. Client request is buffered or streamed, or the server response is buffered or streamed, or the permutation of both streaming. A simple Unary is buffered request and buffered response, while a server stream is buffered request, streamed response. Both pure unary and server stream are the same "write request, read response" with Body being a reader. The choice is to either buffer or stream it. I might be misspeaking on client streaming since that one is a bit more unique and might be limited by stdlib APIs. But the io.Pipe is only necessary to facilitate reading and writing asynchronously. imo the "unary" implementation at minimum can easily cover the pure unary and server stream. If it makes sense for APIs available, to lump client streaming with bidi, I think that's fine too, since client streaming is used even less. We quite heavily utilize server streaming, and there's no reason that needs to be done with the extra goroutine either. The implementaiton is just reading N messages rather than 1. |
Oh, I missed this! Yes, I agree. |
Co-authored-by: Matt Robenolt <matt@ydekproductions.com>
|
Waiting to merge changes in #594 |
| responseReady chan struct{} | ||
| request *http.Request | ||
| response *http.Response | ||
| requestSent atomic.Bool |
There was a problem hiding this comment.
Switch to atomic.Bool to be able to get the state of the sync.Once and error on unary if already called.
| @@ -130,179 +116,196 @@ func (d *duplexHTTPCall) CloseWrite() error { | |||
| // forever. To make sure users don't have to worry about this, the generated | |||
| // code for unary, client streaming, and server streaming RPCs must call | |||
| // CloseWrite automatically rather than requiring the user to do it. | |||
| return d.requestBodyWriter.Close() | |||
| if c.requestBodyWriter != nil { | |||
| return c.requestBodyWriter.Close() | |||
There was a problem hiding this comment.
Ensure we keep the close semantics in #594
| wg.Wait() | ||
| } | ||
|
|
||
| func TestHTTPCallRaces(t *testing.T) { |
There was a problem hiding this comment.
This isn't a very nice test but did help recreate the race conditions I was seeing in CI. Any suggestions for improvements?
| } | ||
|
|
||
| // Close implements io.Closer. It signals that the payload has been fully read. | ||
| func (p *payloadCloser) Close() error { |
There was a problem hiding this comment.
Close should be called by the http.Transport on receiving an io.EOF and finished with the request body. But under testing the HTTP2 transport would sometimes delay the call leading to a deadlock on Wait(). We therefore also signal completion by reading all the bytes in the buffer.
There was a problem hiding this comment.
I'm trying to grok the formulation in #622 as it relates to the changes here. I take it that you'd change the httpCall.Send(*bytes.Buffer) method to instead be httpCall.Send(*envelope), avoid having to create the unified buffer (which means copying the prefix and the serialized message to yet-another-buffer). Is that how you see these two fitting together?
| p.wait.Wait() | ||
| p.mu.Lock() | ||
| defer p.mu.Unlock() | ||
| p.buf = nil // Discard the buffer |
There was a problem hiding this comment.
I think this will defeat the GetBody improvement. It seems likely the HTTP framework could rain the entire body (particularly for small bodies) and/or call Close and still need to be able to rewind the buffer and send again.
There was a problem hiding this comment.
GetBody shouldn't be called after we've received a response. Draining or calling Close will allow Send to return and the buffer to be recycled but GetBody can call Rewind() to reset as many times as needed (5x I think max). The number of Close calls or drains isn't counted.
| // Wait for the payload to be fully drained before we return | ||
| // from Send to ensure the buffer can safely be reused. | ||
| defer payload.Wait() |
There was a problem hiding this comment.
This seems excessively dangerous to have unbounded blocking here. Is it really 100% certain that the body will always be drained or closed in a timely manner, in the face of all possible errors?
And, even if so, is it worth paying potential latency hit to wait for this? I would think instead we'd release the buffer (i.e. return to pool) asynchronously, instead of holding of the train here.
There was a problem hiding this comment.
Close has to be called as any reader would leak otherwise. This keeps the current behaviour between sendUnary and sendStream: both don't modify the buffer and both return and the buffer is safe to mutate afterwards.
|
Superseded by #649 |
Special paths the unary request flow to avoid some of the overhead with streaming request bodies. Unary requests now set
http.Request'sGetBodyfield with the full buffer payload. Buffers are wrapped in apayloadCloserto ensure they can be safely re-used.See issues #609 and #541 .
Status
Unary call optimization is applied to all support protocols.
GetBodyis called on go awayio.Readerandio.WriterTofor envelope and avoid buffer copy for client envelopesRemaining work will be done in a cleanup PR to optimize the envelope payloads.
Benchmarks
Updated to latest commit. From the benchmarks theres some reduction in allocs for Unary flows, small increase in bytes due to the missing optimization on envelopes.