twitchtv / twirp

A simple RPC framework with protobuf service definitions
https://twitchtv.github.io/twirp/docs/intro.html
Apache License 2.0
7.2k stars 326 forks source link

Ensure response body is fully read on context errors #387

Closed nsafai closed 1 year ago

nsafai commented 1 year ago

Issue #, if available: https://github.com/twitchtv/twirp/issues/384

Description of changes: As mentioned in the GH issue above, the Golang docs specify that:

If the Body is not both read to EOF and closed, the Client's underlying RoundTripper (typically Transport) may not be able to re-use a persistent TCP connection to the server for a subsequent "keep-alive" request.

The original poster QueenCitySmitty mentions that

Both doProtobufRequest and doJSONRequest only defer closing the body, not also reading to EOF. This has the potential to cause issues with TCP.

marioizquierdo makes a good suggestion to do this only for context errors as it is:

[...] safer to implement, and will not cause a second read on the response body for regular successful requests.