Set request body before every retry #646
Merged
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
|
Actually, I just realized that I think I can do this with less interface churn. Let me take one more shot at this ... |
brandur-stripe
force-pushed
the
brandur-reset-body-on-retries
branch
from
e96151a to
591e6a1
Compare
This patch changes the interfaces of `NewRequest` and `Do` around a little so that we can set a new request body with every request. In the era of HTTP (1), it was safe to reuse a `Request` object, but with the addition of HTTP/2, it's now only sometimes safe. Reusing a `Request` with a body will break. See some more information here: golang/go#19653 (comment) Fixes #642.
brandur-stripe
force-pushed
the
brandur-reset-body-on-retries
branch
from
591e6a1 to
cb7a4cc
Compare
|
Okay, I think there's a race condition in one of our tests (arg, had to restart one of the builds), but I'll fix that separately. For now, let's try to get this patch out. r? @remi-stripe |
remi-stripe
approved these changes
Aug 4, 2018
There was a problem hiding this comment.
Wow that was some really good sleuthing @brandur-stripe. That nginx error has bitten us more than once before since our libraries can't handle it properly.
This LGTM though as usual I only grasp 90% of the changes themselves.
Thank you for the investigation and the quick fix!
|
NP. Thanks Remi! |
brandur-stripe
pushed a commit
that referenced
this pull request
Aug 6, 2018
In #646, I attempted to fix a problem whereby when using HTTP/2 we were sending invalid requests during automatic retries because a `Request` struct with a body cannot be reused in the context of HTTP/2. Unfortunately, it wasn't blowing up quite as spectacularly as before, the fix didn't quite work either. It would set a new `Body` before retries, but that body would be pointing to an `io.Reader` that was already exhausted as it had been fully consumed during the original request, thereby producing an empty request body. Here we modify the `Do` and `CallMultipart` interfaces so that they take body buffers instead of just readers. When making a request, we create a new reader for those buffers every time, thus ensuring a fresh one. This is relatively trivial when making non-multipart requests (which is most requests) because we were already producing a buffer in almost the right place. It's a little more complicated for multipart requests (file uploads) because we had encoding scheme that wasn't quite compatible, but I've done some refactoring there (and added more tests) to bring things in line. We also add a much improved test framework this time around that verifies that the problem is fixed and will definitively stay fixed. Fixes #647.
brandur-stripe
pushed a commit
that referenced
this pull request
Aug 6, 2018
In #646, I attempted to fix a problem whereby when using HTTP/2 we were sending invalid requests during automatic retries because a `Request` struct with a body cannot be reused in the context of HTTP/2. Unfortunately, it wasn't blowing up quite as spectacularly as before, the fix didn't quite work either. It would set a new `Body` before retries, but that body would be pointing to an `io.Reader` that was already exhausted as it had been fully consumed during the original request, thereby producing an empty request body. Here we modify the `Do` and `CallMultipart` interfaces so that they take body buffers instead of just readers. When making a request, we create a new reader for those buffers every time, thus ensuring a fresh one. This is relatively trivial when making non-multipart requests (which is most requests) because we were already producing a buffer in almost the right place. It's a little more complicated for multipart requests (file uploads) because we had encoding scheme that wasn't quite compatible, but I've done some refactoring there (and added more tests) to bring things in line. We also add a much improved test framework this time around that verifies that the problem is fixed and will definitively stay fixed. Fixes #647.
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.
This patch changes the interfaces of
NewRequestandDoaround alittle so that we can set a new request body with every request.
In the era of HTTP (1), it was safe to reuse a
Requestobject, butwith the addition of HTTP/2, it's now only sometimes safe. Reusing a
Requestwith a body will break.See some more information here:
golang/go#19653 (comment)
Fixes #642.
r? @remi-stripe
cc @stripe/api-libraries