Retry requests on a 429 that's a lock timeout

stripe · Aug 28, 2019 · 37cd06a · 37cd06a
1 parent 1a08a98
commit 37cd06a
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 10 deletions.
diff --git 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
@@ -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()