Upgrade retry logic

stripe · Aug 22, 2019 · caaf644 · caaf644
1 parent 7137501
commit caaf644
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 8 deletions.
diff --git a/lib/HttpClient/CurlClient.php b/lib/HttpClient/CurlClient.php
@@ -258,6 +258,7 @@ public function request($method, $absUrl, $headers, $params, $hasFile)
     private function executeRequestWithRetries($opts, $absUrl)
     {
         $numRetries = 0;
+        $isPost = array_key_exists(CURLOPT_POST, $opts) && $opts[CURLOPT_POST] == 1;
 
         while (true) {
             $rcode = 0;
@@ -277,7 +278,7 @@ private function executeRequestWithRetries($opts, $absUrl)
                 $this->closeCurlHandle();
             }
 
-            if ($this->shouldRetry($errno, $rcode, $numRetries)) {
+            if ($this->shouldRetry($errno, $isPost, $rcode, $numRetries)) {
                 $numRetries += 1;
                 $sleepSeconds = $this->sleepTime($numRetries);
                 usleep(intval($sleepSeconds * 1000000));
@@ -338,11 +339,12 @@ private function handleCurlError($url, $errno, $message, $numRetries)
      * socket errors that may represent an intermittent problem and some special
      * HTTP statuses.
      * @param int $errno
+     * @param bool $isPost
      * @param int $rcode
      * @param int $numRetries
      * @return bool
      */
-    private function shouldRetry($errno, $rcode, $numRetries)
+    private function shouldRetry($errno, $isPost, $rcode, $numRetries)
     {
         if ($numRetries >= Stripe::getMaxNetworkRetries()) {
             return false;
@@ -360,11 +362,25 @@ private function shouldRetry($errno, $rcode, $numRetries)
             return true;
         }
 
-        // 409 conflict
+        // 409 Conflict
         if ($rcode === 409) {
             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) {
+            return true;
+        }
+
+        // 503 Service Unavailable
+        if ($rcode == 503) {
+            return true;
+        }
+
         return false;
     }
 

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, 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, 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, 409, 0));
+        $this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, true, 409, 0));
+    }
+
+    public function testShouldRetryOn500AndNonPost()
+    {
+        Stripe::setMaxNetworkRetries(2);
+
+        $curlClient = new CurlClient();
+
+        $this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, false, 500, 0));
+    }
+
+    public function testShouldNotRetryOn500AndPost()
+    {
+        Stripe::setMaxNetworkRetries(2);
+
+        $curlClient = new CurlClient();
+
+        $this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, true, 500, 0));
+    }
+
+    public function testShouldRetryOn503()
+    {
+        Stripe::setMaxNetworkRetries(2);
+
+        $curlClient = new CurlClient();
+
+        $this->assertTrue($this->shouldRetryMethod->invoke($curlClient, 0, true, 503, 0));
     }
 
     public function testShouldNotRetryAtMaximumCount()
@@ -157,7 +184,7 @@ public function testShouldNotRetryAtMaximumCount()
 
         $curlClient = new CurlClient();
 
-        $this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, 0, Stripe::getMaxNetworkRetries()));
+        $this->assertFalse($this->shouldRetryMethod->invoke($curlClient, 0, true, 0, Stripe::getMaxNetworkRetries()));
     }
 
     public function testShouldNotRetryOnCertValidationError()
@@ -166,7 +193,7 @@ public function testShouldNotRetryOnCertValidationError()
 
         $curlClient = new CurlClient();
 
-        $this->assertFalse($this->shouldRetryMethod->invoke($curlClient, CURLE_SSL_PEER_CERTIFICATE, -1, 0));
+        $this->assertFalse($this->shouldRetryMethod->invoke($curlClient, CURLE_SSL_PEER_CERTIFICATE, true, -1, 0));
     }
 
     public function testSleepTimeShouldGrowExponentially()