Skip to content

Async DNS resolution on Windows#14979

Merged
straight-shoota merged 4 commits intocrystal-lang:masterfrom
HertzDevil:feature/windows-async-dns
Sep 13, 2024
Merged

Async DNS resolution on Windows#14979
straight-shoota merged 4 commits intocrystal-lang:masterfrom
HertzDevil:feature/windows-async-dns

Conversation

@HertzDevil
Copy link
Contributor

Resolves part of #13619.

Here is a very brief benchmark:

require "socket"
require "benchmark"

DOMAINS = %w(
  google.com
  crystal-lang.org
  stackoverflow.com
  llvm.org
)

def dns(domains, n)
  ch = Channel(Array(Socket::Addrinfo)?).new
  n.times do
    spawn do
      domains.each do |domain|
        ch.send(Socket::Addrinfo.tcp(domain, 80))
      rescue
        ch.send(nil)
      end
    end
  end
  (n * domains.size).times { ch.receive }
end

Benchmark.ips(calculation: 60.seconds, warmup: 10.seconds) do |b|
  b.report("good") { dns(DOMAINS, 1000) }
  b.report("bad") { dns(%w(badhostname), 5) }
end
good  11.26  ( 88.83ms) (±11.89%)  2.34MB/op        fastest
 bad 370.10m (  2.70s ) (± 0.44%)  10.4kB/op  30.42× slower

Without this PR:

good   1.82  (550.44ms) (±18.24%)  2.46MB/op        fastest
 bad  74.04m ( 13.51s ) (± 0.15%)  10.6kB/op  24.54× slower

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

issue: GetAddrInfoExW only supports overlapped operations since Windows 8. Crystal technically supports down to Windows 7, although only with a flag (-Dwin7).
If we want to continue support for Windows 7, we would need to keep the getaddrinfo implementation or add a non-overlapped implementation for GetAddrInfoExW.

Comment on lines 66 to 76
if result == 0
return addrinfos
else
case error = WinError.new(result.to_u32!)
when .wsa_io_pending?
# used in `Crystal::IOCP::OverlappedOperation#try_cancel_getaddrinfo`
operation.handle = cancel_handle
else
raise ::Socket::Addrinfo::Error.from_os_error("GetAddrInfoExW", error, domain: domain, type: type, protocol: protocol, service: service)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be simplified:

Suggested change
if result == 0
return addrinfos
else
case error = WinError.new(result.to_u32!)
when .wsa_io_pending?
# used in `Crystal::IOCP::OverlappedOperation#try_cancel_getaddrinfo`
operation.handle = cancel_handle
else
raise ::Socket::Addrinfo::Error.from_os_error("GetAddrInfoExW", error, domain: domain, type: type, protocol: protocol, service: service)
end
end
return addrinfos if result.zero?
case error = WinError.new(result.to_u32!)
when .wsa_io_pending?
# used in `Crystal::IOCP::OverlappedOperation#try_cancel_getaddrinfo`
operation.handle = cancel_handle
else
raise ::Socket::Addrinfo::Error.from_os_error("GetAddrInfoExW", error, domain: domain, type: type, protocol: protocol, service: service)
end

straight-shoota pushed a commit that referenced this pull request Sep 9, 2024
This allows different overlapped operations to provide their own closure data, instead of putting everything in one big class, such as in #14979 (comment).
@straight-shoota straight-shoota modified the milestones: 1.13.3, 1.14.0 Sep 10, 2024
@straight-shoota straight-shoota changed the title Make DNS resolution asynchronous on Windows Async DNS resolution on Windows Sep 13, 2024
@straight-shoota straight-shoota merged commit dea39ad into crystal-lang:master Sep 13, 2024
@HertzDevil HertzDevil deleted the feature/windows-async-dns branch September 13, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants