Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect Stripe-Should-Retry and Retry-After headers #755

Merged
merged 5 commits into from
Oct 7, 2019
+105 −88
Merged

Respect Stripe-Should-Retry and Retry-After headers #755

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c2127b3
Move header callback into executeRequestWithRetries
rattrayalex-stripe Oct 4, 2019
c550dfd
Respect stripe-should-retry header
rattrayalex-stripe Oct 5, 2019
dd58835
Respect retry-after header
rattrayalex-stripe Oct 5, 2019
5c2da7c
Check if header is set
rattrayalex-stripe Oct 5, 2019
9f8463f
Fix warnings
rattrayalex-stripe Oct 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 48 additions & 53 deletions lib/HttpClient/CurlClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,18 +204,6 @@ public function request($method, $absUrl, $headers, $params, $hasFile)
}
}

// Create a callback to capture HTTP headers for the response
$rheaders = new Util\CaseInsensitiveArray();
$headerCallback = function ($curl, $header_line) use (&$rheaders) {
// Ignore the HTTP request line (HTTP/1.1 200 OK)
if (strpos($header_line, ":") === false) {
return strlen($header_line);
}
list($key, $value) = explode(":", trim($header_line), 2);
$rheaders[trim($key)] = trim($value);
return strlen($header_line);
};

// By default for large request body sizes (> 1024 bytes), cURL will
// send a request without a body and with a `Expect: 100-continue`
// header, which gives the server a chance to respond with an error
Expand All @@ -235,7 +223,6 @@ public function request($method, $absUrl, $headers, $params, $hasFile)
$opts[CURLOPT_RETURNTRANSFER] = true;
$opts[CURLOPT_CONNECTTIMEOUT] = $this->connectTimeout;
$opts[CURLOPT_TIMEOUT] = $this->timeout;
$opts[CURLOPT_HEADERFUNCTION] = $headerCallback;
$opts[CURLOPT_HTTPHEADER] = $headers;
$opts[CURLOPT_CAINFO] = Stripe::getCABundlePath();
if (!Stripe::getVerifySslCerts()) {
Expand All @@ -247,7 +234,7 @@ public function request($method, $absUrl, $headers, $params, $hasFile)
$opts[CURLOPT_HTTP_VERSION] = CURL_HTTP_VERSION_2TLS;
}

list($rbody, $rcode) = $this->executeRequestWithRetries($opts, $absUrl);
list($rbody, $rcode, $rheaders) = $this->executeRequestWithRetries($opts, $absUrl);

return [$rbody, $rcode, $rheaders];
}
Expand All @@ -264,6 +251,19 @@ private function executeRequestWithRetries($opts, $absUrl)
$rcode = 0;
$errno = 0;

// Create a callback to capture HTTP headers for the response
$rheaders = new Util\CaseInsensitiveArray();
$headerCallback = function ($curl, $header_line) use (&$rheaders) {
// Ignore the HTTP request line (HTTP/1.1 200 OK)
if (strpos($header_line, ":") === false) {
return strlen($header_line);
}
list($key, $value) = explode(":", trim($header_line), 2);
$rheaders[trim($key)] = trim($value);
return strlen($header_line);
};
$opts[CURLOPT_HEADERFUNCTION] = $headerCallback;

$this->resetCurlHandle();
curl_setopt_array($this->curlHandle, $opts);
$rbody = curl_exec($this->curlHandle);
Expand All @@ -278,9 +278,9 @@ private function executeRequestWithRetries($opts, $absUrl)
$this->closeCurlHandle();
}

if ($this->shouldRetry($errno, $isPost, $rcode, $rbody, $numRetries)) {
if ($this->shouldRetry($errno, $rcode, $rheaders, $numRetries)) {
$numRetries += 1;
$sleepSeconds = $this->sleepTime($numRetries);
$sleepSeconds = $this->sleepTime($numRetries, $rheaders);
usleep(intval($sleepSeconds * 1000000));
} else {
break;
Expand All @@ -291,7 +291,7 @@ private function executeRequestWithRetries($opts, $absUrl)
$this->handleCurlError($absUrl, $errno, $message, $numRetries);
}

return [$rbody, $rcode];
return [$rbody, $rcode, $rheaders];
}

