Fix cancelling happy eyeballs when IPv6 resolution is pending #311
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: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