Implement the Happy Eye Balls RFC's by WyriHaximus · Pull Request #196 · reactphp/socket

WyriHaximus · 2019-02-27T21:32:04Z

By using the happy eye balls algorythm as descripted in RFC6555 and
RFC8305 it will connect to the quickest responding server with a
preference for IPv6.

WyriHaximus · 2019-03-05T21:48:22Z

This PR is now ready for review. A follow up will be filed to replace the DnsConnector with the HappyEyeBallsConnector in the Connector an upcoming PR.

WyriHaximus · 2019-05-02T06:25:58Z

Ping @clue && @jsor did some major refactoring on this PR and it is now ready few review

clue

@WyriHaximus Thank you very much for working on this, this is one of the features that is very high on my personal wishlist! ❤️

However, I realize that the implementation is anything but trivial. I've added some remarks to some places that look odd to me, perhaps you can take a look?

That being said, once all implementation issues are resolved, I wonder what's the best path forward to ensure a safe rollout of this feature in the future? Perhaps it makes sense to first enable this without IPv6 support to see the initial implementation does not break any existing implementations? I can see some (legacy) use cases where it may be desirable to explicitly disable IPv6 because the downstream application might not be IPv6-ready? (think database fields storing an address in a fixed length field)

WyriHaximus · 2019-05-18T22:58:49Z

@WyriHaximus Thank you very much for working on this, this is one of the features that is very high on my personal wishlist!

🎉 !!!

However, I realize that the implementation is anything but trivial. I've added some remarks to some places that look odd to me, perhaps you can take a look?

Thanks for the review, I've addressed those points.

That being said, once all implementation issues are resolved, I wonder what's the best path forward to ensure a safe rollout of this feature in the future? Perhaps it makes sense to first enable this without IPv6 support to see the initial implementation does not break any existing implementations? I can see some (legacy) use cases where it may be desirable to explicitly disable IPv6 because the downstream application might not be IPv6-ready? (think database fields storing an address in a fixed length field)

Not entirely sure yet how to do that. What I want to do is write a follow up PR that initially will go full in on happy-eyeballs. And then we'll comes up during the discussion on that PR how we're shaping that. Because I'm pretty sure we can solve this with a flag on the Connector class 😄 .

clue

@WyriHaximus Thanks for the update! This PR/implementation is not exactly trivial, so I've added some remarks to see if we can avoid some of its complexity. Let me know what you think about it.

WyriHaximus · 2019-06-27T06:15:25Z

@WyriHaximus Thanks for the update! This PR/implementation is not exactly trivial, so I've added some remarks to see if we can avoid some of its complexity. Let me know what you think about it.

It isn't trivial at all. I'll be adding links to certain implementation details linking to the RFC('s) where applicable to understand the why for those things.

WyriHaximus · 2019-06-27T21:08:01Z

@clue oki updated to after your comments

clue

@WyriHaximus Thank you very much, the additional comments really help, I think we're slowly getting there! I've noticed this code currently has ~85% coverage and I've added some remarks to perhaps improve this.

Consider my structural concerns resolved, I'd love to get this in once we've added some additional tests, given the inherent complexity of this algorithm 👍

WyriHaximus · 2019-06-28T16:01:52Z

@clue working on the tests now, noticed the same thing this morning. 15% coverage to go 🎉

WyriHaximus · 2019-08-25T20:22:45Z

We should assert that a second connection will not be attempted while the first one is pending for less than 0.1 seconds.

✅

We should assert that this "next connection" timer is started.

✅

We should assert that a second connection attempt is started when the first attempt is still pending, but the timer fired.

✅

Likewise, add similar tests to check variations of IPv4/IPv6 attempts depending on DNS resolution etc.

❌ (working on it)

WyriHaximus · 2019-08-29T19:58:58Z

Likewise, add similar tests to check variations of IPv4/IPv6 attempts depending on DNS resolution etc.

✅

clue

@WyriHaximus Thanks for the update! All my local tests have succeeded with the updated version 👍

Unfortunately, the test suite now takes ~1.8 minutes(!) to run, whereas the old version took ~8 seconds. Perhaps rebase on #207 which should further improve this and then take a look at why the tests are so slow?

The test suite should probably use a mocked look (unless it's an integration test) and use $promise->then($this->expectCallableOneWith($expectedValue)) instead of running and awaiting the loop. In particular, all timers should be avoided unless absolutely required.

clue · 2019-09-20T12:16:18Z

+
+ if ($that->timer instanceof TimerInterface) {
+ return;
+ }


Can you take a look at this again? I can't reproduce this locally anymore, but I don't see any change here?

clue · 2019-09-20T12:17:15Z

+ $ipv4Deferred = new Promise\Deferred();
+ $deferred = new Promise\Deferred();
+
+ $timer = $that->loop->addTimer($that::RESOLVE_WAIT, function () use ($deferred, $ips) {


Promise cancellation doesn't seem to cancel this timer?

Addressed that

clue · 2019-09-20T12:19:31Z

+ 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()));
+ }
+ }