/**
Expand Down Expand Up @@ -340,14 +340,13 @@ private function handleCurlError($url, $errno, $message, $numRetries)
* HTTP statuses.
*
* @param int $errno
* @param bool $isPost
* @param int $rcode
* @param string $rbody
* @param array|CaseInsensitiveArray $rheaders
* @param int $numRetries
*
* @return bool
*/
private function shouldRetry($errno, $isPost, $rcode, $rbody, $numRetries)
private function shouldRetry($errno, $rcode, $rheaders, $numRetries)
{
if ($numRetries >= Stripe::getMaxNetworkRetries()) {
return false;
Expand All @@ -365,51 +364,41 @@ private function shouldRetry($errno, $isPost, $rcode, $rbody, $numRetries)
return true;
}

// 409 Conflict
if ($rcode === 409) {
return true;
// The API may ask us not to retry (eg; if doing so would be a no-op)
// or advise us to retry (eg; in cases of lock timeouts); we defer to that.
if ($rheaders['stripe-should-retry'] === 'false') {
rattrayalex-stripe marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

// 429 Too Many Requests
//
// There are a few different problems that can lead to a 429. The most
// common is rate limiting, on which we *don't* want to retry because
// that'd likely contribute to more contention problems. However, some
// 429s are lock timeouts, which is when a request conflicted with
// another request or an internal process on some particular object.
// These 429s are safe to retry.
if ($rcode === 429) {
// It's not great that we're doing this here. In a future version,
// we should decouple the retry logic from the CurlClient instance,
// so that we don't need to deserialize here (and also so that the
// retry logic applies to non-curl clients).
$resp = json_decode($rbody, true);
if ($resp !== null && array_key_exists('error', $resp)) {
$error = \Stripe\ErrorObject::constructFrom($resp['error']);
if ($error->code === \Stripe\ErrorObject::CODE_LOCK_TIMEOUT) {
return true;
}
}
if ($rheaders['stripe-should-retry'] === 'true') {
return true;
}

// 500 Internal Server Error
//
// We only bother retrying these for non-POST requests. POSTs end up
// being cached by the idempotency layer so there's no purpose in
// retrying them.
if ($rcode >= 500 && !$isPost) {
// 409 Conflict
if ($rcode === 409) {
return true;
}

// 503 Service Unavailable
if ($rcode == 503) {
// Retry on 500, 503, and other internal errors.
//
// Note that we expect the stripe-should-retry header to be false
// in most cases when a 500 is returned, since our idempotency framework
// would typically replay it anyway.
if ($rcode >= 500) {
return true;
}

return false;
}

private function sleepTime($numRetries)
/**
* Provides the number of seconds to wait before retrying a request.
*
* @param int $numRetries
* @param array|CaseInsensitiveArray $rheaders
*
* @return int
*/
private function sleepTime($numRetries, $rheaders)
{
// Apply exponential backoff with $initialNetworkRetryDelay on the
// number of $numRetries so far as inputs. Do not allow the number to exceed
Expand All @@ -426,6 +415,12 @@ private function sleepTime($numRetries)
// But never sleep less than the base sleep seconds.
$sleepSeconds = max(Stripe::getInitialNetworkRetryDelay(), $sleepSeconds);

// And never sleep less than the time the API asks us to wait, assuming it's a reasonable ask.
$retryAfter = floatval($rheaders['retry-after']);
if (floor($retryAfter) == $retryAfter && $retryAfter <= Stripe::getMaxRetryAfter()) {
$sleepSeconds = max($sleepSeconds, $retryAfter);
}

return $sleepSeconds;
}

Expand Down
11 changes: 11 additions & 0 deletions lib/Stripe.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ class Stripe
// @var float Maximum delay between retries, in seconds
private static $maxNetworkRetryDelay = 2.0;

// @var float Maximum delay between retries, in seconds, that will be respected from the Stripe API
private static $maxRetryAfter = 60.0;

// @var float Initial delay between retries, in seconds
private static $initialNetworkRetryDelay = 0.5;

Expand Down Expand Up @@ -235,6 +238,14 @@ public static function getMaxNetworkRetryDelay()
return self::$maxNetworkRetryDelay;
}

/**
* @return float Maximum delay between retries, in seconds, that will be respected from the Stripe API
*/
public static function getMaxRetryAfter()
{
return self::$maxRetryAfter;
}

/**
* @return float Initial delay between retries, in seconds
*/
Expand Down
79 changes: 44 additions & 35 deletions tests/Stripe/HttpClient/CurlClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public function testShouldRetryOnTimeout()

$curlClient = new CurlClient();

$this->assertTrue($this->shouldRetryMethod->invoke($curlClient, CURLE_OPERATION_TIMEOUTED, true, 0, "", 0));
$this->assertTrue($this->shouldRetryMethod->invoke($curlClient, CURLE_OPERATION_TIMEOUTED, 0, [], 0));
}

public function testShouldRetryOnConnectionFailure()
Expand All @@ -139,7 +139,7 @@ public function testShouldRetryOnConnectionFailure()

$curlClient = new CurlClient();

$this->assertTrue($this->shouldRetryMethod->invoke($curlClient, CURLE_COULDNT_CONNECT, true, 0, "", 0));
$this->assertTrue($this->shouldRetryMethod->invoke($curlClient, CURLE_COULDNT_CONNECT, 0, [], 0));
}

public function testShouldRetryOnConflict()
Expand All @@ -148,61 +148,55 @@ public function testShouldRetryOnConflict()

$curlClient = new CurlClient();

$this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, true, 409, "", 0));
$this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, 409, [], 0));
}

