Avoid unneeded promise wrapping & avoid garbage references on legacy PHP

reactphp · Mar 7, 2020 · a2ee3c0 · a2ee3c0
1 parent cc4737d
commit a2ee3c0
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 70 deletions.
diff --git a/README.md b/README.md
@@ -927,6 +927,22 @@ also shares all of their features and implementation details.
 If you want to typehint in your higher-level protocol implementation, you SHOULD
 use the generic [`ConnectorInterface`](#connectorinterface) instead.
 
+As of `v1.4.0`, the `Connector` class defaults to using the
+[happy eyeballs algorithm](https://en.wikipedia.org/wiki/Happy_Eyeballs) to
+automatically connect over IPv4 or IPv6 when a hostname is given.
+This automatically attempts to connect using both IPv4 and IPv6 at the same time
+(preferring IPv6), thus avoiding the usual problems faced by users with imperfect
+IPv6 connections or setups.
+If you want to revert to the old behavior of only doing an IPv4 lookup and
+only attempt a single IPv4 connection, you can set up the `Connector` like this:
+
+```php
+$connector = new React\Socket\Connector($loop, array(
+    'happy_eyeballs' => false
+));
+```
+
+Similarly, you can also affect the deault DNS behavior as follows.
 The `Connector` class will try to detect your system DNS settings (and uses
 Google's public DNS server `8.8.8.8` as a fallback if unable to determine your
 system settings) to resolve all public hostnames into underlying IP addresses by
@@ -977,7 +993,7 @@ $connector->connect('localhost:80')->then(function (React\Socket\ConnectionInter
 ```
 
 By default, the `tcp://` and `tls://` URI schemes will use timeout value that
-repects your `default_socket_timeout` ini setting (which defaults to 60s).
+respects your `default_socket_timeout` ini setting (which defaults to 60s).
 If you want a custom timeout value, you can simply pass this like this:
 
 ```php
@@ -1094,17 +1110,6 @@ $connector->connect('google.com:80')->then(function (React\Socket\ConnectionInte
   Internally, the `tcp://` and `tls://` connectors will always be wrapped by
   `TimeoutConnector`, unless you disable timeouts like in the above example.
 
-> Internally the `HappyEyeBallsConnector` has replaced the `DnsConnector` as default 
-  resolving connector. It is still available as `Connector` has a new option, namely 
-  `happy_eyeballs`, to control which of the two will be used. By default it's `true` 
-  and will use `HappyEyeBallsConnector`, when set to `false` `DnsConnector` is used. 
-  We only recommend doing so when there are any backwards compatible issues on older 
-  systems only supporting IPv4. The `HappyEyeBallsConnector` implements most of 
-  RFC6555 and RFC8305 and will use concurrency to connect to the remote host by
-  attempting to connect over both IPv4 and IPv6 with a priority for IPv6 when 
-  available. Which ever connection attempt succeeds first will be used, the rest 
-  connection attempts will be canceled.
-
 ### Advanced client usage
 
 #### TcpConnector

diff --git a/src/Connector.php b/src/Connector.php
@@ -36,7 +36,7 @@ public function __construct(LoopInterface $loop, array $options = array())
 
             'dns' => true,
             'timeout' => true,
-            'happy_eyeballs' => \PHP_VERSION_ID < 70000 ? false : true,
+            'happy_eyeballs' => true,
         );
 
         if ($options['timeout'] === true) {

diff --git a/src/HappyEyeBallsConnectionBuilder.php b/src/HappyEyeBallsConnectionBuilder.php
@@ -190,71 +190,49 @@ public function check($resolve, $reject)
      */
     public function attemptConnection($ip)
     {
-        $promise = null;
-        $that = $this;
-
-        return new Promise\Promise(
-            function ($resolve, $reject) use (&$promise, $that, $ip) {
-                $uri = '';
+        $uri = '';
 
-                // prepend original scheme if known
-                if (isset($that->parts['scheme'])) {
-                    $uri .= $that->parts['scheme'] . '://';
-                }
+        // prepend original scheme if known
+        if (isset($this->parts['scheme'])) {
+            $uri .= $this->parts['scheme'] . '://';
+        }
 
-                if (\strpos($ip, ':') !== false) {
-                    // enclose IPv6 addresses in square brackets before appending port
-                    $uri .= '[' . $ip . ']';
-                } else {
-                    $uri .= $ip;
-                }
+        if (\strpos($ip, ':') !== false) {
+            // enclose IPv6 addresses in square brackets before appending port
+            $uri .= '[' . $ip . ']';
+        } else {
+            $uri .= $ip;
+        }
 
-                // append original port if known
-                if (isset($that->parts['port'])) {
-                    $uri .= ':' . $that->parts['port'];
-                }
+        // append original port if known
+        if (isset($this->parts['port'])) {
+            $uri .= ':' . $this->parts['port'];
+        }
 
-                // append orignal path if known
-                if (isset($that->parts['path'])) {
-                    $uri .= $that->parts['path'];
-                }
+        // append orignal path if known
+        if (isset($this->parts['path'])) {
+            $uri .= $this->parts['path'];
+        }
 
-                // append original query if known
-                if (isset($that->parts['query'])) {
-                    $uri .= '?' . $that->parts['query'];
-                }
+        // append original query if known
+        if (isset($this->parts['query'])) {
+            $uri .= '?' . $this->parts['query'];
+        }
 
-                // append original hostname as query if resolved via DNS and if
-                // destination URI does not contain "hostname" query param already
-                $args = array();
-                \parse_str(isset($that->parts['query']) ? $that->parts['query'] : '', $args);
-                if ($that->host !== $ip && !isset($args['hostname'])) {
-                    $uri .= (isset($that->parts['query']) ? '&' : '?') . 'hostname=' . \rawurlencode($that->host);
-                }
+        // append original hostname as query if resolved via DNS and if
+        // destination URI does not contain "hostname" query param already
+        $args = array();
+        \parse_str(isset($this->parts['query']) ? $this->parts['query'] : '', $args);
+        if ($this->host !== $ip && !isset($args['hostname'])) {
+            $uri .= (isset($this->parts['query']) ? '&' : '?') . 'hostname=' . \rawurlencode($this->host);
+        }
 
-                // append original fragment if known
-                if (isset($that->parts['fragment'])) {
-                    $uri .= '#' . $that->parts['fragment'];
-                }
+        // append original fragment if known
+        if (isset($this->parts['fragment'])) {
+            $uri .= '#' . $this->parts['fragment'];
+        }
 
-                $promise = $that->connector->connect($uri);
-                $promise->then($resolve, $reject);
-            },
-            function ($_, $reject) use (&$promise, $that) {
-                // cancellation should reject connection attempt
-                // (try to) cancel pending connection attempt
-                $reject(new \RuntimeException('Connection to ' . $that->uri . ' cancelled during connection attempt'));
-
-                if ($promise instanceof CancellablePromiseInterface) {
-                    // overwrite callback arguments for PHP7+ only, so they do not show
-                    // up in the Exception trace and do not cause a possible cyclic reference.
-                    $_ = $reject = null;
-
-                    $promise->cancel();
-                    $promise = null;
-                }
-            }
-        );
+        return $this->connector->connect($uri);
     }
 
     /**

diff --git a/tests/HappyEyeBallsConnectionBuilderTest.php b/tests/HappyEyeBallsConnectionBuilderTest.php
@@ -0,0 +1,47 @@
+<?php
+
+namespace React\Tests\Socket;
+
+use React\Promise\Promise;
+use React\Socket\HappyEyeBallsConnectionBuilder;
+
+class HappyEyeBallsConnectionBuilderTest extends TestCase
+{
+    public function testAttemptConnectionWillConnectViaConnectorToGivenIpWithPortAndHostnameFromUriParts()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
+        $connector->expects($this->once())->method('connect')->with('tcp://10.1.1.1:80?hostname=reactphp.org')->willReturn(new Promise(function () { }));
+
+        $resolver = $this->getMockBuilder('React\Dns\Resolver\ResolverInterface')->getMock();
+        $resolver->expects($this->never())->method('resolveAll');
+
+        $uri = 'tcp://reactphp.org:80';
+        $host = 'reactphp.org';
+        $parts = parse_url($uri);
+
+        $builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
+
+        $builder->attemptConnection('10.1.1.1');
+    }
+
+    public function testAttemptConnectionWillConnectViaConnectorToGivenIpv6WithAllUriParts()
+    {
+        $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 () { }));
+
+        $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);
+
+        $builder->attemptConnection('::1');
+    }
+}