Improve timeout error messages

Author: clue

Diff for: src/TimeoutConnector.php

@@ -4,6 +4,7 @@
 
 use React\EventLoop\LoopInterface;
 use React\Promise\Timer;
+use React\Promise\Timer\TimeoutException;
 
 final class TimeoutConnector implements ConnectorInterface
 {
@@ -20,6 +21,30 @@ public function __construct(ConnectorInterface $connector, $timeout, LoopInterfa
 
     public function connect($uri)
     {
-        return Timer\timeout($this->connector->connect($uri), $this->timeout, $this->loop);
+        return Timer\timeout($this->connector->connect($uri), $this->timeout, $this->loop)->then(null, self::handler($uri));
+    }
+
+    /**
+     * Creates a static rejection handler that reports a proper error message in case of a timeout.
+     *
+     * This uses a private static helper method to ensure this closure is not
+     * bound to this instance and the exception trace does not include a
+     * reference to this instance and its connector stack as a result.
+     *
+     * @param string $uri
+     * @return callable
+     */
+    private static function handler($uri)
+    {
+        return function (\Exception $e) use ($uri) {
+            if ($e instanceof TimeoutException) {
+                throw new \RuntimeException(
+                    'Connection to ' . $uri . ' timed out after ' . $e->getTimeout() . ' seconds',
+                    \defined('SOCKET_ETIMEDOUT') ? \SOCKET_ETIMEDOUT : 0
+                );
+            }
+
+            throw $e;
+        };
     }
 }

Diff for: tests/DnsConnectorTest.php

@@ -195,9 +195,6 @@ public function testCancelDuringTcpConnectionCancelsTcpConnectionWithTcpRejectio
         $this->throwRejection($promise);
     }
 
