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

Set AI_ADDRCONFIG when making getaddrinfo(3) calls for outgoing conns #7295

Merged
merged 1 commit into from Dec 7, 2023

Conversation

KJTsanaktsidis
Copy link
Contributor

When making an outgoing TCP or UDP connection, set AI_ADDRCONFIG in the hints we send to getaddrinfo(3) (if supported). This will prompt the resolver to NOT issue A or AAAA queries if the system does not actually have an IPv4 or IPv6 address (respectively).

This makes outgoing connections marginally more efficient on non-dual-stack systems, since we don't have to try connecting to an address which can't possibly work.

More importantly, however, this works around a race condition present in some older versions of glibc on aarch64 where it could accidently send the two outgoing DNS queries with the same DNS txnid, and get confused when receiving the responses. This manifests as outgoing connections sometimes taking 5 seconds (the DNS timeout before retry) to be made.

Fixes #19144
https://bugs.ruby-lang.org/issues/19144

@@ -140,4 +140,168 @@ def test_accept_multithread
server_threads.each(&:join)
end
end

def test_ai_addrconfig
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote this test, but it's so complicated and environment dependent that I wonder if it's worth having at all - maybe I should delete this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it is too complicated.
I talked about this test with naruse.
He said this test should be retained but should be always ommitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for merging this!

I see you committed the skip in abf192e. I agree this makes sense, even without the skip this test wouldn't actually do anything on CI anyway - unshare(2) won't work without root privileges and presumably we don't run the tests as root in Github Actions or rubyci.org. So better to just skip and be clear about it.

@KJTsanaktsidis
Copy link
Contributor Author

@akr hey, I was wondering if you could have a look at this? 🙏 thank you!

@mame mame requested a review from akr November 30, 2023 05:49
When making an outgoing TCP or UDP connection, set AI_ADDRCONFIG in the
hints we send to getaddrinfo(3) (if supported). This will prompt the
resolver to _NOT_ issue A or AAAA queries if the system does not
actually have an IPv4 or IPv6 address (respectively).

This makes outgoing connections marginally more efficient on
non-dual-stack systems, since we don't have to try connecting to an
address which can't possibly work.

More importantly, however, this works around a race condition present
in some older versions of glibc on aarch64 where it could accidently
send the two outgoing DNS queries with the same DNS txnid, and get
confused when receiving the responses. This manifests as outgoing
connections sometimes taking 5 seconds (the DNS timeout before retry) to
be made.

Fixes #19144
@mame mame force-pushed the ktsanaktsidis/ai_addrconfig branch from 4fec80c to 673ed41 Compare December 7, 2023 08:24
@@ -140,4 +140,168 @@ def test_accept_multithread
server_threads.each(&:join)
end
end

def test_ai_addrconfig
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it is too complicated.
I talked about this test with naruse.
He said this test should be retained but should be always ommitted.

@akr akr merged commit d2ba8ea into ruby:master Dec 7, 2023
98 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants