From 2e150b4955a6c9ac12315b6904cc892d4aa54816 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20L=C3=BCck?= <christian@clue.engineering>
Date: Thu, 14 May 2020 16:59:07 +0200
Subject: [PATCH 1/4] Immediately try next connection when one attempt fails
 (happy eyeballs)

---
 src/HappyEyeBallsConnectionBuilder.php       | 12 +++-
 tests/HappyEyeBallsConnectionBuilderTest.php | 74 ++++++++++++++++++++
 2 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/src/HappyEyeBallsConnectionBuilder.php b/src/HappyEyeBallsConnectionBuilder.php
index 1d122363..70d8ee09 100644
--- a/src/HappyEyeBallsConnectionBuilder.php
+++ b/src/HappyEyeBallsConnectionBuilder.php
@@ -152,11 +152,21 @@ public function check($resolve, $reject)
             $that->cleanUp();
 
             $resolve($connection);
-        }, function (\Exception $e) use ($that, $ip, $reject) {
+        }, function (\Exception $e) use ($that, $ip, $resolve, $reject) {
             unset($that->connectionPromises[$ip]);
 
             $that->failureCount++;
 
+            // start next connection attempt immediately on error
+            if ($that->connectQueue) {
+                if ($that->nextAttemptTimer !== null) {
+                    $that->loop->cancelTimer($that->nextAttemptTimer);
+                    $that->nextAttemptTimer = null;
+                }
+
+                $that->check($resolve, $reject);
+            }
+
             if ($that->hasBeenResolved() === false) {
                 return;
             }
diff --git a/tests/HappyEyeBallsConnectionBuilderTest.php b/tests/HappyEyeBallsConnectionBuilderTest.php
index da521f44..09966816 100644
--- a/tests/HappyEyeBallsConnectionBuilderTest.php
+++ b/tests/HappyEyeBallsConnectionBuilderTest.php
@@ -217,6 +217,80 @@ public function testConnectWillStartConnectingWithAttemptTimerButWithoutResoluti
         $deferred->reject(new \RuntimeException());
     }
 
+    public function testConnectWillStartConnectingWithAttemptTimerWhenIpv6AndIpv4ResolvesAndWillStartNextConnectionAttemptWithoutAttemptTimerImmediatelyWhenFirstConnectionAttemptFails()
+    {
+        $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->exactly(2))->method('connect')->withConsecutive(
+            array('tcp://[::1]:80?hostname=reactphp.org'),
+            array('tcp://127.0.0.1:80?hostname=reactphp.org')
+        )->willReturnOnConsecutiveCalls(
+            $deferred->promise(),
+            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')),
+            \React\Promise\resolve(array('127.0.0.1'))
+        );
+
+        $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 testConnectWillStartConnectingWithAttemptTimerWhenOnlyIpv6ResolvesAndWillStartNextConnectionAttemptWithoutAttemptTimerImmediatelyWhenFirstConnectionAttemptFails()
+    {
+        $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->exactly(2))->method('connect')->withConsecutive(
+            array('tcp://[::1]:80?hostname=reactphp.org'),
+            array('tcp://[::2]:80?hostname=reactphp.org')
+        )->willReturnOnConsecutiveCalls(
+            $deferred->promise(),
+            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')),
+            \React\Promise\reject(new \RuntimeException())
+        );
+
+        $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;

From 128a9d1dab1f47c056827425ca3c0416de81f616 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20L=C3=BCck?= <christian@clue.engineering>
Date: Thu, 14 May 2020 17:18:46 +0200
Subject: [PATCH 2/4] Fix possible race condition when connector settles
 immediately

---
 src/HappyEyeBallsConnectionBuilder.php       |  5 +--
 tests/HappyEyeBallsConnectionBuilderTest.php | 34 +++++++++++++++++---
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/src/HappyEyeBallsConnectionBuilder.php b/src/HappyEyeBallsConnectionBuilder.php
index 70d8ee09..dc5f1437 100644
--- a/src/HappyEyeBallsConnectionBuilder.php
+++ b/src/HappyEyeBallsConnectionBuilder.php
@@ -146,7 +146,8 @@ public function check($resolve, $reject)
         $ip = \array_shift($this->connectQueue);
 
         $that = $this;
-        $that->connectionPromises[$ip] = $this->attemptConnection($ip)->then(function ($connection) use ($that, $ip, $resolve) {
+        $that->connectionPromises[$ip] = $this->attemptConnection($ip);
+        $that->connectionPromises[$ip]->then(function ($connection) use ($that, $ip, $resolve) {
             unset($that->connectionPromises[$ip]);
 
             $that->cleanUp();
@@ -180,7 +181,7 @@ public function 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) {
+        if ($this->nextAttemptTimer === null && (\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;
 
diff --git a/tests/HappyEyeBallsConnectionBuilderTest.php b/tests/HappyEyeBallsConnectionBuilderTest.php
index 09966816..1040b77c 100644
--- a/tests/HappyEyeBallsConnectionBuilderTest.php
+++ b/tests/HappyEyeBallsConnectionBuilderTest.php
@@ -261,13 +261,12 @@ public function testConnectWillStartConnectingWithAttemptTimerWhenOnlyIpv6Resolv
         $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->exactly(2))->method('connect')->withConsecutive(
             array('tcp://[::1]:80?hostname=reactphp.org'),
             array('tcp://[::2]:80?hostname=reactphp.org')
         )->willReturnOnConsecutiveCalls(
-            $deferred->promise(),
+            \React\Promise\reject(new \RuntimeException()),
             new Promise(function () { })
         );
 
@@ -287,8 +286,6 @@ public function testConnectWillStartConnectingWithAttemptTimerWhenOnlyIpv6Resolv
         $builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
 
         $builder->connect();
-
-        $deferred->reject(new \RuntimeException());
     }
 
     public function testConnectWillStartConnectingAndWillStartNextConnectionWithoutNewAttemptTimerWhenNextAttemptTimerFiresAfterIpv4Rejected()
@@ -566,4 +563,33 @@ public function testAttemptConnectionWillConnectViaConnectorToGivenIpv6WithAllUr
 
         $builder->attemptConnection('::1');
     }
+
+    public function testCheckCallsRejectFunctionImmediateWithoutLeavingDanglingPromiseWhenConnectorRejectsImmediately()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+        $connector->expects($this->once())->method('connect')->with('tcp://[::1]:80/path?test=yes&hostname=reactphp.org#start')->willReturn(\React\Promise\reject(new \RuntimeException()));
+
+        $resolver = $this->getMockBuilder('React\Dns\Resolver\ResolverInterface')->getMock();
+        $resolver->expects($this->never())->method('resolveAll');
+
+        $uri = 'tcp://reactphp.org:80/path?test=yes#start';
+        $host = 'reactphp.org';
+        $parts = parse_url($uri);
+
+        $builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
+
+        $ref = new \ReflectionProperty($builder, 'connectQueue');
+        $ref->setAccessible(true);
+        $ref->setValue($builder, array('::1'));
+
+        $builder->check($this->expectCallableNever(), function () { });
+
+        $ref = new \ReflectionProperty($builder, 'connectionPromises');
+        $ref->setAccessible(true);
+        $promises = $ref->getValue($builder);
+
+        $this->assertEquals(array(), $promises);
+    }
 }

From 0f2a4ec10159a3d59248f6852430984cfdb9e057 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20L=C3=BCck?= <christian@clue.engineering>
Date: Thu, 14 May 2020 17:30:49 +0200
Subject: [PATCH 3/4] Fix connection cleanup when connecting to same IP
 multiple times

---
 src/HappyEyeBallsConnectionBuilder.php       | 14 ++++++----
 tests/HappyEyeBallsConnectionBuilderTest.php | 29 ++++++++++++++++++++
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/src/HappyEyeBallsConnectionBuilder.php b/src/HappyEyeBallsConnectionBuilder.php
index dc5f1437..cb8a55ee 100644
--- a/src/HappyEyeBallsConnectionBuilder.php
+++ b/src/HappyEyeBallsConnectionBuilder.php
@@ -145,16 +145,20 @@ public function check($resolve, $reject)
     {
         $ip = \array_shift($this->connectQueue);
 
+        // start connection attempt and remember array position to later unset again
+        $this->connectionPromises[] = $this->attemptConnection($ip);
+        \end($this->connectionPromises);
+        $index = \key($this->connectionPromises);
+
         $that = $this;
-        $that->connectionPromises[$ip] = $this->attemptConnection($ip);
-        $that->connectionPromises[$ip]->then(function ($connection) use ($that, $ip, $resolve) {
-            unset($that->connectionPromises[$ip]);
+        $that->connectionPromises[$index]->then(function ($connection) use ($that, $index, $resolve) {
+            unset($that->connectionPromises[$index]);
 
             $that->cleanUp();
 
             $resolve($connection);
-        }, function (\Exception $e) use ($that, $ip, $resolve, $reject) {
-            unset($that->connectionPromises[$ip]);
+        }, function (\Exception $e) use ($that, $index, $resolve, $reject) {
+            unset($that->connectionPromises[$index]);
 
             $that->failureCount++;
 
diff --git a/tests/HappyEyeBallsConnectionBuilderTest.php b/tests/HappyEyeBallsConnectionBuilderTest.php
index 1040b77c..f63918b3 100644
--- a/tests/HappyEyeBallsConnectionBuilderTest.php
+++ b/tests/HappyEyeBallsConnectionBuilderTest.php
@@ -592,4 +592,33 @@ public function testCheckCallsRejectFunctionImmediateWithoutLeavingDanglingPromi
 
         $this->assertEquals(array(), $promises);
     }
+
+    public function testCleanUpCancelsAllPendingConnectionAttempts()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+        $connector->expects($this->exactly(2))->method('connect')->with('tcp://[::1]:80/path?test=yes&hostname=reactphp.org#start')->willReturnOnConsecutiveCalls(
+            new Promise(function () { }, $this->expectCallableOnce()),
+            new Promise(function () { }, $this->expectCallableOnce())
+        );
+
+        $resolver = $this->getMockBuilder('React\Dns\Resolver\ResolverInterface')->getMock();
+        $resolver->expects($this->never())->method('resolveAll');
+
+        $uri = 'tcp://reactphp.org:80/path?test=yes#start';
+        $host = 'reactphp.org';
+        $parts = parse_url($uri);
+
+        $builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
+
+        $ref = new \ReflectionProperty($builder, 'connectQueue');
+        $ref->setAccessible(true);
+        $ref->setValue($builder, array('::1', '::1'));
+
+        $builder->check($this->expectCallableNever(), function () { });
+        $builder->check($this->expectCallableNever(), function () { });
+
+        $builder->cleanUp();
+    }
 }

From 6b3aa7b2f4ee6456cc7c48508699a3d49fd57518 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20L=C3=BCck?= <christian@clue.engineering>
Date: Thu, 14 May 2020 21:07:14 +0200
Subject: [PATCH 4/4] Fix creating leftover connections when cancelling
 connection attempts

---
 src/HappyEyeBallsConnectionBuilder.php       |  3 ++
 tests/HappyEyeBallsConnectionBuilderTest.php | 27 +++++++++++
 tests/HappyEyeBallsConnectorTest.php         | 47 --------------------
 3 files changed, 30 insertions(+), 47 deletions(-)

diff --git a/src/HappyEyeBallsConnectionBuilder.php b/src/HappyEyeBallsConnectionBuilder.php
index cb8a55ee..0f610cd0 100644
--- a/src/HappyEyeBallsConnectionBuilder.php
+++ b/src/HappyEyeBallsConnectionBuilder.php
@@ -251,6 +251,9 @@ public function attemptConnection($ip)
      */
     public function cleanUp()
     {
+        // clear list of outstanding IPs to avoid creating new connections
+        $this->connectQueue = array();
+
         foreach ($this->connectionPromises as $connectionPromise) {
             if ($connectionPromise instanceof CancellablePromiseInterface) {
                 $connectionPromise->cancel();
diff --git a/tests/HappyEyeBallsConnectionBuilderTest.php b/tests/HappyEyeBallsConnectionBuilderTest.php
index f63918b3..4521ab09 100644
--- a/tests/HappyEyeBallsConnectionBuilderTest.php
+++ b/tests/HappyEyeBallsConnectionBuilderTest.php
@@ -621,4 +621,31 @@ public function testCleanUpCancelsAllPendingConnectionAttempts()
 
         $builder->cleanUp();
     }
+
+    public function testCleanUpCancelsAllPendingConnectionAttemptsWithoutStartingNewAttemptsDueToCancellationRejection()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+        $connector->expects($this->once())->method('connect')->with('tcp://[::1]:80/path?test=yes&hostname=reactphp.org#start')->willReturn(new Promise(function () { }, function () {
+            throw new \RuntimeException();
+        }));
+
+        $resolver = $this->getMockBuilder('React\Dns\Resolver\ResolverInterface')->getMock();
+        $resolver->expects($this->never())->method('resolveAll');
+
+        $uri = 'tcp://reactphp.org:80/path?test=yes#start';
+        $host = 'reactphp.org';
+        $parts = parse_url($uri);
+
+        $builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
+
+        $ref = new \ReflectionProperty($builder, 'connectQueue');
+        $ref->setAccessible(true);
+        $ref->setValue($builder, array('::1', '::1'));
+
+        $builder->check($this->expectCallableNever(), function () { });
+
+        $builder->cleanUp();
+    }
 }
diff --git a/tests/HappyEyeBallsConnectorTest.php b/tests/HappyEyeBallsConnectorTest.php
index eb8f468c..6c1c24a6 100644
--- a/tests/HappyEyeBallsConnectorTest.php
+++ b/tests/HappyEyeBallsConnectorTest.php
@@ -138,30 +138,6 @@ public function testPassThroughResolverIfGivenExplicitHost()
         $this->loop->run();
     }
 
-    /**
-     * @dataProvider provideIpvAddresses
-     */
-    public function testIpv4ResolvesFirstSoButIPv6IsTheFirstToConnect(array $ipv6, array $ipv4)
-    {
-        $this->resolver->expects($this->at(0))->method('resolveAll')->with('google.com', Message::TYPE_AAAA)->will($this->returnValue(Promise\Timer\resolve(0.001, $this->loop)->then(function () use ($ipv6) {
-            return Promise\resolve($ipv6);
-        })));
-        $this->resolver->expects($this->at(1))->method('resolveAll')->with('google.com', Message::TYPE_A)->will($this->returnValue(Promise\resolve($ipv4)));
-        $i = 0;
-        while (count($ipv6) > 0 || count($ipv4) > 0) {
-            if (count($ipv6) > 0) {
-                $this->tcp->expects($this->at($i++))->method('connect')->with($this->equalTo('scheme://[' . array_shift($ipv6) . ']:80/?hostname=google.com'))->will($this->returnValue(Promise\resolve()));
-            }
-            if (count($ipv4) > 0) {
-                $this->tcp->expects($this->at($i++))->method('connect')->with($this->equalTo('scheme://' . array_shift($ipv4) . ':80/?hostname=google.com'))->will($this->returnValue(Promise\resolve()));
-            }
-        }
-
-        $this->connector->connect('scheme://google.com:80/?hostname=google.com');
-
-        $this->loop->run();
-    }
-
     /**
      * @dataProvider provideIpvAddresses
      */
@@ -202,29 +178,6 @@ public function testIpv6DoesntResolvesWhileIpv4DoesFirstSoIpv4Connects(array $ip
         $this->loop->run();
     }
 
-    /**
-     * @dataProvider provideIpvAddresses
-     */
-    public function testAttemptsToConnectBothIpv6AndIpv4Addresses(array $ipv6, array $ipv4)
-    {
-        $this->resolver->expects($this->at(0))->method('resolveAll')->with('google.com', Message::TYPE_AAAA)->will($this->returnValue(Promise\resolve($ipv6)));
-        $this->resolver->expects($this->at(1))->method('resolveAll')->with('google.com', Message::TYPE_A)->will($this->returnValue(Promise\resolve($ipv4)));
-
-        $i = 0;
-        while (count($ipv6) > 0 || count($ipv4) > 0) {
-            if (count($ipv6) > 0) {
-                $this->tcp->expects($this->at($i++))->method('connect')->with($this->equalTo('scheme://[' . array_shift($ipv6) . ']:80/?hostname=google.com'))->will($this->returnValue(Promise\resolve()));
-            }
-            if (count($ipv4) > 0) {
-                $this->tcp->expects($this->at($i++))->method('connect')->with($this->equalTo('scheme://' . array_shift($ipv4) . ':80/?hostname=google.com'))->will($this->returnValue(Promise\resolve()));
-            }
-        }
-
-        $this->connector->connect('scheme://google.com:80/?hostname=google.com');
-
-        $this->loop->run();
-    }
-
     public function testThatTheIpv4ConnectionWillWait100MilisecondsWhenIpv6AndIpv4ResolveSimultaniously()
     {
         $timings = array();