-    /**
-     * @requires PHP 7
-     */
     public function testRejectionDuringDnsLookupShouldNotCreateAnyGarbageReferences()
     {
         if (class_exists('React\Promise\When')) {
@@ -217,9 +214,6 @@ public function testRejectionDuringDnsLookupShouldNotCreateAnyGarbageReferences(
         $this->assertEquals(0, gc_collect_cycles());
     }
 
-    /**
-     * @requires PHP 7
-     */
     public function testRejectionAfterDnsLookupShouldNotCreateAnyGarbageReferences()
     {
         if (class_exists('React\Promise\When')) {
@@ -242,9 +236,6 @@ public function testRejectionAfterDnsLookupShouldNotCreateAnyGarbageReferences()
         $this->assertEquals(0, gc_collect_cycles());
     }
 
-    /**
-     * @requires PHP 7
-     */
     public function testRejectionAfterDnsLookupShouldNotCreateAnyGarbageReferencesAgain()
     {
         if (class_exists('React\Promise\When')) {
@@ -270,9 +261,6 @@ public function testRejectionAfterDnsLookupShouldNotCreateAnyGarbageReferencesAg
         $this->assertEquals(0, gc_collect_cycles());
     }
 
-    /**
-     * @requires PHP 7
-     */
     public function testCancelDuringDnsLookupShouldNotCreateAnyGarbageReferences()
     {
         if (class_exists('React\Promise\When')) {
@@ -295,9 +283,6 @@ public function testCancelDuringDnsLookupShouldNotCreateAnyGarbageReferences()
         $this->assertEquals(0, gc_collect_cycles());
     }
 
-    /**
-     * @requires PHP 7
-     */
     public function testCancelDuringTcpConnectionShouldNotCreateAnyGarbageReferences()
     {
         if (class_exists('React\Promise\When')) {

Diff for: tests/IntegrationTest.php

@@ -181,9 +181,6 @@ function ($e) use (&$wait) {
         $this->assertEquals(0, gc_collect_cycles());
     }
 
-    /**
-     * @requires PHP 7
-     */
     public function testWaitingForConnectionTimeoutDuringDnsLookupShouldNotCreateAnyGarbageReferences()
     {
         if (class_exists('React\Promise\When')) {
@@ -217,9 +214,6 @@ function ($e) use (&$wait) {
         $this->assertEquals(0, gc_collect_cycles());
     }
 
-    /**
-     * @requires PHP 7
-     */
     public function testWaitingForConnectionTimeoutDuringTcpConnectionShouldNotCreateAnyGarbageReferences()
     {
         if (class_exists('React\Promise\When')) {

Diff for: tests/TimeoutConnectorTest.php

@@ -2,13 +2,19 @@
 
 namespace React\Tests\Socket;
 
+use Clue\React\Block;
 use React\Socket\TimeoutConnector;
 use React\Promise;
 use React\EventLoop\Factory;
+use React\Promise\Deferred;
 
 class TimeoutConnectorTest extends TestCase
 {
-    public function testRejectsOnTimeout()
+    /**
+     * @expectedException RuntimeException
+     * @expectedExceptionMessage Connection to google.com:80 timed out after 0.01 seconds
+     */
+    public function testRejectsWithTimeoutReasonOnTimeout()
     {
         $promise = new Promise\Promise(function () { });
 
@@ -19,17 +25,16 @@ public function testRejectsOnTimeout()
 
         $timeout = new TimeoutConnector($connector, 0.01, $loop);
 
-        $timeout->connect('google.com:80')->then(
-            $this->expectCallableNever(),
-            $this->expectCallableOnce()
-        );
-
-        $loop->run();
+        Block\await($timeout->connect('google.com:80'), $loop);
     }
 
-    public function testRejectsWhenConnectorRejects()
+    /**
+     * @expectedException RuntimeException
+     * @expectedExceptionMessage Failed
+     */
+    public function testRejectsWithOriginalReasonWhenConnectorRejects()
     {
-        $promise = Promise\reject(new \RuntimeException());
+        $promise = Promise\reject(new \RuntimeException('Failed'));
 
         $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
         $connector->expects($this->once())->method('connect')->with('google.com:80')->will($this->returnValue($promise));
@@ -38,12 +43,7 @@ public function testRejectsWhenConnectorRejects()
 
         $timeout = new TimeoutConnector($connector, 5.0, $loop);
 
-        $timeout->connect('google.com:80')->then(
-            $this->expectCallableNever(),
-            $this->expectCallableOnce()
-        );
-
-        $loop->run();
+        Block\await($timeout->connect('google.com:80'), $loop);
     }
 
     public function testResolvesWhenConnectorResolves()
@@ -100,4 +100,51 @@ public function testCancelsPendingPromiseOnCancel()
 
         $out->then($this->expectCallableNever(), $this->expectCallableOnce());
     }
+
+    public function testRejectionDuringConnectionShouldNotCreateAnyGarbageReferences()
+    {
+        if (class_exists('React\Promise\When')) {
+            $this->markTestSkipped('Not supported on legacy Promise v1 API');
+        }
+
+        gc_collect_cycles();
+
+        $connection = new Deferred();
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+        $connector->expects($this->once())->method('connect')->with('example.com:80')->willReturn($connection->promise());
+
+        $loop = Factory::create();
+        $timeout = new TimeoutConnector($connector, 0.01, $loop);
+
+        $promise = $timeout->connect('example.com:80');
+        $connection->reject(new \RuntimeException('Connection failed'));
+        unset($promise, $connection);
+
+        $this->assertEquals(0, gc_collect_cycles());
+    }
+
+    public function testRejectionDueToTimeoutShouldNotCreateAnyGarbageReferences()
+    {
+        if (class_exists('React\Promise\When')) {
+            $this->markTestSkipped('Not supported on legacy Promise v1 API');
+        }
+
+        gc_collect_cycles();
+
+        $connection = new Deferred(function () {
+            throw new \RuntimeException('Connection cancelled');
+        });
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+        $connector->expects($this->once())->method('connect')->with('example.com:80')->willReturn($connection->promise());
+
+        $loop = Factory::create();
+        $timeout = new TimeoutConnector($connector, 0, $loop);
+
+        $promise = $timeout->connect('example.com:80');
+
+        $loop->run();
+        unset($promise, $connection);
+
+        $this->assertEquals(0, gc_collect_cycles());
+    }
 }