Don't check the response body close error in the client #336
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hello, thank you for the PR. The change has some extraneous changes due to an older version of |
cespare
force-pushed
the
remove-response-close-check
branch
from
00fd4e4 to
1e64f8b
Compare
The 'go generate' command used sh to run a bash script. That script uses 'set -o pipefail', but that is Bash-specific. Instead, run the script directly, so that we use Bash.
If we successfully read the response all the way to EOF and decode it, that constitutes a successful RPC. If the body close happens to return a non-nil error, the client shouldn't care, since it was able to fully read the response.
cespare
force-pushed
the
remove-response-close-check
branch
from
1e64f8b to
e0e9a4d
Compare
Sure, done. Though if you care about that, ISTM that the tooling ought to check for the particular version. |
|
Thank you for the update, looks good 👍
I agree. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If we successfully read the response all the way to EOF and decode it, that constitutes a successful RPC. If the body close happens to return a non-nil error, the client shouldn't care.
The context where this came up is that Go 1.17.3 seemingly introduced a bug (reported here) where sometimes the response body Close method returns a spurious error. This meant that upgrading to Go 1.17.3 caused Twirp requests to start failing for us.
The actual bug is on the Go side, but the fact that nobody noticed and the tests didn't catch it is more evidence that checking this error is uncommon and unnecessary.
(And if I may editorialize slightly: this is why the errcheck linter is not a good blanket policy.)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.