Add a DNS name resolver #1826
Add a DNS name resolver #1826
Conversation
Motivation: Many users will rely on DNS to resolve the IP addresses of servers to connect to, we should therefore provide a DNS name resolver. Modifications: - Add a DNS name resolver factory capable of resolving IP addresses and service config via TXT records. - Add the resolve to the registry defaults Result: Can resolve DNS targets
|
Note: we can't merge this until we have a 1.0.0 for swift-async-dns-resolver, however, I'd like to get a review on this in the meantime to make sure we're happy with the shape of this for now. |
| /// The hostname of the system the resolver is running on. May be used when a service config | ||
| /// from a list of config choices. |
There was a problem hiding this comment.
Nit: the second sentence sounds wrong, not sure if you meant s/when/with or something else.
| func next() async throws -> NameResolutionResult? { | ||
| return try await self.resolve() | ||
| } |
There was a problem hiding this comment.
I've got some concerns about this conformance to AsyncIteratorProtocol. First of all, this sequence never ends (unless it throws) which I guess is okay. However, the docs say:
After returning nil (or throwing an error) from next(), the iterator enters a terminal state, and all future calls to next() must return nil.
And we're not respecting this, as nothing stops the caller from calling next() again after throwing, and getting a value back.
Secondly, I'm not sure we're dealing with cancellation appropriately. We're not checking whether the task is cancelled nor using a cancellation handler, and we're not returning nil if the task does get cancelled.
There was a problem hiding this comment.
I've got some concerns about this conformance to
AsyncIteratorProtocol. First of all, this sequence never ends (unless it throws) which I guess is okay.
Yeah, there's no requirement for it to end so this part is fine.
However, the docs say:
After returning nil (or throwing an error) from next(), the iterator enters a terminal state, and all future calls to next() must return nil.
And we're not respecting this, as nothing stops the caller from calling
next()again after throwing, and getting a value back.
Interesting, I'm not sure how widely we respect this. I'll fix this up though.
Secondly, I'm not sure we're dealing with cancellation appropriately. We're not checking whether the task is cancelled nor using a cancellation handler, and we're not returning nil if the task does get cancelled.
I don't think we need to check explicitly: at the moment we just let calls to the underlying DNS resolver fail with a a cancellation error and have that bubble up.
There was a problem hiding this comment.
IIUC the resolver's resolve() method only throws RPCError when calling get() on the Result. We're using a non-throwing TaskGroup in resolveEndpoints() so cancellation can't bubble up via CancellationError.
Motivation:
Many users will rely on DNS to resolve the IP addresses of servers to connect to, we should therefore provide a DNS name resolver.
Modifications:
Result:
Can resolve DNS targets