bug #36766 [HttpClient] add TimeoutExceptionInterface (nicolas-grekas)

This PR was merged into the 5.1-dev branch. Discussion ---------- [HttpClient] add TimeoutExceptionInterface | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | yes | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - This is fixing a design issue: unlike any other `TransportExceptionInterface`, timeouts are not fatal transport errors. For this reason, we must allow one to identify them. ~This PR also marks exception classes as `@internal`, to encourage ppl to catch them using their interface and not their class name.~ this would revert #30549 Commits ------- ab8eca0 [HttpClient] add TimeoutExceptionInterface
symfony · May 10, 2020 · 3acc28f · 3acc28f
2 parents 2ed6a0d + ab8eca0
commit 3acc28f
Show file tree

Hide file tree

Showing 8 changed files with 79 additions and 36 deletions.
diff --git a/src/Symfony/Component/HttpClient/Chunk/ErrorChunk.php b/src/Symfony/Component/HttpClient/Chunk/ErrorChunk.php
@@ -11,6 +11,7 @@
 
 namespace Symfony\Component\HttpClient\Chunk;
 
+use Symfony\Component\HttpClient\Exception\TimeoutException;
 use Symfony\Component\HttpClient\Exception\TransportException;
 use Symfony\Contracts\HttpClient\ChunkInterface;
 
@@ -61,7 +62,7 @@ public function isTimeout(): bool
     public function isFirst(): bool
     {
         $this->didThrow = true;
-        throw new TransportException($this->errorMessage, 0, $this->error);
+        throw null !== $this->error ? new TransportException($this->errorMessage, 0, $this->error) : new TimeoutException($this->errorMessage);
     }
 
     /**
@@ -70,7 +71,7 @@ public function isFirst(): bool
     public function isLast(): bool
     {
         $this->didThrow = true;
-        throw new TransportException($this->errorMessage, 0, $this->error);
+        throw null !== $this->error ? new TransportException($this->errorMessage, 0, $this->error) : new TimeoutException($this->errorMessage);
     }
 
     /**
@@ -79,7 +80,7 @@ public function isLast(): bool
     public function getInformationalStatus(): ?array
     {
         $this->didThrow = true;
-        throw new TransportException($this->errorMessage, 0, $this->error);
+        throw null !== $this->error ? new TransportException($this->errorMessage, 0, $this->error) : new TimeoutException($this->errorMessage);
     }
 
     /**
@@ -88,7 +89,7 @@ public function getInformationalStatus(): ?array
     public function getContent(): string
     {
         $this->didThrow = true;
-        throw new TransportException($this->errorMessage, 0, $this->error);
+        throw null !== $this->error ? new TransportException($this->errorMessage, 0, $this->error) : new TimeoutException($this->errorMessage);
     }
 
     /**
@@ -119,7 +120,7 @@ public function __destruct()
     {
         if (!$this->didThrow) {
             $this->didThrow = true;
-            throw new TransportException($this->errorMessage, 0, $this->error);
+            throw null !== $this->error ? new TransportException($this->errorMessage, 0, $this->error) : new TimeoutException($this->errorMessage);
         }
     }
 }
diff --git a/src/Symfony/Component/HttpClient/Exception/TimeoutException.php b/src/Symfony/Component/HttpClient/Exception/TimeoutException.php
@@ -0,0 +1,21 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpClient\Exception;
+
+use Symfony\Contracts\HttpClient\Exception\TimeoutExceptionInterface;
+
+/**
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+final class TimeoutException extends TransportException implements TimeoutExceptionInterface
+{
+}
diff --git a/src/Symfony/Component/HttpClient/Exception/TransportException.php b/src/Symfony/Component/HttpClient/Exception/TransportException.php
@@ -16,6 +16,6 @@
 /**
  * @author Nicolas Grekas <p@tchwork.com>
  */
-final class TransportException extends \RuntimeException implements TransportExceptionInterface
+class TransportException extends \RuntimeException implements TransportExceptionInterface
 {
 }
diff --git a/src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php b/src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php
@@ -12,7 +12,6 @@
 namespace Symfony\Component\HttpClient\Tests;
 
 use Symfony\Component\HttpClient\Exception\ClientException;
