From 37cd06a83c2c49dd8c7c9f55b843435fc7754417 Mon Sep 17 00:00:00 2001 From: Olivier Bellone Date: Wed, 28 Aug 2019 14:47:33 -0700 Subject: [PATCH] Retry requests on a 429 that's a lock timeout --- lib/HttpClient/CurlClient.php | 29 ++++++++++++++- tests/Stripe/HttpClient/CurlClientTest.php | 43 ++++++++++++++++++---- 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/lib/HttpClient/CurlClient.php b/lib/HttpClient/CurlClient.php index ec3c3bb30..9887108e6 100644 --- a/lib/HttpClient/CurlClient.php +++ b/lib/HttpClient/CurlClient.php @@ -278,7 +278,7 @@ private function executeRequestWithRetries($opts, $absUrl) $this->closeCurlHandle(); } - if ($this->shouldRetry($errno, $isPost, $rcode, $numRetries)) { + if ($this->shouldRetry($errno, $isPost, $rcode, $rbody, $numRetries)) { $numRetries += 1; $sleepSeconds = $this->sleepTime($numRetries); usleep(intval($sleepSeconds * 1000000)); @@ -338,13 +338,16 @@ private function handleCurlError($url, $errno, $message, $numRetries) * Checks if an error is a problem that we should retry on. This includes both * socket errors that may represent an intermittent problem and some special * HTTP statuses. + * * @param int $errno * @param bool $isPost * @param int $rcode + * @param string $rbody * @param int $numRetries + * * @return bool */ - private function shouldRetry($errno, $isPost, $rcode, $numRetries) + private function shouldRetry($errno, $isPost, $rcode, $rbody, $numRetries) { if ($numRetries >= Stripe::getMaxNetworkRetries()) { return false; @@ -367,6 +370,28 @@ private function shouldRetry($errno, $isPost, $rcode, $numRetries) 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; + } + } + } + // 500 Internal Server Error // // We only bother retrying these for non-POST requests. POSTs end up diff --git a/tests/Stripe/HttpClient/CurlClientTest.php b/tests/Stripe/HttpClient/CurlClientTest.php index d0827ced4..c01525358 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, true, 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, true, 0, "", 0)); } public function testShouldRetryOnConflict() @@ -148,7 +148,34 @@ public function testShouldRetryOnConflict() $curlClient = new CurlClient(); - $this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, true, 409, 0)); + $this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, true, 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() + { + Stripe::setMaxNetworkRetries(2); + + $curlClient = new CurlClient(); + + $this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, true, 429, '{"error": {"code": "rate_limited"}}', 0)); + } + + public function testShouldNotRetryOn429WhenInvalidJson() + { + Stripe::setMaxNetworkRetries(2); + + $curlClient = new CurlClient(); + + $this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, true, 429, 'this is not valid JSON', 0)); } public function testShouldRetryOn500AndNonPost() @@ -157,7 +184,7 @@ public function testShouldRetryOn500AndNonPost() $curlClient = new CurlClient(); - $this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, false, 500, 0)); + $this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, false, 500, "", 0)); } public function testShouldNotRetryOn500AndPost() @@ -166,7 +193,7 @@ public function testShouldNotRetryOn500AndPost() $curlClient = new CurlClient(); - $this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, true, 500, 0)); + $this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, true, 500, "", 0)); } public function testShouldRetryOn503() @@ -175,7 +202,7 @@ public function testShouldRetryOn503() $curlClient = new CurlClient(); - $this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, true, 503, 0)); + $this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, true, 503, "", 0)); } public function testShouldNotRetryAtMaximumCount() @@ -184,7 +211,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, true, 0, "", Stripe::getMaxNetworkRetries())); } public function testShouldNotRetryOnCertValidationError() @@ -193,7 +220,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, true, -1, "", 0)); } public function testSleepTimeShouldGrowExponentially()