feat(Storage): Add retry conformance tests (#5637)

* feat(Storage): Add contextual retry capability. * feat(Storage): Add retry conformance tests.
googleapis · Apr 25, 2023 · a8211f2 · a8211f2
1 parent fa1e622
commit a8211f2
Show file tree

Hide file tree

Showing 20 changed files with 2,366 additions and 70 deletions.
diff --git a/.github/workflows/storage-emulator-retry-conformance-tests.yaml b/.github/workflows/storage-emulator-retry-conformance-tests.yaml
@@ -0,0 +1,40 @@
+on:
+  push:
+    branches:
+      - main
+    paths:
+      - 'Storage/**'
+      - '.github/workflows/storage-emulator-retry-conformance-tests.yaml'
+  pull_request:
+    paths:
+      - 'Storage/**'
+      - '.github/workflows/storage-emulator-retry-conformance-tests.yaml'
+name: Run Storage Retry Conformance Tests With Emulator
+jobs:
+  test:
+    runs-on: ubuntu-20.04
+
+    services:
+      emulator:
+        image: gcr.io/cloud-devrel-public-resources/storage-testbench:v0.35.0
+        ports:
+          - 9000:9000
+
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v3
+
+      - name: Setup PHP
+        uses: shivammathur/setup-php@v2
+        with:
+          php-version: '7.4'
+
+      - name: Install dependencies
+        run: |
+          composer update --prefer-dist --no-interaction --no-suggest
+
+      - name: Run storage retry conformance tests
+        run: |
+          vendor/bin/phpunit -c Storage/phpunit-conformance.xml.dist
+        env:
+          STORAGE_EMULATOR_HOST: http://localhost:9000
diff --git a/Core/src/ExponentialBackoff.php b/Core/src/ExponentialBackoff.php
@@ -44,14 +44,32 @@ class ExponentialBackoff
      */
     private $calcDelayFunction;
 
+    /**
+     * @var callable|null
+     */
+    private $retryListener;
+
     /**
      * @param int $retries [optional] Number of retries for a failed request.
-     * @param callable $retryFunction [optional] returns bool for whether or not to retry
+     * @param callable $retryFunction [optional] returns bool for whether or not
+     *        to retry
+     * @param callable $retryListener [optional] Runs after the
+     *        $retryFunction. Unlike the $retryFunction,this function isn't
+     *        responsible to decide if a retry should happen or not, but it gives the
+     *        users flexibility to consume exception messages and add custom logic.
+     *        Function definition should match:
+     *            function (\Exception $e, int $attempt, array $arguments): array
+     *        Ex: One might want to change headers on every retry, this function can
+     *        be used to achieve such a functionality.
      */
-    public function __construct($retries = null, callable $retryFunction = null)
-    {
+    public function __construct(
+        $retries = null,
+        callable $retryFunction = null,
+        callable $retryListener = null
+    ) {
         $this->retries = $retries !== null ? (int) $retries : 3;
         $this->retryFunction = $retryFunction;
+        $this->retryListener = $retryListener;
         // @todo revisit this approach
         // @codeCoverageIgnoreStart
         $this->delayFunction = static function ($delay) {
@@ -74,7 +92,6 @@ public function execute(callable $function, array $arguments = [])
         $calcDelayFunction = $this->calcDelayFunction ?: [$this, 'calculateDelay'];
         $retryAttempt = 0;
         $exception = null;
-
         while (true) {
             try {
                 return call_user_func_array($function, $arguments);
@@ -91,6 +108,14 @@ public function execute(callable $function, array $arguments = [])
 
                 $delayFunction($calcDelayFunction($retryAttempt));
                 $retryAttempt++;
+                if ($this->retryListener) {
+                    // Developer can modify the $arguments using the retryListener
+                    // callback.
+                    call_user_func_array(
+                        $this->retryListener,
+                        [$exception, $retryAttempt, &$arguments]
+                    );
+                }
             }
         }
 

diff --git a/Core/src/RequestWrapper.php b/Core/src/RequestWrapper.php
@@ -30,6 +30,7 @@
 use Psr\Http\Message\RequestInterface;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\StreamInterface;
+use Google\ApiCore\AgentHeader;
 
 /**
  * The RequestWrapper is responsible for delivering and signing requests.
@@ -173,6 +174,11 @@ public function __construct(array $config = [])
      *     @type callable $restRetryFunction Sets the conditions for whether or
      *           not a request should attempt to retry. Function signature should
      *           match: `function (\Exception $ex) : bool`.
+     *     @type callable $restRetryListener Runs after the restRetryFunction.
+     *           This might be used to simply consume the exception and
+     *           $arguments b/w retries. This returns the new $arguments thus
+     *           allowing modification on demand for $arguments. For ex:
+     *           changing the headers in b/w retries.
      *     @type callable $restDelayFunction Executes a delay, defaults to
      *           utilizing `usleep`. Function signature should match:
      *           `function (int $delay) : void`.
@@ -189,7 +195,8 @@ public function send(RequestInterface $request, array $options = [])
         $retryOptions = $this->getRetryOptions($options);
         $backoff = new ExponentialBackoff(
             $retryOptions['retries'],
-            $retryOptions['retryFunction']
+            $retryOptions['retryFunction'],
+            $retryOptions['retryListener'],
         );
 
         if ($retryOptions['delayFunction']) {
@@ -202,7 +209,7 @@ public function send(RequestInterface $request, array $options = [])
 
         try {
             return $backoff->execute($this->httpHandler, [
-                $this->applyHeaders($request),
+                $this->applyHeaders($request, $options),
                 $this->getRequestOptions($options)
             ]);
         } catch (\Exception $ex) {
@@ -252,7 +259,7 @@ public function sendAsync(RequestInterface $request, array $options = [])
             }
 
             return $asyncHttpHandler(
-                $this->applyHeaders($request),
+                $this->applyHeaders($request, $options),
                 $this->getRequestOptions($options)
             )->then(null, function (\Exception $ex) use ($fn, $retryAttempt, $retryOptions) {
                 $shouldRetry = $retryOptions['retryFunction']($ex, $retryAttempt);
@@ -276,15 +283,29 @@ public function sendAsync(RequestInterface $request, array $options = [])
      * Applies headers to the request.
      *
      * @param RequestInterface $request A PSR-7 request.
+     * @param array $options
      * @return RequestInterface
      */
-    private function applyHeaders(RequestInterface $request)
+    private function applyHeaders(RequestInterface $request, array $options = [])
     {
         $headers = [
             'User-Agent' => 'gcloud-php/' . $this->componentVersion,
-            'x-goog-api-client' => 'gl-php/' . PHP_VERSION . ' gccl/' . $this->componentVersion,
+            AgentHeader::AGENT_HEADER_KEY => sprintf(
+                'gl-php/%s gccl/%s',
+                PHP_VERSION,
+                $this->componentVersion
+            ),
         ];
 
+        if (isset($options['retryHeaders'])) {
+            $headers[AgentHeader::AGENT_HEADER_KEY] = sprintf(
+                '%s %s',
+                $headers[AgentHeader::AGENT_HEADER_KEY],
+                implode(' ', $options['retryHeaders'])
+            );
+            unset($options['retryHeaders']);
+        }
+
         if ($this->shouldSignRequest) {
             $quotaProject = $this->quotaProject;
             $token = null;
@@ -427,6 +448,9 @@ private function getRetryOptions(array $options)
             'retryFunction' => isset($options['restRetryFunction'])
                 ? $options['restRetryFunction']
                 : $this->retryFunction,
+            'retryListener' => isset($options['restRetryListener'])
+                ? $options['restRetryListener']
+                : null,
             'delayFunction' => isset($options['restDelayFunction'])
                 ? $options['restDelayFunction']
                 : $this->delayFunction,

diff --git a/Core/src/RestTrait.php b/Core/src/RestTrait.php
@@ -90,8 +90,10 @@ public function send($resource, $method, array $options = [], $whitelisted = fal
         $requestOptions = $this->pluckArray([
             'restOptions',
             'retries',
+            'retryHeaders',
             'requestTimeout',
-            'restRetryFunction'
+            'restRetryFunction',
+            'restRetryListener',
         ], $options);
 
         try {

diff --git a/Core/src/Upload/AbstractUploader.php b/Core/src/Upload/AbstractUploader.php
@@ -103,7 +103,9 @@ public function __construct(
         $this->requestOptions = array_intersect_key($options, [
             'restOptions' => null,
             'retries' => null,
-            'requestTimeout' => null
+            'requestTimeout' => null,
+            'restRetryFunction' => null,
+            'restRetryListener' => null
         ]);
 
         $this->contentType = $options['contentType'] ?? 'application/octet-stream';

diff --git a/Core/tests/Unit/ExponentialBackoffTest.php b/Core/tests/Unit/ExponentialBackoffTest.php
@@ -160,4 +160,37 @@ public function delayProvider()
             [10, 60000000, 60000000]
         ];
     }
+
+    /**
+     * Tests whether `retryListener()` callback is
+     * properly invoked when exception occurs in the request being made.
+     */
+    public function testRetryListener()
+    {
+        $args = ['foo' => 'bar'];
+        $retryListener = function (
+            $ex,
+            $retryAttempt,
+            $arguments
+        ) {
+            self::assertEquals(0, $retryAttempt);
+            self::assertEquals('bar', $arguments[0]['foo']);
+        };
+
+        // Setting $retries to 0 so that retry doesn't happens after first
+        // failure.
+        $backoff = new ExponentialBackoff(0, null, $retryListener);
+        try {
+            $backoff->execute(
+                function () {
+                    throw new \Exception('Intentionally failing request');
+                },
+                [$args]
+            );
+        } catch (\Exception $err) {
+            // Do nothing.
+            // Catching the intentional failing call being made above:
+            // "Intentionally failing request"
+        }
+    }
 }
diff --git a/Core/tests/Unit/RequestWrapperTest.php b/Core/tests/Unit/RequestWrapperTest.php
@@ -631,6 +631,43 @@ public function testEmptyTokenThrowsException()
 
         $requestWrapper->send(new Request('GET', 'http://www.example.com'));
     }
+
+    /**
+     * This test asserts that the retry related options and callbacks are
+     * properly mapped and set in the RequestWrapper's `$requestOptions`
+     * property.
+     */
+    public function testPassingInRetryOptions()
+    {
+        $attempt = 0;
+        $retryFunctionCalled = false;
+        $retryListenerCalled = false;
+        $options = [
+            'restRetryFunction' => function () use (&$retryFunctionCalled) {
+                $retryFunctionCalled = true;
+                return true;
+            },
+            'restRetryListener' => function () use (&$retryListenerCalled) {
+                $retryListenerCalled = true;
+            },
+        ];
+        $request = new Request('GET', 'http://www.example.com');
+        $requestWrapper = new RequestWrapper([
+            'authHttpHandler' => function () {
+                return new Response(200, [], '{"access_token": "abc"}');
+            },
+            'httpHandler' => function () use (&$attempt) {
+                if ($attempt++ < 1) {
+                    throw new \Exception('retry!');
+                }
+                return new Response(200, []);
+            }
+        ]);
+        $requestWrapper->send($request, $options);
+
+        $this->assertTrue($retryFunctionCalled);
+        $this->assertTrue($retryListenerCalled);
+    }
 }
 
 //@codingStandardsIgnoreStart

diff --git a/Core/tests/Unit/Upload/ResumableUploaderTest.php b/Core/tests/Unit/Upload/ResumableUploaderTest.php
@@ -174,6 +174,49 @@ public function testResumeFinishedUpload()
         );
     }
 
+    /**
+     * This tests whether retry related options are properly set in the
+     * abstract uploader class. Since we already had these tests for
+     * ResumableUploader class which extends the AbstractUploader class,
+     * thus testing it here would be sufficient.
+     */
+    public function testRetryOptionsPassing()
+    {
+        $attempt = 0;
+        $retryFunctionCalled = false;
+        $retryListenerCalled = false;
+        $requestWrapper = new RequestWrapper([
+            'httpHandler' => function () use (&$attempt) {
+                if ($attempt++ < 1) {
+                    throw new \Exception('retry!');
+                }
+                return new Response(200, [], $this->successBody);
+            },
+            'authHttpHandler' => function () {
+                return new Response(200, [], '{"access_token": "abc"}');
+            },
+        ]);
+        $options = [
+            'restRetryFunction' => function () use (&$retryFunctionCalled) {
+                $retryFunctionCalled = true;
+                return true;
+            },
+            'restRetryListener' => function () use (&$retryListenerCalled) {
+                $retryListenerCalled = true;
+            },
+        ];
+        $uploader = new ResumableUploader(
+            $requestWrapper,
+            $this->stream,
+            'http://www.example.com',
+            $options
+        );
+        $uploader->upload();
+
+        $this->assertTrue($retryFunctionCalled);
+        $this->assertTrue($retryListenerCalled);
+    }
+
     public function testThrowsExceptionWhenAttemptsAsyncUpload()
     {
         $this->expectException(GoogleException::class);

diff --git a/Storage/composer.json b/Storage/composer.json
@@ -6,7 +6,8 @@
     "require": {
         "php": ">=7.4",
         "google/cloud-core": "^1.43",
-        "google/crc32": "^0.2.0"
+        "google/crc32": "^0.2.0",
+        "ramsey/uuid": "^4.2.3"
     },
     "require-dev": {
         "phpunit/phpunit": "^9.0",

diff --git a/Storage/phpunit-conformance.xml.dist b/Storage/phpunit-conformance.xml.dist
@@ -0,0 +1,16 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" colors="true" xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd">
+  <coverage>
+    <include>
+      <directory suffix=".php">src</directory>
+    </include>
+    <exclude>
+      <directory suffix=".php">src/V[!a-zA-Z]*</directory>
+    </exclude>
+  </coverage>
+  <testsuites>
+    <testsuite name="Conformance Test Suite">
+      <directory>tests/Conformance</directory>
+    </testsuite>
+  </testsuites>
+</phpunit>