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

Call getaddrinfo from Ruby #139

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

casperisfine
Copy link
Contributor

Ref: https://bugs.ruby-lang.org/issues/19965

getaddrinfo is notorious for not having a timeout, which can make it hang for a very long time. Since Trilogy releases the GVL before calling it, that can cause the whole VM to be unresponsive and no longer respond to ctrl+c etc.

In 3.3.0, Ruby is now doing name resolution from a background thread, making Socket.getaddrinfo interruptible. So by doing the name resolution on the Ruby side, we should avoid such scenarios.

@jhawthorn @matthewd @composerinteralia @adrianna-chang-shopify what do you think?

Copy link
Collaborator

@adrianna-chang-shopify adrianna-chang-shopify left a comment

Choose a reason for hiding this comment

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

This makes sense to me -- a couple comments / questions, but I'll let other folks weigh in.

assert_equal expected_connection_options, client.connection_options
actual_options = client.connection_options.dup
actual_options.delete(:ip_address)
assert_equal expected_connection_options, actual_options
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we assert ip_address on the expected connection options instead?

contrib/ruby/ext/trilogy-ruby/cext.c Show resolved Hide resolved
raise Trilogy::BaseConnectionError, "Couldn't resolve host: #{options[:host].inspect}"
end

addrs.sort_by!(&:first) # Priority to IPv4
Copy link
Collaborator

Choose a reason for hiding this comment

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

For my own understanding: does prioritizing IPv4 here match what happens when we were calling getaddrinfo directly in trilogy_sock_resolve? Were we resolving to a single address previously, or potentially to multiple? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a good question to which I don't have a proper answer. This PR is more proof of concept territory because I'm not very good at networking, so there might be a much simpler / better way to do this.

The crux is really that I see processes stuck connecting to MySQL, and I highly suspect this is the cause.

@jhawthorn
Copy link
Member

I think it's a good idea to use Ruby as much as we can, but I'm also a bit unsure about the IP sorting

@casperisfine
Copy link
Contributor Author

I'm also a bit unsure about the IP sorting

Yeah me too, I think I need to dig a bit more on how this works

`getaddrinfo` is notorious for not having a timeout, which can make
it hang for a very long time. Since Trilogy releases the GVL before
calling it, that can cause the whole VM to be unresponsive and no
longer respond to ctrl+c etc.

In 3.3.0, Ruby is now doing name resolution from a background thread,
making `Socket.getaddrinfo` interruptible. So by doing the name resolution
on the Ruby side, we should avoid such scenarios.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants