bug #37491 [HttpClient] Fix promise behavior in HttplugClient (brentybh)

This PR was squashed before being merged into the 4.4 branch.

Discussion
----------

[HttpClient] Fix promise behavior in HttplugClient

| Q             | A
| ------------- | ---
| Branch?       | 4.4 & up <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #37488 <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->
<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Never break backward compatibility (see https://symfony.com/bc).
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too.)
 - Features and deprecations must be submitted against branch master.
-->

## The Problem
Promises have 2 important methods: `then` and `wait`.
To implement Httplug's promise interface, we built `HttplugPromise` on top of Guzzle promise.
However, when an error occurred (Httplug `NetworkException` thrown) while init the request/before actually sending the request,
`HttplugClient::sendAsyncRequest` will return a `Http\Promise\RejectedPromise`, which is a dummy implementation.

If the `then` callable returns a promise-like object, `Http\Promise\RejectedPromise` will treat it as plain value.
Guzzle promise will try to resolve the promise-like value, which is an object that has `then` method on it.

bbf3b200bc/src/Promise.php (L116)

To fix this, I edited `src/Symfony/Component/HttpClient/HttplugClient.php`.

Next, let me explain why to edit `src/Symfony/Component/HttpClient/Response/HttplugPromise.php`.
After the previous fix, when a Guzzle promise returned by the `then` callable, things will work.
However, If I return a `HttplugPromiseInterface`, it doesn't work, because Guzzle promise `wait` the return value (result)
only if it's a Guzzle promise.

bbf3b200bc/src/Promise.php (L63)

To fix this, I referenced the `wait` code of Guzzle promise and edited our `HttplugPromise`.

## How this fix make sense

So, why to return a promise from the `then` callable?
This let us change the promise chain according to current promise's result (fulfilled/rejected).

For example, we can retry an HTTP request if it failed.
Please take a look at my test code.

Commits
-------

147b6adc39 [HttpClient] Fix promise behavior in HttplugClient

src/Symfony/Component/HttpClient/HttplugClient.php

@@ -12,6 +12,7 @@
 namespace Symfony\Component\HttpClient;
 
 use GuzzleHttp\Promise\Promise as GuzzlePromise;
+use GuzzleHttp\Promise\RejectedPromise;
 use Http\Client\Exception\NetworkException;
 use Http\Client\Exception\RequestException;
 use Http\Client\HttpAsyncClient;
@@ -22,7 +23,6 @@ use Http\Message\RequestFactory;
 use Http\Message\StreamFactory;
 use Http\Message\UriFactory;
 use Http\Promise\Promise;
-use Http\Promise\RejectedPromise;
 use Nyholm\Psr7\Factory\Psr17Factory;
 use Nyholm\Psr7\Request;
 use Nyholm\Psr7\Uri;
@@ -114,7 +114,7 @@ final class HttplugClient implements HttplugInterface, HttpAsyncClient, RequestF
         try {
             $response = $this->sendPsr7Request($request, true);
         } catch (NetworkException $e) {
-            return new RejectedPromise($e);
+            return new HttplugPromise(new RejectedPromise($e));
         }
 
         $waitLoop = $this->waitLoop;

src/Symfony/Component/HttpClient/Response/HttplugPromise.php

@@ -54,6 +54,12 @@ final class HttplugPromise implements HttplugPromiseInterface
      */
     public function wait($unwrap = true)
     {
-        return $this->promise->wait($unwrap);
+        $result = $this->promise->wait($unwrap);
+
+        while ($result instanceof HttplugPromiseInterface || $result instanceof GuzzlePromiseInterface) {
+            $result = $result->wait($unwrap);
+        }
+
+        return $result;
     }
 }

src/Symfony/Component/HttpClient/Tests/HttplugClientTest.php

@@ -11,13 +11,18 @@
 
 namespace Symfony\Component\HttpClient\Tests;
 
+use GuzzleHttp\Promise\FulfilledPromise as GuzzleFulfilledPromise;
 use Http\Client\Exception\NetworkException;
 use Http\Client\Exception\RequestException;
+use Http\Promise\FulfilledPromise;
 use Http\Promise\Promise;
 use PHPUnit\Framework\TestCase;
 use Psr\Http\Message\ResponseInterface;
+use Symfony\Component\HttpClient\Exception\TransportException;
 use Symfony\Component\HttpClient\HttplugClient;
