diff --git a/lib/HttpClient/CurlClient.php b/lib/HttpClient/CurlClient.php index 9887108e6..7673e397d 100644 --- a/lib/HttpClient/CurlClient.php +++ b/lib/HttpClient/CurlClient.php @@ -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 @@ -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()) { @@ -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]; } @@ -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); @@ -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; @@ -291,7 +291,7 @@ private function executeRequestWithRetries($opts, $absUrl) $this->handleCurlError($absUrl, $errno, $message, $numRetries); } - return [$rbody, $rcode]; + return [$rbody, $rcode, $rheaders]; } /** @@ -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; @@ -365,51 +364,43 @@ private function shouldRetry($errno, $isPost, $rcode, $rbody, $numRetries) return true; } - // 409 Conflict - if ($rcode === 409) { - return true; - } - - // 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; - } + // 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 (isset($rheaders['stripe-should-retry'])) { + if ($rheaders['stripe-should-retry'] === 'false') { + return false; + } + 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 @@ -426,6 +417,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 = isset($rheaders['retry-after']) ? floatval($rheaders['retry-after']) : 0.0; + if (floor($retryAfter) == $retryAfter && $retryAfter <= Stripe::getMaxRetryAfter()) { + $sleepSeconds = max($sleepSeconds, $retryAfter); + } + return $sleepSeconds; } diff --git a/lib/Stripe.php b/lib/Stripe.php index 59b69159d..14f6a15b5 100644 --- a/lib/Stripe.php +++ b/lib/Stripe.php @@ -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; @@ -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 */ diff --git a/tests/Stripe/HttpClient/CurlClientTest.php b/tests/Stripe/HttpClient/CurlClientTest.php index c01525358..3c54f9d89 100644 --- a/tests/Stripe/HttpClient/CurlClientTest.php +++ b/tests/Stripe/HttpClient/CurlClientTest.php @@ -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() @@ -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() @@ -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() @@ -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() @@ -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() @@ -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, []) ); } @@ -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() @@ -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()