Make stream client closers non-blocking #791
Conversation
This updates the behavior of the client streaming methods `BidiStreamForClient.CloseResponse` and `ServerStreamForClient.Close` to be non-blocking, aligning it with the standard behavior of `net/http`'s `Request.Body` closure. Previously, the implementation used a graceful, blocking closure that fully read from the stream before closing. This allows for reuse of the underlying TCP connection. However, this behavior could lead to unexpected client hangs, as users may not anticipate blocking on close. To address this, the closers no long drain the stream. Documentation has been updated to clarify the behavior and provide users a workaround to keep the optimization by Receiving messages until the stream is drain. This avoids unexpected blocking behavior in client applications. Signed-off-by: Edward McFarlane <[email protected]>
There was a problem hiding this comment.
At one point, I understood the point of draining the response stream to not only enable connection re-use with HTTP 1.1 but also to allow the application to get the status and trailers.
However, after playing around with it, this has never really worked (just as you expected, @emcfarlane).
If you are using the gRPC protocol and your application calls stream.Receive after stream.CloseResponse, then the application can get to the trailers. But that call to Receive returns an error along the lines of "connection closed" (with, unexpectedly, an "invalid_argument" status code) even if trailers indicated an RPC error. This of course doesn't work with Connect or gRPC-web protocols since the discard operation is blindly draining the body and not looking at envelopes and thus not trying to extract the final status/trailers from the body.
So I can now confidently say that this change is sufficiently backwards-compatible to merge.
This updates the behavior of the streaming client methods
BidiStreamForClient.CloseResponseandServerStreamForClient.Closeto be non-blocking, aligning it with the standard behavior ofnet/http'sRequest.Bodyclosure.Previously, the implementation used a graceful, blocking closure that fully read from the stream before closing. This allows for reuse of the underlying TCP connection. However, this behavior could lead to unexpected client hangs, as users may not anticipate blocking on close.
To address this, the closers no longer drain the stream. Documentation has been updated to clarify the behavior and provide users a workaround to keep the optimization by calling receive until the stream is drained. This avoids unexpected blocking behavior in client applications.
Fixes #789