Fix exception thrown twice in waiter (#928)

Author: jderusse

src/Waiter.php

@@ -49,6 +49,11 @@ class Waiter
      */
     private $finalState;
 
+    /**
+     * @var bool
+     */
+    private $resolved = false;
+
     public function __construct(Response $response, AbstractApi $awsClient, $request)
     {
         $this->response = $response;
@@ -58,7 +63,9 @@ public function __construct(Response $response, AbstractApi $awsClient, $request
 
     public function __destruct()
     {
-        $this->resolve();
+        if (!$this->resolved) {
+            $this->resolve();
+        }
     }
 
     final public function isSuccess(): bool
@@ -91,10 +98,12 @@ final public function getState(): string
             $exception = null;
         } catch (HttpException $exception) {
             // use $exception later
+        } finally {
+            $this->resolved = true;
+            $this->needRefresh = true;
         }
 
         $state = $this->extractState($this->response, $exception);
-        $this->needRefresh = true;
 
         switch ($state) {
             case self::STATE_SUCCESS:
@@ -126,6 +135,8 @@ final public function resolve(?float $timeout = null): bool
             return $this->response->resolve($timeout);
         } catch (HttpException $exception) {
             return true;
+        } finally {
+            $this->resolved = true;
         }
     }
 
@@ -148,6 +159,7 @@ final public function cancel(): void
     {
         $this->response->cancel();
         $this->needRefresh = true;
+        $this->resolved = true;
     }
 
     /**
@@ -161,18 +173,27 @@ final public function cancel(): void
      */
     final public function wait(float $timeout = null, float $delay = null): bool
     {
+        if (null !== $this->finalState) {
+            return true;
+        }
+
         $timeout = $timeout ?? static::WAIT_TIMEOUT;
         $delay = $delay ?? static::WAIT_DELAY;
 
         $start = \microtime(true);
         while (true) {
+            if ($this->needRefresh) {
+                $this->stealResponse($this->refreshState());
+            }
+
             // If request times out
             if (!$this->resolve($timeout - (\microtime(true) - $start))) {
                 break;
             }
 
+            $this->getState();
             // If we reached a final state
-            if (\in_array($this->getState(), [self::STATE_SUCCESS, self::STATE_FAILURE])) {
+            if ($this->finalState) {
                 return true;
             }
 
@@ -182,7 +203,6 @@ final public function wait(float $timeout = null, float $delay = null): bool
             }
 
             \usleep((int) ceil($delay * 1000000));
-            $this->stealResponse($this->refreshState());
         }
 
         return false;
@@ -201,6 +221,8 @@ protected function refreshState(): Waiter
     private function stealResponse(self $waiter): void
     {
         $this->response = $waiter->response;
+        $this->resolved = $waiter->resolved;
+        $waiter->resolved = true;
         $this->needRefresh = false;
     }
 }

tests/Integration/WaiterTest.php

@@ -0,0 +1,83 @@
+<?php
+
+declare(strict_types=1);
+
+namespace AsyncAws\Core\Tests\Integration;
+
+use AsyncAws\Core\Exception\Http\NetworkException;
+use AsyncAws\S3\S3Client;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\HttpClient\HttpClient;
+
+class WaiterTest extends TestCase
+{
+    public function testWaiterException(): void
+    {
+        if (!\class_exists(S3Client::class)) {
+            self::markTestSkipped('This test needs a client with waiter endpoints');
+        }
+
+        $client = new S3Client(['endpoint' => 'http://127.0.0.1:10'], null, HttpClient::create(['timeout' => 5]));
+        $result = $client->bucketExists(['Bucket' => 'test']);
+
+        $this->expectException(NetworkException::class);
+        $result->isSuccess();
+    }
+
+    public function testWaiterThrowOnce(): void
+    {
+        if (!\class_exists(S3Client::class)) {
+            self::markTestSkipped('This test needs a client with waiter endpoints');
+        }
+
+        $client = new S3Client(['endpoint' => 'http://127.0.0.1:10'], null, HttpClient::create(['timeout' => 5]));
+        $result = $client->bucketExists(['Bucket' => 'test']);
+
+        try {
+            $result->isSuccess();
+            self::fail('An exception should have been triggered');
+        } catch (NetworkException $e) {
+        }
+
+        unset($result);
+        self::expectNotToPerformAssertions();
+    }
+
+    public function testWaiterThrowOnRetry(): void
+    {
+        if (!\class_exists(S3Client::class)) {
+            self::markTestSkipped('This test needs a client with waiter endpoints');
+        }
+
+        $client = new S3Client(['endpoint' => 'http://127.0.0.1:10'], null, HttpClient::create(['timeout' => 5]));
+        $result = $client->bucketExists(['Bucket' => 'test']);
+
+        try {
+            $result->isSuccess();
+            self::fail('An exception should have been triggered');
+        } catch (NetworkException $e) {
+        }
+
+        $this->expectException(NetworkException::class);
+        $result->isSuccess();
+    }
+
+    public function testWaiterThrowOnWait(): void
+    {
+        if (!\class_exists(S3Client::class)) {
+            self::markTestSkipped('This test needs a client with waiter endpoints');
+        }
+
+        $client = new S3Client(['endpoint' => 'http://127.0.0.1:10'], null, HttpClient::create(['timeout' => 5]));
+        $result = $client->bucketExists(['Bucket' => 'test']);
+
+        try {
+            $result->isSuccess();
+            self::fail('An exception should have been triggered');
+        } catch (NetworkException $e) {
+        }
+
+        $this->expectException(NetworkException::class);
+        $result->wait();
+    }
+}