StorageClient::send does not use the client's retryStrategy correctly

[jayktaylor]

Environment details

• OS: Debian GNU/Linux 13 (trixie)
• PHP version: 8.2.32
• Package name and version: google/cloud-storage 1.50 (though the relevant code is the same on master)

Steps to reproduce

1. Create a StorageClient with retryStrategy set to StorageClient::RETRY_ALWAYS
2. Make a non-idempotent API call
3. Encounter an API error. No retry will be made.

Code example

$client = new StorageClient( [
	'retryStrategy' => StorageClient::RETRY_ALWAYS
] );
$bucket = $client->bucket( 'test' );
$bucket->object( 'test.jpg' )->delete();

Explanation

When you create a StorageClient with a retryStrategy set, it also sets it as a property on a Rest class instance (see here).

When you call a method on StorageClient, or a StorageObject, it calls Rest::send. This method prepares options to pass into google/cloud-core's RestTrait::send (and then on to RequestWrapper::send). One of the options is the restRetryFunction:

        $options['restRetryFunction'] = $this->restRetryFunction ?? $this->getRestRetryFunction(
            $retryResource,
            $method,
            $options
        );

If $this->restRetryFunction is null (which it would be unless you passed restRetryFunction as an option when instantiating the StorageClient), then we create a new default retry function using RetryTrait::getRestRetryFunction. $options is passed into this method, which are the options provided to the individual call (e.g StorageObject->delete( [ 'optName' => 'optValue' ] ), but do not include the StorageClient options.

So the alarm bells start ringing - this default retry function checks the retryStrategy passed into the options - again, only the options for the individual call, and do this:

        $retryStrategy = isset($args['retryStrategy']) ?
            $args['retryStrategy'] :
            StorageClient::RETRY_IDEMPOTENT;

So, unless retryStrategy is explicitly passed to the individual calls that make the Rest requests, this will always fall back to StorageClient::RETRY_IDEMPOTENT. StorageClient::retryStrategy is ignored.

Separately, for some reason, StorageClient::retryStrategy is passed on to google/cloud-core's classes, but they don't care about it and do nothing with it.

Most of the work around this was added or changed in b6de1e9. That commit included tests, including one to make sure that the RETRY_ALWAYS strategy works when added to StorageClient. However, that test includes this:

$this->assertInstanceOf(Bucket::class, $client->createBucket('myBucket'));

which makes sure that when creating a bucket, the rest handler retries, and then eventually returns a Bucket object. But this operation will always retry, because creating a bucket is an idempotent operation. So this test would not have failed, and this issue would not have been spotted. The test should probably be changed to use a non-idempotent operation.