Not a big fan of having this logic in the test suite. It's my understanding this should be rather static and/or multiple independent tests. After all, where are the tests for this test suite? 👀 I'm okay with this for now.

Not a big fan of it either. But this lets us test different lenghth IPvX responses against several tests. This uncovered certain edge cases in the past and help me first the issue you commented about on line 60.

WyriHaximus · 2019-09-20T17:46:39Z

@clue decreased tests time, they're still slower then before but that's due to the timings involved.

clue · 2019-09-21T08:55:27Z

@WyriHaximus Can confirm the test time is now down to ~50s on my machine. There still seem to be plenty of timers, so I would still suggest mocking the loop and explicitly invoking the timer callback like this:

$loop = $this->createMock(LoopInterface::class);
$timer = null;
$loop->expects($this->once())->method('addTimer')->with(2.0, $this->callback(function ($cb) use (&$timer) {
$timer = $cb;
returntrue;
}));
// continue with test$this->assertNotNull($timer);
$timer();

WyriHaximus · 2019-09-22T19:17:43Z

@clue yeah this is going to be fun 🤐

WyriHaximus · 2019-09-22T20:39:43Z

@clue went for a slightly different approach that still does a full run with an actual event loop and timers, but I've speed it up instead by a factor of 10.

clue

@WyriHaximus Thanks for keeping up with this, I can confirm that test time changed to ~13s (~8s prior to this patch) 👍 Now let's get this feature shipped!

WyriHaximus · 2019-10-23T19:07:12Z

+It will then replace the hostname in the destination URI with this IP's and
+append a `hostname` query parameter and pass this updated URI to the underlying
+connector. 
+The Happy Eye Balls algorythm describes looking the IPv6 and IPv4 address for 


algorithm

WyriHaximus · 2019-10-23T19:07:20Z

