switch from c-ares to getdns #38184
switch from c-ares to getdns #38184
Conversation
nodejs-github-bot
added
build
devsnek
added
the
semver-major
|
Current status: working on polyfilling the current dns api/behavior as much as possible. although this is semver-major, its nice not to needlessly hurt people. |
devsnek
force-pushed
the
getdns-v2
branch
4 times, most recently
from
d4a63ad
to
2839734
Compare
| } | ||
|
|
||
| DNSWrap::~DNSWrap() { | ||
| getdns_context_destroy(context_); |
There was a problem hiding this comment.
adding a using GetDnsContextPointer = DeleteFnPtr<getdns_context, getdns_context_destroy>; would be nice.
|
|
||
| char* json = getdns_print_json_dict(response, 0); | ||
| Local<String> result = String::NewFromUtf8(isolate, json).ToLocalChecked(); | ||
| free(json); |
There was a problem hiding this comment.
Given that this is semver-major already, I'm curious if instead of doing the JSON conversion here... can we wrap the response in an object and just return that, allowing the user code to decide when to extract the details? Or, is that just too much additional complexity here?
src/node_dns.cc
Outdated
| Local<Value> DNSWrap::RegisterTransaction(getdns_transaction_t tid) { | ||
| Local<Promise::Resolver> p = | ||
| Promise::Resolver::New(env()->context()).ToLocalChecked(); | ||
| transactions_[tid].Reset(env()->isolate(), p); | ||
| return p->GetPromise(); | ||
| } |
There was a problem hiding this comment.
Would be nice to return MaybeLocal and avoid the ToLocalChecked()
| Local<Value> DNSWrap::RegisterTransaction(getdns_transaction_t tid) { | |
| Local<Promise::Resolver> p = | |
| Promise::Resolver::New(env()->context()).ToLocalChecked(); | |
| transactions_[tid].Reset(env()->isolate(), p); | |
| return p->GetPromise(); | |
| } | |
| MaybeLocal<Value> DNSWrap::RegisterTransaction(getdns_transaction_t tid) { | |
| Local<Promise::Resolver> p; | |
| if (!Promise::Resolve::New(env()->context()).ToLocal(&p)) | |
| return Nothing<Value>(); | |
| transactions_[tid].Reset(env()->isolate(), p); | |
| return p->GetPromise(); | |
| } |
| for (uint32_t i = 0; i < length; i += 1) { | ||
| getdns_transport_list_t transport = | ||
| static_cast<getdns_transport_list_t>(transports->Get(env->context(), i) | ||
| .ToLocalChecked() |
There was a problem hiding this comment.
Would be great to avoid the ToLocalChecked() in here.
| @@ -282,6 +282,28 @@ static void IsConstructor(const FunctionCallbackInfo<Value>& args) { | |||
| args.GetReturnValue().Set(args[0].As<v8::Function>()->IsConstructor()); | |||
| } | |||
|
|
|||
| static void CanonicalizeIP(const FunctionCallbackInfo<Value>& args) { | |||
There was a problem hiding this comment.
Might be good to add this in node_sockaddr instead.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Is there anyone who would be able to help me with the build file conversion? I'm running into some weird issues and I also lack knowledge about a lot of gyp stuff. |
|
|
|
Looks like linux is passed now. Not sure the other two platforms. root@0a77ea91a229 /o/d/node (getdns-v2)# ./out/Release/node -p process.versions
{
node: '16.0.0-pre',
v8: '9.0.257.13-node.9',
uv: '1.41.0',
zlib: '1.2.11',
brotli: '1.0.9',
modules: '93',
nghttp2: '1.42.0',
napi: '8',
llhttp: '2.1.3',
getdns: '1.6.0',
openssl: '1.1.1k+quic',
cldr: '38.1',
icu: '68.2',
tz: '2020d',
unicode: '13.0',
ngtcp2: '0.1.0-DEV',
nghttp3: '0.1.0-DEV'
} |
|
My local linux build was passing even without the node_metadata changes 😅, but it was failing on the ci from saying |
|
Still same issue: |
Could this be ccache issue ? cc @richardlau @rvagg I am on ubuntu 20.04. Clean build works for me now. |
I would say yes for sure. The trailing dot is bound to trip up someone. Have you benchmarked the new implementation compared to the old yet? The use of the microtaskqueue as opposed to SetImmediate in the old impl should have a noticeable impact but I'm a bit concerned about the JSON.parse and Array.prototype.map usage being a bottleneck and offsetting the gains. |
|
on build: The build works with Ninja on linux and macOS , but not traditional make. ar: /home/runner/work/node/node/out/Release/obj.target/getdns/deps/getdns/getdns/src/version.o: No such file or directory@nodejs/gyp Any clue ? |
|
I don't think make is the issue, I don't use ninja config locally. |
devsnek
force-pushed
the
getdns-v2
branch
2 times, most recently
from
688bc19
to
ccfbb27
Compare
|
@jasnell i did some light benchmarking and it seems like some things are 1-2% faster and some things are 1-2% slower. that being said, i see a lot of room for improvement here if we can make some upstream changes in getdns to improve how data is moved around. |
|
Hmm.. I would have hoped for a bit more of an improvement there. I really do suspect we could do better if we could avoid the JSON conversion and parse. It's good that it doesn't regress however. One other idea I was considering... Rather than dropping cares immediately, getdns can be introduced in parallel. Done right, it could come in as semver minor so that we can get it into 14 and 16 also with an option to choose between the impls. For 17+ cares would be dropped. This gives a bit more time to make further improvements. |
|
Patch fix windows @devsnek iff --git a/deps/getdns/configure_file.py b/deps/getdns/configure_file.py
index 7225a7195b..9391b53bb8 100644
--- a/deps/getdns/configure_file.py
+++ b/deps/getdns/configure_file.py
@@ -36,24 +36,24 @@ vars = {
'HAVE_STDLIB_H': True,
'HAVE_STRING_H': True,
'HAVE_TIME_H': True,
- 'HAVE_UNISTD_H': True,
- 'HAVE_FCNTL_H': True,
- 'HAVE_SIGNAL_H': True,
- 'HAVE_SYS_POLL_H': True,
- 'HAVE_POLL_H': True,
+ 'HAVE_UNISTD_H': not IS_WINDOWS,
+ 'HAVE_FCNTL_H': not IS_WINDOWS,
+ 'HAVE_SIGNAL_H': not IS_WINDOWS,
+ 'HAVE_SYS_POLL_H': not IS_WINDOWS,
+ 'HAVE_POLL_H': not IS_WINDOWS,
'HAVE_RESOURCE_H': False,
'HAVE_SYS_TYPES_H': True,
'HAVE_SYS_STAT_H': True,
'HAVE_ENDIAN_H': True,
- 'HAVE_NETDB_H': True,
- 'HAVE_ARPA_INET_H': True,
- 'HAVE_NETINET_IN_H': True,
- 'HAVE_NETINET_TCP_H': True,
- 'HAVE_SYS_SELECT_H': True,
- 'HAVE_SYS_SOCKET_H': True,
- 'HAVE_SYS_SYSCTL_H': True,
- 'HAVE_SYS_TIME_H': True,
- 'HAVE_SYS_WAIT_H': True,
+ 'HAVE_NETDB_H': not IS_WINDOWS,
+ 'HAVE_ARPA_INET_H': not IS_WINDOWS,
+ 'HAVE_NETINET_IN_H': not IS_WINDOWS,
+ 'HAVE_NETINET_TCP_H': not IS_WINDOWS,
+ 'HAVE_SYS_SELECT_H': not IS_WINDOWS,
+ 'HAVE_SYS_SOCKET_H': not IS_WINDOWS,
+ 'HAVE_SYS_SYSCTL_H': not IS_WINDOWS,
+ 'HAVE_SYS_TIME_H': not IS_WINDOWS,
+ 'HAVE_SYS_WAIT_H': not IS_WINDOWS,
'HAVE_WINDOWS_H': IS_WINDOWS,
'HAVE_WINSOCK_H': IS_WINDOWS,
'HAVE_WINSOCK2_H': IS_WINDOWS,
@@ -125,21 +125,21 @@ vars = {
'HAVE_DECL_INET_NTOP': True,
'HAVE_WIN_DECL_INET_PTON': False,
'HAVE_WIN_DECL_INET_NTOP': False,
- 'HAVE_DECL_MKSTEMP': True,
+ 'HAVE_DECL_MKSTEMP': not IS_WINDOWS,
'HAVE_DECL_SIGEMPTYSET': True,
'HAVE_DECL_SIGFILLSET': True,
'HAVE_DECL_SIGADDSET': True,
- 'HAVE_DECL_STRPTIME': True,
+ 'HAVE_DECL_STRPTIME': not IS_WINDOWS,
'HAVE_DECL_TCP_FASTOPEN': IS_LINUX,
'HAVE_DECL_TCP_FASTOPEN_CONNECT': IS_LINUX,
'HAVE_DECL_MSG_FASTOPEN': IS_LINUX,
- 'HAVE_FCNTL': True,
- 'HAVE_GETTIMEOFDAY': True,
+ 'HAVE_FCNTL': not IS_WINDOWS,
+ 'HAVE_GETTIMEOFDAY': not IS_WINDOWS,
'HAVE_IOCTLSOCKET': False,
'HAVE_SIGEMPTYSET': True,
'HAVE_SIGFILLSET': True,
'HAVE_SIGADDSET': True,
- 'HAVE_STRPTIME': True,
+ 'HAVE_STRPTIME': not IS_WINDOWS,
'HAVE_SIGSET_T': True,
'HAVE__SIGSET_T': False,
'HAVE_BSD_STDLIB_H': IS_BSD,
@@ -167,7 +167,7 @@ vars = {
'HAVE_EVENT_BASE_FREE': False,
'DEFAULT_EVENTLOOP': 'select_eventloop',
'USE_POLL_DEFAULT_EVENTLOOP': False,
- 'STRPTIME_WORKS': True,
+ 'STRPTIME_WORKS': not IS_WINDOWS,
'FD_SETSIZE': False,
'REQ_DEBUG': False,
'SCHED_DEBUG': False,
diff --git a/deps/getdns/getdns.gyp b/deps/getdns/getdns.gyp
index 6c64140c5c..b51172e682 100644
--- a/deps/getdns/getdns.gyp
+++ b/deps/getdns/getdns.gyp
@@ -40,6 +40,18 @@
'getdns/src/compat/strlcpy.c',
],
}],
+ [ 'OS=="win"', {
+ 'sources': [
+ 'getdns/src/compat/gettimeofday.c',
+ 'getdns/src/compat/arc4random.c',
+ 'getdns/src/compat/arc4_lock.c',
+ 'getdns/src/compat/arc4random_uniform.c',
+ 'getdns/src/compat/getentropy_win.c',
+ 'getdns/src/compat/strlcpy.c',
+ 'getdns/src/compat/mkstemp.c',
+ 'getdns/src/compat/strptime.c',
+ ],
+ }],
[ 'OS=="freebsd"', {
'defines': [
'_POSIX_C_SOURCE=200112L', |
Ninja works on clean build. Make not (need to build two times). I am thinking we dump the auto generate |
|
make works on a clean build for me |
|
@nodejs/gyp any ideas about these build failures? |
I believe this patch fix linux and macOS build. |
deps/getdns/getdns.gyp
Outdated
| 'getdns/src/getdns/getdns.h', | ||
| 'getdns/src/getdns/getdns_extra.h', | ||
| 'getdns/src/version.c', | ||
| 'getdns/getdns.pc', |
There was a problem hiding this comment.
Usually the outputs would go into <(INTERMEDIATE_DIR) or <(SHARED_INTERMEDIATE_DIR).
There was a problem hiding this comment.
this is mirroring the output of the cmake config. is it related to the cause of the ci failures?
There was a problem hiding this comment.
Yes.
diff --git a/deps/getdns/getdns.gyp b/deps/getdns/getdns.gyp
index f035598dd0..fb13a83bda 100644
--- a/deps/getdns/getdns.gyp
+++ b/deps/getdns/getdns.gyp
@@ -10,10 +10,12 @@
'getdns/src/openssl',
'getdns/src/tls',
'getdns/src/yxml',
+ '<(SHARED_INTERMEDIATE_DIR)/getdns/src',
],
'direct_dependent_settings': {
'include_dirs': [
'getdns/src',
+ '<(SHARED_INTERMEDIATE_DIR)/getdns/src',
],
},
'conditions': [
@@ -81,11 +83,11 @@
'getdns/getdns.pc.in',
],
'outputs': [
- 'getdns/src/config.h',
- 'getdns/src/getdns/getdns.h',
- 'getdns/src/getdns/getdns_extra.h',
- 'getdns/src/version.c',
- 'getdns/getdns.pc',
+ '<(SHARED_INTERMEDIATE_DIR)/getdns/src/config.h',
+ '<(SHARED_INTERMEDIATE_DIR)/getdns/src/getdns/getdns.h',
+ '<(SHARED_INTERMEDIATE_DIR)/getdns/src/getdns/getdns_extra.h',
+ '<(SHARED_INTERMEDIATE_DIR)/getdns/src/version.c',
+ '<(SHARED_INTERMEDIATE_DIR)/getdns/getdns.pc',
],
'action': [
'python', '<@(_inputs)', '--target', '<@(_outputs)',
@@ -139,6 +141,7 @@
'getdns/src/openssl/tls.c',
'getdns/src/openssl/pubkey-pinning-internal.c',
'getdns/src/openssl/keyraw-internal.c',
+ '<(SHARED_INTERMEDIATE_DIR)/getdns/src/version.c',
]
}
]allows me to build from a clean repository (git clean -fdX). I get 10 test failures but I assume those are not unexpected/being worked out.
There was a problem hiding this comment.
I haven't tested it but it's possible that the non-headers (i.e. getdns/src/version.c and getdns/getdns.pc) could go in <(INTERMEDIATE_DIR) as they probably aren't needed be available to the things linking to getdns (whereas the generated headers are and need to go into the shared area).
|
|
||
| class ReqWrap { | ||
| constructor(name) { | ||
| this[kResource] = new AsyncResource(name); |
There was a problem hiding this comment.
Could the ReqWrap itself just extend from AsyncResource?
There was a problem hiding this comment.
it could, though that would add the AsyncResource api to the req wrap, which is returned from all the various api calls to the user.
Co-Authored-By: Jiawen Geng <technicalcute@gmail.com> Co-Authored-By: Richard Lau <rlau@redhat.com>
|
@nodejs/crypto anyone know what's up with this test? |
|
The build config doesn't work with ninja: |
|
|
||
| '<(SHARED_INTERMEDIATE_DIR)/getdns/src/version.c', |
There was a problem hiding this comment.
| '<(SHARED_INTERMEDIATE_DIR)/getdns/src/version.c', |
I think process_outputs_as_sources makes this redundant. Removing the line fixes the ninja build.
|
On my mac, |
|
i thought this attempt really might work but getdns hasn't responded to my questions about issues i've found, it seems like the project might not be maintained enough for us to really use :( |
This will allow us to support lots of new things like dns over tls, dnssec, new dns question types, etc.