Automatic request retries #428
Conversation
|
This is awesome! I'll be able to remove some home-grown code that does this in app land |
lib/HttpClient/CurlClient.php
Outdated
| $headers['Idempotency-Key'] = bin2hex(openssl_random_pseudo_bytes(32)); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think you meant "PUT" here and not "DELETE"?
Your own documentation says it's not necessary for DELETE.
There was a problem hiding this comment.
I cargo-culted the implementation from stripe-ruby, but you're absolutely correct: idempotency does not apply to DELETE requests.
The API does not use PUT requests at all, so POST is the only method that can use idempotency. I'll update the PR. Thanks 👍
ob-stripe
force-pushed
the
ob-retry-requests
branch
2 times, most recently
from
c1e2a0c to
1ff2463
Compare
ob-stripe
force-pushed
the
ob-retry-requests
branch
from
1ff2463 to
e5ba09d
Compare
There was a problem hiding this comment.
Doh. Sorry about the delay here @ob-stripe! I just found these two comments that I left literally a week ago, but just forgot to submit the review. Overall, looks great, thanks!
lib/Util/RandomGenerator.php
Outdated
|
|
||
| namespace Stripe\Util; | ||
|
|
||
| class RandomGenerator |
There was a problem hiding this comment.
Maybe add a note in here that this exists for testability by using it with dependency injection.
|
|
||
| private function setMaxNetworkRetryDelay($maxNetworkRetryDelay) | ||
| { | ||
| $property = $this->getPrivateProperty('Stripe\Stripe', 'maxNetworkRetryDelay'); |
There was a problem hiding this comment.
IMO, it might be going a little too far by adding helpers to access private members for this. After these are available, the tendency will be to use them more often, and IMO it's not great practice to reach into the private parts of any implementation — even if it's your own.
I don't have an amazing alternate suggestion, but I think it would be better even to just move the helpers back into this test suite so that they're not generalized in TestCase. Thoughts?
There was a problem hiding this comment.
Yeah, that makes sense. I've removed the helpers from TestCase and the various reflectors are now directly instantiated in CurlClientTest.setUpReflectors.
lib/HttpClient/CurlClient.php
Outdated
| if ((($method == 'post') || ($method == 'delete')) && | ||
| (Stripe::$maxNetworkRetries > 0)) { | ||
| if (!isset($headers['Idempotency-Key'])) { | ||
| $headers['Idempotency-Key'] = bin2hex(openssl_random_pseudo_bytes(32)); |
There was a problem hiding this comment.
Just going to say that it's kind of weird to have a $randomGenerator injected via dependency, use it in one place, but then use a different method for generating randomness elsewhere in the same class :)
There was a problem hiding this comment.
Agreed. I've actually already updated RandomGenerator with a uuid method that returns a real V4 UUID since that's the best practice recommended in our documentation (and what's done in stripe-ruby too).
There was a problem hiding this comment.
Nice! Love the UUID generator.
|
ptal @brandur-stripe |
|
LGTM. 🚢 |
r? @brandur-stripe
cc @stripe/api-libraries
Fixes #394.
Implements automatic request retries, a la stripe-ruby.
As discussed IRL, I'm not totally happy with this implementation as it's not possible to fully test it -- we would need to be able to mock
curl_exec()and other curl functions to do so, which isn't really possible in PHP.Ideally, this feature should be implemented in
ApiRequestorand not directly inCurlClient, but doing so would require substantial changes to theHttpClientinterface -- for starters, we'd need to have a standard way of raising connection errors so thatApiRequestorcould decide whether to retry or not. (Right nowCurlClientjust raises anError\ApiConnectionexception for all connection errors.)