public function testShouldRetryOn429WhenLockTimeout()
{
Stripe::setMaxNetworkRetries(2);

$curlClient = new CurlClient();

$this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, true, 429, '{"error": {"code": "lock_timeout"}}', 0));
}

public function testShouldNotRetryOn429WhenNotLockTimeout()
public function testShouldNotRetryOn429()
{
Stripe::setMaxNetworkRetries(2);

$curlClient = new CurlClient();

$this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, true, 429, '{"error": {"code": "rate_limited"}}', 0));
$this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, 429, [], 0));
}

public function testShouldNotRetryOn429WhenInvalidJson()
public function testShouldRetryOn500()
{
Stripe::setMaxNetworkRetries(2);

$curlClient = new CurlClient();

$this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, true, 429, 'this is not valid JSON', 0));
$this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, 500, [], 0));
}

public function testShouldRetryOn500AndNonPost()
public function testShouldRetryOn503()
{
Stripe::setMaxNetworkRetries(2);

$curlClient = new CurlClient();

$this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, false, 500, "", 0));
$this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, 503, [], 0));
}

public function testShouldNotRetryOn500AndPost()
public function testShouldRetryOnStripeShouldRetryTrue()
{
Stripe::setMaxNetworkRetries(2);

$curlClient = new CurlClient();

$this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, true, 500, "", 0));
$this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, 400, [], 0));
$this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, 400, ['stripe-should-retry' => 'true'], 0));
}

public function testShouldRetryOn503()
public function testShouldNotRetryOnStripeShouldRetryFalse()
{
Stripe::setMaxNetworkRetries(2);

$curlClient = new CurlClient();

$this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, true, 503, "", 0));
$this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, 500, [], 0));
$this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, 500, ['stripe-should-retry' => 'false'], 0));
}

public function testShouldNotRetryAtMaximumCount()
Expand All @@ -211,7 +205,7 @@ public function testShouldNotRetryAtMaximumCount()

$curlClient = new CurlClient();

$this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, true, 0, "", Stripe::getMaxNetworkRetries()));
$this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, 0, [], Stripe::getMaxNetworkRetries()));
}

public function testShouldNotRetryOnCertValidationError()
Expand All @@ -220,7 +214,7 @@ public function testShouldNotRetryOnCertValidationError()

$curlClient = new CurlClient();

$this->assertFalse($this->shouldRetryMethod->invoke($curlClient, CURLE_SSL_PEER_CERTIFICATE, true, -1, "", 0));
$this->assertFalse($this->shouldRetryMethod->invoke($curlClient, CURLE_SSL_PEER_CERTIFICATE, -1, [], 0));
}