+The `HappyEyeBallsConnector` class implements the
+[`ConnectorInterface`](#connectorinterface) and allows you to create plaintext
+TCP/IP connections to any hostname-port-combination. Internally it implements the 
+happy eyeballs algorythm from [`RFC6555`](https://tools.ietf.org/html/rfc6555) and 


algorithm

WyriHaximus · 2019-10-23T19:09:21Z

So @jsor spotted two typo's after approving, will fix those before merging 👍

By using the happy eye balls algorithm as described in RFC6555 and RFC8305 it will connect to the quickest responding server with a preference for IPv6.

WyriHaximus added the new feature label Feb 27, 2019

WyriHaximus added this to the v1.3.0 milestone Feb 27, 2019

WyriHaximus requested review from clue and jsor February 27, 2019 21:32

WyriHaximus force-pushed the happy-eye-balls branch 6 times, most recently from 4662be9 to ac92884Compare March 5, 2019 21:46

WyriHaximus marked this pull request as ready for review March 5, 2019 21:46

WyriHaximus force-pushed the happy-eye-balls branch 2 times, most recently from 89c9769 to e4676eaCompare March 6, 2019 16:49

WyriHaximus force-pushed the happy-eye-balls branch 2 times, most recently from 46a9866 to 25c1ecdCompare May 2, 2019 06:25

WyriHaximus force-pushed the happy-eye-balls branch from 25c1ecd to 6d7fc27Compare May 2, 2019 06:28

clue requested changes May 18, 2019
View reviewed changes

Comment threadsrc/HappyEyeBallsConnectionBuilder.php Outdated
Comment threadsrc/HappyEyeBallsConnectionBuilder.php Outdated

WyriHaximus force-pushed the happy-eye-balls branch 2 times, most recently from 3117a07 to 8fa1d3eCompare May 18, 2019 22:43

clue requested changes Jun 26, 2019
View reviewed changes

WyriHaximus force-pushed the happy-eye-balls branch 2 times, most recently from 1326b88 to 335cdcbCompare June 27, 2019 15:36

clue reviewed Jun 28, 2019
View reviewed changes

Comment threadsrc/HappyEyeBallsConnectionBuilder.php Outdated
Comment threadsrc/HappyEyeBallsConnectionBuilder.php
Comment threadsrc/HappyEyeBallsConnector.php Outdated

WyriHaximus force-pushed the happy-eye-balls branch from 335cdcb to 82ce847Compare July 6, 2019 13:15

WyriHaximus force-pushed the happy-eye-balls branch 4 times, most recently from c886e32 to d227b9cCompare August 25, 2019 19:43

WyriHaximus force-pushed the happy-eye-balls branch 4 times, most recently from dcc19c6 to 22d2f6bCompare August 29, 2019 19:37

clue requested changes Sep 20, 2019
View reviewed changes

WyriHaximus force-pushed the happy-eye-balls branch 3 times, most recently from e73fa33 to 888941fCompare September 20, 2019 15:46

WyriHaximus force-pushed the happy-eye-balls branch from 888941f to 29695e1Compare September 22, 2019 20:32

clue approved these changes Oct 23, 2019
View reviewed changes

jsor approved these changes Oct 23, 2019
View reviewed changes

WyriHaximus commented Oct 23, 2019
View reviewed changes

Implement the Happy Eye Balls RFC's
f2b5fc4
By using the happy eye balls algorithm as described in RFC6555 and RFC8305 it will connect to the quickest responding server with a preference for IPv6.

WyriHaximus force-pushed the happy-eye-balls branch from 29695e1 to f2b5fc4Compare October 23, 2019 21:09

WyriHaximus merged commit 529a5f8 into reactphp:masterOct 23, 2019

WyriHaximus mentioned this pull request Oct 28, 2019
Hookup happy eyeballs #216
Closed

clue mentioned this pull request Mar 8, 2020
[Client] DNS should have option to use IPv6 (AAAA) records #116
Closed

clue mentioned this pull request Dec 8, 2023
Fix cancelling happy eyeballs when IPv6 resolution is pending #311
Merged

Uh oh!

Uh oh!

Conversation

WyriHaximus commented Feb 27, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WyriHaximus commented Mar 5, 2019

Uh oh!

WyriHaximus commented May 2, 2019

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

WyriHaximus commented May 18, 2019

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WyriHaximus commented Jun 27, 2019

Uh oh!

WyriHaximus commented Jun 27, 2019

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WyriHaximus commented Jun 28, 2019

Uh oh!

WyriHaximus commented Aug 25, 2019

Uh oh!

WyriHaximus commented Aug 29, 2019

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

clueSep 20, 2019

Choose a reason for hiding this comment

Uh oh!

clueSep 20, 2019

Choose a reason for hiding this comment

Uh oh!

WyriHaximusSep 20, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clueSep 20, 2019

Choose a reason for hiding this comment

Uh oh!

WyriHaximusSep 20, 2019

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Sep 20, 2019

Uh oh!

clue commented Sep 21, 2019

Uh oh!

WyriHaximus commented Sep 22, 2019

Uh oh!

WyriHaximus commented Sep 22, 2019

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

WyriHaximusOct 23, 2019

Choose a reason for hiding this comment

Uh oh!

WyriHaximusOct 23, 2019

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Oct 23, 2019

Uh oh!

WyriHaximus commented Feb 27, 2019•
edited
Loading

WyriHaximusSep 20, 2019•
edited
Loading