+use Symfony\Component\HttpClient\MockHttpClient;
 use Symfony\Component\HttpClient\NativeHttpClient;
+use Symfony\Component\HttpClient\Response\MockResponse;
 use Symfony\Contracts\HttpClient\Test\TestHttpServer;
 
 class HttplugClientTest extends TestCase
@@ -152,4 +157,114 @@ class HttplugClientTest extends TestCase
         $this->expectException(RequestException::class);
         $client->sendRequest($client->createRequest('BAD.METHOD', 'http://localhost:8057'));
     }
+
+    public function testRetry404()
+    {
+        $client = new HttplugClient(new NativeHttpClient());
+
+        $successCallableCalled = false;
+        $failureCallableCalled = false;
+
+        $promise = $client
+            ->sendAsyncRequest($client->createRequest('GET', 'http://localhost:8057/404'))
+            ->then(
+                function (ResponseInterface $response) use (&$successCallableCalled, $client) {
+                    $this->assertSame(404, $response->getStatusCode());
+                    $successCallableCalled = true;
+
+                    return $client->sendAsyncRequest($client->createRequest('GET', 'http://localhost:8057'));
+                },
+                function (\Exception $exception) use (&$failureCallableCalled) {
+                    $failureCallableCalled = true;
+
+                    throw $exception;
+                }
+            )
+        ;
+
+        $response = $promise->wait(true);
+
+        $this->assertTrue($successCallableCalled);
+        $this->assertFalse($failureCallableCalled);
+        $this->assertSame(200, $response->getStatusCode());
+    }
+
+    public function testRetryNetworkError()
+    {
+        $client = new HttplugClient(new NativeHttpClient());
+
+        $successCallableCalled = false;
+        $failureCallableCalled = false;
+
+        $promise = $client
+            ->sendAsyncRequest($client->createRequest('GET', 'http://localhost:8057/chunked-broken'))
+            ->then(function (ResponseInterface $response) use (&$successCallableCalled) {
+                $successCallableCalled = true;
+
+                return $response;
+            }, function (\Exception $exception) use (&$failureCallableCalled, $client) {
+                $this->assertSame(NetworkException::class, \get_class($exception));
+                $this->assertSame(TransportException::class, \get_class($exception->getPrevious()));
+                $failureCallableCalled = true;
+
+                return $client->sendAsyncRequest($client->createRequest('GET', 'http://localhost:8057'));
+            })
+        ;
+
+        $response = $promise->wait(true);
+
+        $this->assertFalse($successCallableCalled);
+        $this->assertTrue($failureCallableCalled);
+        $this->assertSame(200, $response->getStatusCode());
+    }
+
+    public function testRetryEarlierError()
+    {
+        $isFirstRequest = true;
+        $errorMessage = 'Error occurred before making the actual request.';
+
+        $client = new HttplugClient(new MockHttpClient(function () use (&$isFirstRequest, $errorMessage) {
+            if ($isFirstRequest) {
+                $isFirstRequest = false;
+                throw new TransportException($errorMessage);
+            }
+
+            return new MockResponse('OK', ['http_code' => 200]);
+        }));
+
+        $request = $client->createRequest('GET', 'http://test');
+
+        $successCallableCalled = false;
+        $failureCallableCalled = false;
+
+        $promise = $client
+            ->sendAsyncRequest($request)
+            ->then(
+                function (ResponseInterface $response) use (&$successCallableCalled) {
+                    $successCallableCalled = true;
+
+                    return $response;
+                },
+                function (\Exception $exception) use ($errorMessage, &$failureCallableCalled, $client, $request) {
+                    $this->assertSame(NetworkException::class, \get_class($exception));
+                    $this->assertSame($errorMessage, $exception->getMessage());
+                    $failureCallableCalled = true;
+
+                    // Ensure arbitrary levels of promises work.
+                    return (new FulfilledPromise(null))->then(function () use ($client, $request) {
+                        return (new GuzzleFulfilledPromise(null))->then(function () use ($client, $request) {
+                            return $client->sendAsyncRequest($request);
+                        });
+                    });
+                }
+            )
+        ;
+
+        $response = $promise->wait(true);
+
+        $this->assertFalse($successCallableCalled);
+        $this->assertTrue($failureCallableCalled);
+        $this->assertSame(200, $response->getStatusCode());
+        $this->assertSame('OK', (string) $response->getBody());
+    }
 }