public function testSleepTimeShouldGrowExponentially()
Expand All @@ -231,19 +225,19 @@ public function testSleepTimeShouldGrowExponentially()

$this->assertEquals(
Stripe::getInitialNetworkRetryDelay() * 1,
$this->sleepTimeMethod->invoke($curlClient, 1)
$this->sleepTimeMethod->invoke($curlClient, 1, [])
);
$this->assertEquals(
Stripe::getInitialNetworkRetryDelay() * 2,
$this->sleepTimeMethod->invoke($curlClient, 2)
$this->sleepTimeMethod->invoke($curlClient, 2, [])
);
$this->assertEquals(
Stripe::getInitialNetworkRetryDelay() * 4,
$this->sleepTimeMethod->invoke($curlClient, 3)
$this->sleepTimeMethod->invoke($curlClient, 3, [])
);
$this->assertEquals(
Stripe::getInitialNetworkRetryDelay() * 8,
$this->sleepTimeMethod->invoke($curlClient, 4)
$this->sleepTimeMethod->invoke($curlClient, 4, [])
);
}

Expand All @@ -254,10 +248,25 @@ public function testSleepTimeShouldEnforceMaxNetworkRetryDelay()

$curlClient = new CurlClient(null, $this->createFakeRandomGenerator());

$this->assertEquals(1, $this->sleepTimeMethod->invoke($curlClient, 1));
$this->assertEquals(2, $this->sleepTimeMethod->invoke($curlClient, 2));
$this->assertEquals(2, $this->sleepTimeMethod->invoke($curlClient, 3));
$this->assertEquals(2, $this->sleepTimeMethod->invoke($curlClient, 4));
$this->assertEquals(1, $this->sleepTimeMethod->invoke($curlClient, 1, []));
$this->assertEquals(2, $this->sleepTimeMethod->invoke($curlClient, 2, []));
$this->assertEquals(2, $this->sleepTimeMethod->invoke($curlClient, 3, []));
$this->assertEquals(2, $this->sleepTimeMethod->invoke($curlClient, 4, []));
}

public function testSleepTimeShouldRespectRetryAfter()
{
$this->setInitialNetworkRetryDelay(1);
$this->setMaxNetworkRetryDelay(2);

$curlClient = new CurlClient(null, $this->createFakeRandomGenerator());

// Uses max of default and header.
$this->assertEquals(10, $this->sleepTimeMethod->invoke($curlClient, 1, ['retry-after' => '10']));
$this->assertEquals(2, $this->sleepTimeMethod->invoke($curlClient, 2, ['retry-after' => '1']));

// Ignores excessively large values.
$this->assertEquals(2, $this->sleepTimeMethod->invoke($curlClient, 2, ['retry-after' => '100']));
}

public function testSleepTimeShouldAddSomeRandomness()
Expand All @@ -272,12 +281,12 @@ public function testSleepTimeShouldAddSomeRandomness()

// the initial value cannot be smaller than the base,
// so the randomness is ignored
$this->assertEquals(Stripe::getInitialNetworkRetryDelay(), $this->sleepTimeMethod->invoke($curlClient, 1));
$this->assertEquals(Stripe::getInitialNetworkRetryDelay(), $this->sleepTimeMethod->invoke($curlClient, 1, []));

// after the first one, the randomness is applied
$this->assertEquals($baseValue * 2, $this->sleepTimeMethod->invoke($curlClient, 2));
$this->assertEquals($baseValue * 4, $this->sleepTimeMethod->invoke($curlClient, 3));
$this->assertEquals($baseValue * 8, $this->sleepTimeMethod->invoke($curlClient, 4));
$this->assertEquals($baseValue * 2, $this->sleepTimeMethod->invoke($curlClient, 2, []));
$this->assertEquals($baseValue * 4, $this->sleepTimeMethod->invoke($curlClient, 3, []));
$this->assertEquals($baseValue * 8, $this->sleepTimeMethod->invoke($curlClient, 4, []));
}

public function testResponseHeadersCaseInsensitive()
Expand Down