Implement non-blocking async DNS resolver #2060
Conversation
Motivation: The GRPCHTTP2Core module needs a DNS resolver, as currently, IP addresses have to be passed in manually. Modifications: - Add a new type `SimpleAsyncDNSResolver` with a method `resolve(host:port:)` that calls the `getaddrinfo` `libc` function to resolve a hostname and port number to a list of IP addresses and port numbers. `resolve(host:port:)` is non-blocking and asynchronous. As calls to `getaddrinfo` are blocking, `resolve(host:port)` executes `getaddrinfo` in a `DispatchQueue` and uses a `CheckedContinuation` to interface with the execution. Result: The GRPCHTTP2Core module will have a DNS resolver.
There was a problem hiding this comment.
Good start Clinton. The overall shape is okay but we can simplify a few things and restructure things so that they're better structured and easier to maintain.
It also looks like you need to handle Linux better, the CI is failing because a bunch of constants can't be found etc.
| #elseif canImport(Musl) | ||
| import Musl | ||
| #endif | ||
| import CNIOLinux |
There was a problem hiding this comment.
We don't need CNIOLinux here
| #if canImport(Darwin) | ||
| import Darwin | ||
| #elseif os(Linux) || os(FreeBSD) || os(Android) | ||
| #if canImport(Glibc) | ||
| import Glibc | ||
| #elseif canImport(Musl) | ||
| import Musl | ||
| #endif | ||
| import CNIOLinux | ||
| #else | ||
| #error("The GRPCHTTP2Core module was unable to identify your C library.") | ||
| #endif |
There was a problem hiding this comment.
The simplest way to do these checks if to just use canImports:
#if canImport(Darwin)
import Darwin
#elseif canImport(Glibc)
import Glibc
#elseif canImport(Musl)
import Musl
#else
#error(...)
#endif| * limitations under the License. | ||
| */ | ||
|
|
||
| import Dispatch |
There was a problem hiding this comment.
Can you add appropriate access levels to the imports? Should be possible for both of these to be private
|
|
||
| /// An asynchronous non-blocking DNS resolver built on top of the libc `getaddrinfo` function. | ||
| @available(macOS 10.15, *) | ||
| package enum SimpleAsyncDNSResolver { |
There was a problem hiding this comment.
We don't need to include Simple or Async in the name, we can just call it DNSResolver
| @available(macOS 10.15, *) | ||
| package enum SimpleAsyncDNSResolver { | ||
| private static let dispatchQueue = DispatchQueue( | ||
| label: "io.grpc.SimpleAsyncDNSResolver.dispatchQueue" |
There was a problem hiding this comment.
No need to include dispatchQueue in the name: io.grpc.DNSResolver is fine
| family: Int32, | ||
| length: Int32 | ||
| ) -> String { | ||
| var resultingAddressBytes = [Int8](repeating: 0, count: Int(length)) |
There was a problem hiding this comment.
It'd be a bit clearer to call these presentationBytes (it's then more obvious what it is in relation to the function name)
There was a problem hiding this comment.
We're also passing this to a C function which is expected CChar (which is typealiased to Int8) – let's use CChar here because that's what the C function is expecting.
| (resultingAddressBytesPtr: inout UnsafeMutableBufferPointer<Int8>) -> String in | ||
|
|
||
| // Convert | ||
| inet_ntop(family, addressPtr, resultingAddressBytesPtr.baseAddress!, socklen_t(length)) |
There was a problem hiding this comment.
We shouldn't ignore the return value from inet_ntop
| return resultingAddressBytesPtr.baseAddress!.withMemoryRebound( | ||
| to: UInt8.self, | ||
| capacity: Int(length) | ||
| ) { resultingAddressBytesPtr -> String in | ||
| String(cString: resultingAddressBytesPtr) | ||
| } |
There was a problem hiding this comment.
We can do this without rebinding: String has an overload which accepts UnsafePointer<CChar>
| } | ||
|
|
||
| /// `Error` that may be thrown based on the error code returned by `getaddrinfo`. | ||
| package enum SimpleAsyncDNSResolverError: Error { |
There was a problem hiding this comment.
Can we embed this within the DNSResolver type?
| import XCTest | ||
|
|
||
| class SimpleAsyncDNSResolverTests: XCTestCase { |
Agree this would be a nice use case. Let's do this in a follow up though. |
| #endif | ||
| @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) |
There was a problem hiding this comment.
Use vertical whitespace to separate different things
| #endif | |
| @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) | |
| #endif | |
| @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) |
| if Task.isCancelled { | ||
| return [] | ||
| } |
There was a problem hiding this comment.
Given this function now throws we should also throw if the task is cancelled, you can replace this with: try Task.checkCancellation()
| var socketAddresses = [SocketAddress]() | ||
|
|
||
| while true { | ||
| let addressBytes: UnsafeRawPointer = .init(result.pointee.ai_addr) |
There was a problem hiding this comment.
prefer using the typename rather than .init
| let addressBytes: UnsafeRawPointer = .init(result.pointee.ai_addr) | |
| let addressBytes = UnsafeRawPointer(result.pointee.ai_addr) |
| fileprivate static func convertAddressFromNetworkToPresentationFormat<T>( | ||
| addressPtr: UnsafePointer<T>, | ||
| family: CInt, | ||
| length: CInt | ||
| ) throws -> String { |
There was a problem hiding this comment.
You don't need the generics here
| fileprivate static func convertAddressFromNetworkToPresentationFormat<T>( | |
| addressPtr: UnsafePointer<T>, | |
| family: CInt, | |
| length: CInt | |
| ) throws -> String { | |
| fileprivate static func convertAddressFromNetworkToPresentationFormat( | |
| addressPtr: UnsafeRawPointer, | |
| family: CInt, | |
| length: CInt | |
| ) throws -> String { |
As a caller you can still pass in the UnsafePointer<sockaddr_in> etc. though, it will be implicitly converted.
| var presentationAddress = "" | ||
|
|
||
| try withUnsafePointer(to: address.sin_addr) { addressPtr in | ||
| presentationAddress = try DNSResolver.convertAddressFromNetworkToPresentationFormat( |
There was a problem hiding this comment.
We can assigned directly back into the string:
let presentationAddress = try withUnsaferPointer(to: ...) { ... }
| .ipv4(host: "127.0.0.1", port: 80), | ||
| ] | ||
|
|
||
| let result = try await DNSResolver.resolve(host: "localhost", port: 80) |
There was a problem hiding this comment.
I don't know if we can rely on the ordering of the results. Probably worth resolve "::1" and "127.0.0.1" directly and checking we get back what we expect.
|
superseded by grpc/grpc-swift-nio-transport#4 |
Motivation:
The GRPCHTTP2Core module needs a DNS resolver, as currently, IP addresses have to be passed in manually.
Modifications:
DNSResolverwith a methodresolve(host:port:)that calls thegetaddrinfolibcfunction to resolve a hostname and port number to a list of socket addresses.resolve(host:port:)is non-blocking and asynchronous.Result:
The GRPCHTTP2Core module will have a DNS resolver.