-use Symfony\Component\HttpClient\Exception\TransportException;
 use Symfony\Component\HttpClient\Response\StreamWrapper;
 use Symfony\Component\Process\Exception\ProcessFailedException;
 use Symfony\Component\Process\Process;
@@ -105,32 +104,6 @@ public function testNonBlockingStream()
         $this->assertTrue(feof($stream));
     }
 
-    public function testTimeoutIsNotAFatalError()
-    {
-        $client = $this->getHttpClient(__FUNCTION__);
-        $response = $client->request('GET', 'http://localhost:8057/timeout-body', [
-            'timeout' => 0.25,
-        ]);
-
-        try {
-            $response->getContent();
-            $this->fail(TransportException::class.' expected');
-        } catch (TransportException $e) {
-        }
-
-        for ($i = 0; $i < 10; ++$i) {
-            try {
-                $this->assertSame('<1><2>', $response->getContent());
-                break;
-            } catch (TransportException $e) {
-            }
-        }
-
-        if (10 === $i) {
-            throw $e;
-        }
-    }
-
     public function testResponseStreamRewind()
     {
         $client = $this->getHttpClient(__FUNCTION__);

diff --git a/src/Symfony/Component/HttpClient/composer.json b/src/Symfony/Component/HttpClient/composer.json
@@ -23,7 +23,7 @@
     "require": {
         "php": "^7.2.5",
         "psr/log": "^1.0",
-        "symfony/http-client-contracts": "^1.1.8|^2",
+        "symfony/http-client-contracts": "^2.1.1",
         "symfony/polyfill-php73": "^1.11",
         "symfony/polyfill-php80": "^1.15",
         "symfony/service-contracts": "^1.0|^2"

diff --git a/src/Symfony/Contracts/HttpClient/Exception/TimeoutExceptionInterface.php b/src/Symfony/Contracts/HttpClient/Exception/TimeoutExceptionInterface.php
@@ -0,0 +1,21 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Contracts\HttpClient\Exception;
+
+/**
+ * When an idle timeout occurs.
+ *
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+interface TimeoutExceptionInterface extends TransportExceptionInterface
+{
+}
diff --git a/src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php b/src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php
@@ -116,7 +116,7 @@
         echo '<1>';
         @ob_flush();
         flush();
-        usleep(500000);
+        usleep(600000);
         echo '<2>';
         exit;
 

diff --git a/src/Symfony/Contracts/HttpClient/Test/HttpClientTestCase.php b/src/Symfony/Contracts/HttpClient/Test/HttpClientTestCase.php
@@ -14,6 +14,7 @@
 use PHPUnit\Framework\TestCase;
 use Symfony\Contracts\HttpClient\Exception\ClientExceptionInterface;
 use Symfony\Contracts\HttpClient\Exception\RedirectionExceptionInterface;
+use Symfony\Contracts\HttpClient\Exception\TimeoutExceptionInterface;
 use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
 use Symfony\Contracts\HttpClient\HttpClientInterface;
 
@@ -739,9 +740,35 @@ public function testTimeoutOnAccess()
         $response->getHeaders();
     }
 
-    public function testTimeoutOnStream()
+    public function testTimeoutIsNotAFatalError()
     {
         usleep(300000); // wait for the previous test to release the server
+        $client = $this->getHttpClient(__FUNCTION__);
+        $response = $client->request('GET', 'http://localhost:8057/timeout-body', [
+            'timeout' => 0.3,
+        ]);
+
+        try {
+            $response->getContent();
+            $this->fail(TimeoutExceptionInterface::class.' expected');
+        } catch (TimeoutExceptionInterface $e) {
+        }
+
+        for ($i = 0; $i < 10; ++$i) {
+            try {
+                $this->assertSame('<1><2>', $response->getContent());
+                break;
+            } catch (TimeoutExceptionInterface $e) {
+            }
+        }
+
+        if (10 === $i) {
+            throw $e;
+        }
+    }
+
+    public function testTimeoutOnStream()
+    {
         $client = $this->getHttpClient(__FUNCTION__);
         $response = $client->request('GET', 'http://localhost:8057/timeout-body');