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

Make Socket.getaddrinfo interruptible #2827

Open
wants to merge 1 commit into
base: master
from

Conversation

@kirs
Copy link

kirs commented Jan 10, 2020

This attempts to solve https://bugs.ruby-lang.org/issues/16476 and https://bugs.ruby-lang.org/issues/16381.

Before, Socket.getaddrinfo was using the blocking getaddrinfo(3) call.
That didn't allow to wrap it into Timeout.timeout or interrupt the thread.

Since we already have support for getaddrinfo_a(3) - the async version
of getaddrinfo(3), we should be able to make Socket.getaddrinfo leverage that
when async version is available in the system.

@kirs kirs force-pushed the kirs:socket-rsock_getaddrinfo_a branch 3 times, most recently from b0c00e7 to dbe3ac9 Jan 10, 2020
@methodmissing

This comment has been minimized.

Copy link
Contributor

methodmissing commented Jan 10, 2020

Changes LGTM, just minor code indentation divergences.

The async nature of the lookup seems to have issues with this test:

unknown object: "\x04\b[\vi*i\x02O\x01[\f[\bI\"\x0FTestSocket\x06:\x06EFI\"<test_getaddrinfo_raises_no_errors_on_port_argument_of_0\x06;\x00Fo:\x18MiniTest::Assertion\b:\tmesgI\"][ruby-core:29427].\nException raised:\n<#<SocketError: getaddrinfo_a: All requests done>>.\x06;\x00T:\abt[\x16I\"S/home/runner/work/ruby/ruby/src/test/socket/test_socket.rb:98:in `getaddrinfo'\x06;\x00FI\"\x01\x83/home/runner/work/ruby/ruby/src/test/socket/test_socket.rb:98:in `block in test_getaddrinfo_raises_no_errors_on_port_argument_of_0'\x06;\x00FI\"c/home/runner/work/ruby/ruby/src/tool/lib/test/unit/assertions.rb:79:in `assert_nothing_raised'\x06;\x00FI\"\x7F/home/runner/work/ruby/ruby/src/test/socket/test_socket.rb:98:in `test_getaddrinfo_raises_no_errors_on_port_argument_of_0'\x06;\x00FI\"L/home/runner/work/ruby/ruby/src/tool/lib/minitest/unit.rb:1301:in 

References

print "unknown object: #{$1.unpack("m")[0].dump}"

arg.timeout = timeout;
} else {
arg.timeout = NULL;
}

This comment has been minimized.

Copy link
@nobu

nobu Jan 10, 2020

Member

This change seems no meanings.

This comment has been minimized.

Copy link
@kirs

kirs Jan 11, 2020

Author

Thanks for pointing it out, you're right.

@kirs kirs force-pushed the kirs:socket-rsock_getaddrinfo_a branch 4 times, most recently from 1f8f245 to 1f21442 Jan 11, 2020
return ret;
}
if (ret && ret != EAI_ALLDONE) {
/* EAI_ALLDONE indicates that the request already completed and gai_suspend was redundant */

This comment has been minimized.

Copy link
@kirs

kirs Jan 11, 2020

Author

This was necessary to fix tests that showed a related bug that has already existed: when resolving localhost, gai_suspend immediatelly returns EAI_ALLDONE because resolving localhost is fast and by the time gai_suspend is called, the address is already resolved.

See an example in https://gist.github.com/kirs/f1472f6c553d2d628dd359c8dd2386b7

@kirs

This comment has been minimized.

Copy link
Author

kirs commented Jan 11, 2020

Thanks everyone for the review, the tests are now passing and this should be ready for another pass 👀

if (ret && ret != EAI_ALLDONE) {
/* EAI_ALLDONE indicates that the request already completed and gai_suspend was redundant */
/* on Ubuntu 18.04 (or other systems), gai_suspend(3) returns EAI_SYSTEM/ENOENT on timeout */
if (ret == EAI_SYSTEM && errno == ENOENT) {

This comment has been minimized.

Copy link
@lisovskyvlad

lisovskyvlad Jan 11, 2020

I believe, indentation among the file is 4 spaces, and here I see 1 tab instead.

Before, Socket.getaddrinfo was using the blocking getaddrinfo(3) call.
That didn't allow to wrap it into Timeout.timeout or interrupt the thread.

Since we already have support for getaddrinfo_a(3) - the async version
of getaddrinfo, we should be able to make Socket.getaddrinfo leverage that
when async version is available in the system.
@kirs kirs force-pushed the kirs:socket-rsock_getaddrinfo_a branch from 1f21442 to a11ec8d Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.