Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Hookup happy eyeballs #216

Closed
wants to merge 2 commits into from
Closed

Hookup happy eyeballs #216

wants to merge 2 commits into from

Conversation

WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Oct 24, 2019 •
edited
Loading

Hooked up happy eyeballs from #196 and made it available in Connector with the happy_eyeballs flag. (Default set to false.)

@WyriHaximus WyriHaximus added this to the v1.4.0 milestone Oct 24, 2019
@WyriHaximus WyriHaximus force-pushed the hookup-happy-eyeballs branch 11 times, most recently from 770727f to e88598d Compare October 28, 2019 21:34
@WyriHaximus WyriHaximus changed the title [WIP] Hookup happy eyeballs Hookup happy eyeballs Oct 28, 2019
@WyriHaximus WyriHaximus requested review from clue and jsor October 28, 2019 21:35
clue
clue reviewed Nov 2, 2019
View reviewed changes
tests/FunctionalConnectorTest.php Outdated Show resolved Hide resolved
README.md Outdated
set to false by default to be backwards compatible with older systems only
supporting IPv4. By setting the `happy_eyeballs` option to `true` new connection
attempts are made over both IPv6 and IPv4 with the priority for IPv6. And return
the connection attempt that connects first.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last sentence seems to be cut off?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded that entire block

README.md Outdated
set to false by default to be backwards compatible with older systems only
supporting IPv4. By setting the `happy_eyeballs` option to `true` new connection
attempts are made over both IPv6 and IPv4 with the priority for IPv6. And return
the connection attempt that connects first.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this feature makes perfect sense and agree that having a feature toggle makes sense for avoid any problems with either edge cases we didn't cover or higher-level application logic that doesn't cope well with IPv6.

I don't consider this to be a BC break on our API side, so I think this should (eventually) be enabled by default. Having this disabled by default might make sense for a transition period. I personally don't mind either way, but would lean towards enabling this from the beginning to gather more feedback.

What do you think about this? Can we update the text to describe this (future) default behavior?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we initially talked about this you suggested disabling it by default where I wanted to enable by default IIRC. But I could be wrong and am glad to hear you also rather see it enabled by default. I'll change it as such 🎉 .

Also I want to do a follow up PR deprecating the DnsConnector and marking it for removal in 2.0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what was discussed before, but I think you're on the right track with your suggestion 👍

Deprecating DnsConnector sounds reasonable. Make sure said follow-up PR discusses what happens when happy_eyeballs will be turned off for whatever reasons and the application only wants to support IPv4.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what was discussed before, but I think you're on the right track with your suggestion

It's been a while :).

Deprecating DnsConnector sounds reasonable. Make sure said follow-up PR discusses what happens when happy_eyeballs will be turned off for whatever reasons and the application only wants to support IPv4.

Will do

@WyriHaximus WyriHaximus force-pushed the hookup-happy-eyeballs branch 5 times, most recently from 2dcb800 to b0e541f Compare November 8, 2019 19:50
clue
clue reviewed Nov 16, 2019
View reviewed changes
tests/FunctionalConnectorTest.php
* @group internet
*
* @expectedException \RuntimeException
* @expectedExceptionMessageRegExp /Connection to ipv6.tlund.se:80 failed/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test expects an exception and has assertions, can you check which is correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, resolved that 👍

tests/FunctionalConnectorTest.php
{
if ($this->ipv4() === true) {
$this->markTestSkipped('IPv4 supported on this system');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check makes sense, but I don't link how this marks the whole test result as yellow in a properly configured system (this would match most common systems). This might be preference, but how about using return with a comment instead? I think this is also in one of the other tests?

tests/FunctionalConnectorTest.php Show resolved Hide resolved
tests/IntegrationTest.php Outdated
@@ -322,6 +322,10 @@ function ($e) use (&$wait) {

public function testWaitingForSuccessfullyClosedConnectionShouldNotCreateAnyGarbageReferences()
{
if (PHP_VERSION_ID < 70000) {
$this->markTestSkipped('Not support on PHP 5.x, due to GC internals');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting one. This seems to suggest this changeset introduces a cyclic reference for legacy PHP?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, during tests about 560ish cycles (depending on some fiddling) are left for the GC to clean up on PHP 5.x. PHP 7.x handles this without any issues. When running gc_collect_cycles directly after HappyEyeBallsConnectionBuilder::connect resolves but before return the promise will result in 0 cycles collect by the test gc_collect_cycles. In the end this is because of the design of the HappyEyeBallsConnector to use a HappyEyeBallsConnectionBuilder per connection attempt due to the state that has be to tracked for each connection attempt.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I've disabled happy eyeballs by default on PHP 5.x because it could be to much of an impact and should be a conscious decision on legacy PHP. Since PHP 7 handles it just fine it's enabled by default there.

@WyriHaximus WyriHaximus force-pushed the hookup-happy-eyeballs branch 3 times, most recently from 2eaea98 to ab16549 Compare November 17, 2019 16:18
@clue clue mentioned this pull request Mar 7, 2020
Merged
@clue clue removed this from the v1.4.0 milestone Mar 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clue clue clue left review comments

@jsor jsor Awaiting requested review from jsor

Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@WyriHaximus @clue