-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Move asynchronous DNS implementation to use c-ares #427
Conversation
@Reflejo this is being tracked in #143 and @htuch from Google is already working on this so we've unfortunately got some duplication of effort. (I was not aware that you were working on this). Not sure of the best way of reconciling. I can make offline introductions if necessary. Also, as an aside, if all of your patches are working towards #128 can you let me know so that I can assign that issue to you. |
Yeah, I've put out a PR with my current changes at #428. I had a quick look and compared the two PRs. There are a few implementation differences. The PR I sent uses a single ares_channel for all requests, which might have some benefits for tail latency if we decided to reuse TCP connections one day. Also, the libevent integration doesn't really on ares_getsock() which has a 16 FD limits. I also added a minimal loopback test DNS server to validate the event behavior. |
@htuch yes I chose to do one channel per query to favor individual cancelations and simplicity. Give than there is one channel per query ares_getsock works just fine. I'm good with any of these approaches and I'll take a deeper look to yours later and maybe we can sync offline. |
Yep, let's chat Monday offline, I also was initially doing one per channel per query as mentioned in #143 (comment). |
e4db1ef
to
b15ca1a
Compare
The motivation for this change was to: - Move away from `getaddrinfo_a` which has a terrible API *and* uses POSIX signal handling. - `getaddrinfo_a` is a GNU extension and only available on Linux - c-ares support SRV
Automatic merge from submit-queue. [DO NOT MERGE] Auto PR to update dependencies of mixerclient This PR will be merged automatically once checks are successful. ```release-note none ```
) (#164) * Fix use-after-free error for remote .wasm file accesses. (envoyproxy#427) Signed-off-by: John Plevyak <jplevyak@gmail.com> * Add using declarations to work around upstream renaming. Signed-off-by: John Plevyak <jplevyak@gmail.com>
This is a first attempt to get feedback on the approach. I'm using libevent and the existing dispatching loop. The motivation for this change was:
getaddrinfo_a
which has a terrible API and uses POSIX signal handling.getaddrinfo_a
is a GNU extension and only available on Linux