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

GetaddrinfoResolver succeeds futures on eventLoop #3042

Merged
merged 4 commits into from
Jan 8, 2025

Conversation

rnro
Copy link
Contributor

@rnro rnro commented Jan 8, 2025

Motivation:

testClientBindWorksOnSocketsBoundToEitherIPv4OrIPv6Only would fail sometimes leaking the IPv4 promise in GetaddrinfoResolver

HappyEyeballsConnector returns the connection when it resolves either IPv4 of IPv6. It uses the GetaddrinfoResolver which holds a promise for each of the IPv4 and IPv6 resolution; when one is completed the connection will be returned and it is possible to start tearing down the test and shutting down the event loop before the other is completed and we leak the promise.

Modifications:

Complete both futures on the event loop rather than the dispatch queue.

Result:

The futures are completed in the same event loop tick meaning that we cannot continue execution and leak one.

Motivation:

`testClientBindWorksOnSocketsBoundToEitherIPv4OrIPv6Only` would fail
sometimes leaking the IPv4 promise in `GetaddrinfoResolver`

`HappyEyeballsConnector` returns the connection when it resolves either IPv4 of IPv6.
It uses the `GetaddrinfoResolver` which holds a promise for each of the IPv4 and IPv6 resolution;
when one is completed it is possible to start tearing down the test and shutting down the event loop
before the other is completed and we leak the promise.

Modifications:

Complete both futures on the event loop rather than the dispatch queue.

Result:

The futures are completed in the same event loop tick meaning that we
cannot continue execution and leak one.
@rnro rnro added the 🔨 semver/patch No public API change. label Jan 8, 2025
Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Nice one!

Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

5.9 thresholds are off it seems.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Nice one.

@rnro rnro enabled auto-merge (squash) January 8, 2025 16:12
@rnro rnro requested a review from glbrntt January 8, 2025 16:12
@rnro rnro merged commit 27c839f into apple:main Jan 8, 2025
32 of 35 checks passed
@rnro rnro deleted the GetaddrinfoResolver_results_synchronized branch January 8, 2025 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants