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.

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

Fix cancelling happy eyeballs when IPv6 resolution is pending #311

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

clue
Copy link
Member

@clue clue commented Dec 8, 2023

This changeset fixes a super nasty bug that would only show when cancelling a happy eyeballs connection attempt during the 50ms resolution delay when IPv4 resolution has already completed, but IPv6 resolution is still pending. In this case, it would (successfully) reject the promise and (successfully) cancel the IPv6 resolution but erroneously start new IPv4 connections that would occupy resources and keep the loop running until the server side decides to close these idle connections.

Interestingly, this is relatively easy to reproduce on dual-stack hosts by immediately cancelling a connection attempt to a host such as localhost that resolves to an IPv4 address instantly due to a /etc/hosts entry but requires a full DNS lookup for IPv6:

$connector = new React\Socket\Connector();
$connector->connect('localhost:6379')->cancel();

It looks like this problem was originally introduced with #258 in v1.7.0 quite some time ago. The updated test suite confirms we now successfully reject DNS resolution without starting any additional connection attempts in this case. The existing test suite confirms this does not affect any other cases, such as when DNS resolution is already completed.

I've stumbled upon this while updating the functional test suite of clue/reactphp-redis to use https://github.com/reactphp/async (as per clue/reactphp-redis#135), as the functional test suite happens to run the loop to test the cancellation behavior. Prior to applying these changes, the test suite would keep running for minutes.

Builds on top of #258, #225, #196 and potentially others

@clue clue added the bug label Dec 8, 2023
@clue clue added this to the v1.15.0 milestone Dec 8, 2023
Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

This is one hell of a bug, having a second look tomorrow to fully understand it. Nice work 👍 !

src/HappyEyeBallsConnectionBuilder.php Show resolved Hide resolved
src/HappyEyeBallsConnectionBuilder.php Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants