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.
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
localhostthat resolves to an IPv4 address instantly due to a/etc/hostsentry but requires a full DNS lookup for IPv6:It looks like this problem was originally introduced with #258 in
v1.7.0quite 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 https://github.com/clue/reactphp-redis/pull/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