Merge pull request #231 from clue-labs/eyeballs-errors

jsor · web-flow · commit e88fcf30f7ad · 2020-05-12T21:19:10.000+02:00
Improve error reporting when TCP/IP connection fails (happy eyeballs)
diff --git a/src/HappyEyeBallsConnectionBuilder.php b/src/HappyEyeBallsConnectionBuilder.php
@@ -71,11 +71,10 @@ public function connect()
 
                     $that->mixIpsIntoConnectQueue($ips);
 
-                    if ($that->nextAttemptTimer instanceof TimerInterface) {
-                        return;
+                    // start next connection attempt if not already awaiting next
+                    if ($that->nextAttemptTimer === null && $that->connectQueue) {
+                        $that->check($resolve, $reject);
                     }
-
-                    $that->check($resolve, $reject);
                 };
             };
 
@@ -124,6 +123,12 @@ public function resolve($type, $reject)
             unset($that->resolverPromises[$type]);
             $that->resolved[$type] = true;
 
+            // cancel next attempt timer when there are no more IPs to connect to anymore
+            if ($that->nextAttemptTimer !== null && !$that->connectQueue) {
+                $that->loop->cancelTimer($that->nextAttemptTimer);
+                $that->nextAttemptTimer = null;
+            }
+
             if ($that->hasBeenResolved() && $that->ipsCount === 0) {
                 $that->resolverPromises = null;
                 $reject(new \RuntimeException('Connection to ' . $that->uri . ' failed during DNS lookup: ' . $e->getMessage()));
@@ -138,15 +143,6 @@ public function resolve($type, $reject)
      */
     public function check($resolve, $reject)
     {
-        if (\count($this->connectQueue) === 0 && $this->resolved[Message::TYPE_A] === true && $this->resolved[Message::TYPE_AAAA] === true && $this->nextAttemptTimer instanceof TimerInterface) {
-            $this->loop->cancelTimer($this->nextAttemptTimer);
-            $this->nextAttemptTimer = null;
-        }
-
-        if (\count($this->connectQueue) === 0) {
-            return;
-        }
-
         $ip = \array_shift($this->connectQueue);
 
         $that = $this;
@@ -156,7 +152,7 @@ public function check($resolve, $reject)
             $that->cleanUp();
 
             $resolve($connection);
-        }, function () use ($that, $ip, $reject) {
+        }, function (\Exception $e) use ($that, $ip, $reject) {
             unset($that->connectionPromises[$ip]);
 
             $that->failureCount++;
@@ -168,19 +164,19 @@ public function check($resolve, $reject)
             if ($that->ipsCount === $that->failureCount) {
                 $that->cleanUp();
 
-                $reject(new \RuntimeException('All attempts to connect to "' . $that->host . '" have failed'));
+                $reject(new \RuntimeException('Connection to ' . $that->uri . ' failed: ' . $e->getMessage()));
             }
         });
 
-        /**
-         * As long as we haven't connected yet keep popping an IP address of the connect queue until one of them
-         * succeeds or they all fail. We will wait 100ms between connection attempts as per RFC.
-         *
-         * @link https://tools.ietf.org/html/rfc8305#section-5
-         */
-        if ((\count($this->connectQueue) > 0 || ($this->resolved[Message::TYPE_A] === false || $this->resolved[Message::TYPE_AAAA] === false)) && $this->nextAttemptTimer === null) {
-            $this->nextAttemptTimer = $this->loop->addPeriodicTimer(self::CONNECTION_ATTEMPT_DELAY, function () use ($that, $resolve, $reject) {
-                $that->check($resolve, $reject);
+        // Allow next connection attempt in 100ms: https://tools.ietf.org/html/rfc8305#section-5
+        // Only start timer when more IPs are queued or when DNS query is still pending (might add more IPs)
+        if (\count($this->connectQueue) > 0 || $this->resolved[Message::TYPE_A] === false || $this->resolved[Message::TYPE_AAAA] === false) {
+            $this->nextAttemptTimer = $this->loop->addTimer(self::CONNECTION_ATTEMPT_DELAY, function () use ($that, $resolve, $reject) {
+                $that->nextAttemptTimer = null;
+
+                if ($that->connectQueue) {
+                    $that->check($resolve, $reject);
+                }
             });
         }
     }
diff --git a/tests/HappyEyeBallsConnectionBuilderTest.php b/tests/HappyEyeBallsConnectionBuilderTest.php
@@ -65,10 +65,10 @@ public function testConnectWillRejectWhenBothDnsLookupsReject()
         $this->assertEquals('Connection to tcp://reactphp.org:80 failed during DNS lookup: DNS lookup error', $exception->getMessage());
     }
 
-    public function testConnectWillStartTimerWhenIpv4ResolvesAndIpv6IsPending()
+    public function testConnectWillStartDelayTimerWhenIpv4ResolvesAndIpv6IsPending()
     {
         $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
-        $loop->expects($this->once())->method('addTimer');
+        $loop->expects($this->once())->method('addTimer')->with(0.05, $this->anything());
         $loop->expects($this->never())->method('cancelTimer');
 
         $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
@@ -92,10 +92,76 @@ public function testConnectWillStartTimerWhenIpv4ResolvesAndIpv6IsPending()
         $builder->connect();
     }
 
-    public function testConnectWillStartConnectingWithoutTimerWhenIpv6ResolvesAndIpv4IsPending()
+    public function testConnectWillStartConnectingWithAttemptTimerButWithoutResolutionTimerWhenIpv6ResolvesAndIpv4IsPending()
     {
         $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
-        $loop->expects($this->never())->method('addTimer');
+        $loop->expects($this->once())->method('addTimer')->with(0.1, $this->anything());
+        $loop->expects($this->never())->method('cancelTimer');
+
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+        $connector->expects($this->once())->method('connect')->with('tcp://[::1]:80?hostname=reactphp.org')->willReturn(new Promise(function () { }));
+
+        $resolver = $this->getMockBuilder('React\Dns\Resolver\ResolverInterface')->getMock();
+        $resolver->expects($this->exactly(2))->method('resolveAll')->withConsecutive(
+            array('reactphp.org', Message::TYPE_AAAA),
+            array('reactphp.org', Message::TYPE_A)
+        )->willReturnOnConsecutiveCalls(
+            \React\Promise\resolve(array('::1')),
+            new Promise(function () { })
+        );
+
+        $uri = 'tcp://reactphp.org:80';
+        $host = 'reactphp.org';
+        $parts = parse_url($uri);
+
+        $builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
+
+        $builder->connect();
+    }
+
+    public function testConnectWillStartConnectingAndWillStartNextConnectionWithNewAttemptTimerWhenNextAttemptTimerFiresWithIpv4StillPending()
+    {
+        $timer = null;
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+        $loop->expects($this->exactly(2))->method('addTimer')->with(0.1, $this->callback(function ($cb) use (&$timer) {
+            $timer = $cb;
+            return true;
+        }));
+        $loop->expects($this->never())->method('cancelTimer');
+
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+        $connector->expects($this->exactly(2))->method('connect')->willReturn(new Promise(function () { }));
+
+        $resolver = $this->getMockBuilder('React\Dns\Resolver\ResolverInterface')->getMock();
+        $resolver->expects($this->exactly(2))->method('resolveAll')->withConsecutive(
+            array('reactphp.org', Message::TYPE_AAAA),
+            array('reactphp.org', Message::TYPE_A)
+        )->willReturnOnConsecutiveCalls(
+            \React\Promise\resolve(array('::1', '::2')),
+            new Promise(function () { })
+        );
+
+        $uri = 'tcp://reactphp.org:80';
+        $host = 'reactphp.org';
+        $parts = parse_url($uri);
+
+        $builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
+
+        $builder->connect();
+
+        $this->assertNotNull($timer);
+        $timer();
+    }
+
+    public function testConnectWillStartConnectingAndWillDoNothingWhenNextAttemptTimerFiresWithNoOtherIps()
+    {
+        $timer = null;
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+        $loop->expects($this->once())->method('addTimer')->with(0.1, $this->callback(function ($cb) use (&$timer) {
+            $timer = $cb;
+            return true;
+        }));
+        $loop->expects($this->never())->method('cancelTimer');
 
         $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
         $connector->expects($this->once())->method('connect')->with('tcp://[::1]:80?hostname=reactphp.org')->willReturn(new Promise(function () { }));
@@ -116,15 +182,93 @@ public function testConnectWillStartConnectingWithoutTimerWhenIpv6ResolvesAndIpv
         $builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
 
         $builder->connect();
+
+        $this->assertNotNull($timer);
+        $timer();
     }
 
-    public function testConnectWillStartTimerAndCancelTimerWhenIpv4ResolvesAndIpv6ResolvesAfterwardsAndStartConnectingToIpv6()
+    public function testConnectWillStartConnectingWithAttemptTimerButWithoutResolutionTimerWhenIpv6ResolvesAndWillCancelAttemptTimerWhenIpv4Rejects()
     {
         $timer = $this->getMockBuilder('React\EventLoop\TimerInterface')->getMock();
         $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
-        $loop->expects($this->once())->method('addTimer')->willReturn($timer);
+        $loop->expects($this->once())->method('addTimer')->with(0.1, $this->anything())->willReturn($timer);
         $loop->expects($this->once())->method('cancelTimer')->with($timer);
-        $loop->expects($this->once())->method('addPeriodicTimer')->willReturn($this->getMockBuilder('React\EventLoop\TimerInterface')->getMock());
+
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+        $connector->expects($this->once())->method('connect')->with('tcp://[::1]:80?hostname=reactphp.org')->willReturn(new Promise(function () { }));
+
+        $deferred = new Deferred();
+        $resolver = $this->getMockBuilder('React\Dns\Resolver\ResolverInterface')->getMock();
+        $resolver->expects($this->exactly(2))->method('resolveAll')->withConsecutive(
+            array('reactphp.org', Message::TYPE_AAAA),
+            array('reactphp.org', Message::TYPE_A)
+        )->willReturnOnConsecutiveCalls(
+            \React\Promise\resolve(array('::1')),
+            $deferred->promise()
+        );
+
+        $uri = 'tcp://reactphp.org:80';
+        $host = 'reactphp.org';
+        $parts = parse_url($uri);
+
+        $builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
+
+        $builder->connect();
+        $deferred->reject(new \RuntimeException());
+    }
+
+    public function testConnectWillStartConnectingAndWillStartNextConnectionWithoutNewAttemptTimerWhenNextAttemptTimerFiresAfterIpv4Rejected()
+    {
+        $timer = null;
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+        $loop->expects($this->once())->method('addTimer')->with(0.1, $this->callback(function ($cb) use (&$timer) {
+            $timer = $cb;
+            return true;
+        }));
+        $loop->expects($this->never())->method('cancelTimer');
+
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+        $connector->expects($this->exactly(2))->method('connect')->willReturn(new Promise(function () { }));
+
+        $deferred = new Deferred();
+        $resolver = $this->getMockBuilder('React\Dns\Resolver\ResolverInterface')->getMock();
+        $resolver->expects($this->exactly(2))->method('resolveAll')->withConsecutive(
+            array('reactphp.org', Message::TYPE_AAAA),
+            array('reactphp.org', Message::TYPE_A)
+        )->willReturnOnConsecutiveCalls(
+            \React\Promise\resolve(array('::1', '::2')),
+            $deferred->promise()
+        );
+
+        $uri = 'tcp://reactphp.org:80';
+        $host = 'reactphp.org';
+        $parts = parse_url($uri);
+
+        $builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
+
+        $builder->connect();
+        $deferred->reject(new \RuntimeException());
+
+        $this->assertNotNull($timer);
+        $timer();
+    }
+
+    public function testConnectWillStartAndCancelResolutionTimerAndStartAttemptTimerWhenIpv4ResolvesAndIpv6ResolvesAfterwardsAndStartConnectingToIpv6()
+    {
+        $timerDelay = $this->getMockBuilder('React\EventLoop\TimerInterface')->getMock();
+        $timerAttempt = $this->getMockBuilder('React\EventLoop\TimerInterface')->getMock();
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+        $loop->expects($this->exactly(2))->method('addTimer')->withConsecutive(
+            array(
+                0.05,
+                $this->anything()
+            ),
+            array(
+                0.1,
+                $this->anything()
+            )
+        )->willReturnOnConsecutiveCalls($timerDelay, $timerAttempt);
+        $loop->expects($this->once())->method('cancelTimer')->with($timerDelay);
 
         $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
         $connector->expects($this->once())->method('connect')->with('tcp://[::1]:80?hostname=reactphp.org')->willReturn(new Promise(function () { }));
@@ -149,6 +293,44 @@ public function testConnectWillStartTimerAndCancelTimerWhenIpv4ResolvesAndIpv6Re
         $deferred->resolve(array('::1'));
     }
 
+    public function testConnectWillRejectWhenOnlyTcpConnectionRejectsAndCancelNextAttemptTimerImmediately()
+    {
+        $timer = $this->getMockBuilder('React\EventLoop\TimerInterface')->getMock();
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+        $loop->expects($this->once())->method('addTimer')->with(0.1, $this->anything())->willReturn($timer);
+        $loop->expects($this->once())->method('cancelTimer')->with($timer);
+
+        $deferred = new Deferred();
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+        $connector->expects($this->once())->method('connect')->with('tcp://[::1]:80?hostname=reactphp.org')->willReturn($deferred->promise());
+
+        $resolver = $this->getMockBuilder('React\Dns\Resolver\ResolverInterface')->getMock();
+        $resolver->expects($this->exactly(2))->method('resolveAll')->withConsecutive(
+            array('reactphp.org', Message::TYPE_AAAA),
+            array('reactphp.org', Message::TYPE_A)
+        )->willReturnOnConsecutiveCalls(
+            \React\Promise\resolve(array('::1')),
+            \React\Promise\reject(new \RuntimeException('ignored'))
+        );
+
+        $uri = 'tcp://reactphp.org:80';
+        $host = 'reactphp.org';
+        $parts = parse_url($uri);
+
+        $builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
+
+        $promise = $builder->connect();
+        $deferred->reject(new \RuntimeException('Connection refused'));
+
+        $exception = null;
+        $promise->then(null, function ($e) use (&$exception) {
+            $exception = $e;
+        });
+
+        $this->assertInstanceOf('RuntimeException', $exception);
+        $this->assertEquals('Connection to tcp://reactphp.org:80 failed: Connection refused', $exception->getMessage());
+    }
+
     public function testCancelConnectWillRejectPromiseAndCancelBothDnsLookups()
     {
         $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
@@ -193,7 +375,7 @@ public function testCancelConnectWillRejectPromiseAndCancelBothDnsLookups()
         $this->assertEquals('Connection to tcp://reactphp.org:80 cancelled during DNS lookup', $exception->getMessage());
     }
 
-    public function testCancelConnectWillRejectPromiseAndCancelPendingIpv6LookupAndCancelTimer()
+    public function testCancelConnectWillRejectPromiseAndCancelPendingIpv6LookupAndCancelDelayTimer()
     {
         $timer = $this->getMockBuilder('React\EventLoop\TimerInterface')->getMock();
         $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
@@ -230,10 +412,12 @@ public function testCancelConnectWillRejectPromiseAndCancelPendingIpv6LookupAndC
         $this->assertEquals('Connection to tcp://reactphp.org:80 cancelled during DNS lookup', $exception->getMessage());
     }
 
-    public function testCancelConnectWillRejectPromiseAndCancelPendingIpv6ConnectionAttemptAndPendingIpv4Lookup()
+    public function testCancelConnectWillRejectPromiseAndCancelPendingIpv6ConnectionAttemptAndPendingIpv4LookupAndCancelAttemptTimer()
     {
+        $timer = $this->getMockBuilder('React\EventLoop\TimerInterface')->getMock();
         $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
-        $loop->expects($this->never())->method('addTimer');
+        $loop->expects($this->once())->method('addTimer')->with(0.1, $this->anything())->willReturn($timer);
+        $loop->expects($this->once())->method('cancelTimer')->with($timer);
 
         $cancelled = 0;
         $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
diff --git a/tests/HappyEyeBallsConnectorTest.php b/tests/HappyEyeBallsConnectorTest.php
@@ -338,13 +338,13 @@ public function testRejectsWithTcpConnectorRejectionIfGivenIp()
 
     /**
      * @expectedException RuntimeException
-     * @expectedExceptionMessage All attempts to connect to "example.com" have failed
+     * @expectedExceptionMessage Connection to example.com:80 failed: Connection refused
      * @dataProvider provideIpvAddresses
      */
     public function testRejectsWithTcpConnectorRejectionAfterDnsIsResolved(array $ipv6, array $ipv4)
     {
         $that = $this;
-        $promise = Promise\reject(new \RuntimeException('Connection failed'));
+        $promise = Promise\reject(new \RuntimeException('Connection refused'));
         $this->resolver->expects($this->at(0))->method('resolveAll')->with($this->equalTo('example.com'), $this->anything())->willReturn(Promise\resolve($ipv6));
         $this->resolver->expects($this->at(1))->method('resolveAll')->with($this->equalTo('example.com'), $this->anything())->willReturn(Promise\resolve($ipv4));
         $this->tcp->expects($this->any())->method('connect')->with($this->stringContains(':80?hostname=example.com'))->willReturn($promise);
