From d4442b15bfd833dd3efab22d2d02978afba1f793 Mon Sep 17 00:00:00 2001
From: Ben Noordhuis <info@bnoordhuis.nl>
Date: Tue, 19 May 2020 22:39:58 +0200
Subject: [PATCH] dns: make dns.Resolver timeout configurable

PR-URL: https://github.com/nodejs/node/pull/33472
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
---
 doc/api/dns.md                            | 16 +++++++
 lib/internal/dns/promises.js              |  6 ++-
 lib/internal/dns/utils.js                 | 13 +++++-
 src/cares_wrap.cc                         | 26 +++++++----
 test/parallel/test-dns-channel-timeout.js | 53 +++++++++++++++++++++++
 5 files changed, 101 insertions(+), 13 deletions(-)
 create mode 100644 test/parallel/test-dns-channel-timeout.js

diff --git a/doc/api/dns.md b/doc/api/dns.md
index ff318016d4becb..a5b3b82ea12b41 100644
--- a/doc/api/dns.md
+++ b/doc/api/dns.md
@@ -92,6 +92,22 @@ The following methods from the `dns` module are available:
 * [`resolver.reverse()`][`dns.reverse()`]
 * [`resolver.setServers()`][`dns.setServers()`]
 
+### `Resolver([options])`
+<!-- YAML
+added: v8.3.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/33472
+    description: The constructor now accepts an `options` object.
+                 The single supported option is `timeout`.
+-->
+
+Create a new resolver.
+
+* `options` {Object}
+  * `timeout` {integer} Query timeout in milliseconds, or `-1` to use the
+    default timeout.
+
 ### `resolver.cancel()`
 <!-- YAML
 added: v8.3.0
diff --git a/lib/internal/dns/promises.js b/lib/internal/dns/promises.js
index e5adc5b048515f..16cd97e06ac5ca 100644
--- a/lib/internal/dns/promises.js
+++ b/lib/internal/dns/promises.js
@@ -10,6 +10,7 @@ const {
   bindDefaultResolver,
   Resolver: CallbackResolver,
   validateHints,
+  validateTimeout,
   emitInvalidHostnameWarning,
 } = require('internal/dns/utils');
 const { codes, dnsException } = require('internal/errors');
@@ -208,8 +209,9 @@ const resolveMap = ObjectCreate(null);
 
 // Resolver instances correspond 1:1 to c-ares channels.
 class Resolver {
-  constructor() {
-    this._handle = new ChannelWrap();
+  constructor(options = undefined) {
+    const timeout = validateTimeout(options);
+    this._handle = new ChannelWrap(timeout);
   }
 }
 
diff --git a/lib/internal/dns/utils.js b/lib/internal/dns/utils.js
index 93aa529a733a89..6642a4f090bf67 100644
--- a/lib/internal/dns/utils.js
+++ b/lib/internal/dns/utils.js
@@ -6,6 +6,7 @@ const {
 
 const errors = require('internal/errors');
 const { isIP } = require('internal/net');
+const { validateInt32 } = require('internal/validators');
 const {
   ChannelWrap,
   strerror,
@@ -23,10 +24,17 @@ const {
   ERR_INVALID_OPT_VALUE
 } = errors.codes;
 
+function validateTimeout(options) {
+  const { timeout = -1 } = { ...options };
+  validateInt32(timeout, 'options.timeout', -1, 2 ** 31 - 1);
+  return timeout;
+}
+
 // Resolver instances correspond 1:1 to c-ares channels.
 class Resolver {
-  constructor() {
-    this._handle = new ChannelWrap();
+  constructor(options = undefined) {
+    const timeout = validateTimeout(options);
+    this._handle = new ChannelWrap(timeout);
   }
 
   cancel() {
@@ -162,6 +170,7 @@ module.exports = {
   getDefaultResolver,
   setDefaultResolver,
   validateHints,
+  validateTimeout,
   Resolver,
   emitInvalidHostnameWarning,
 };
diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc
index 8d1e3bc8794dfe..4a332db71279a4 100644
--- a/src/cares_wrap.cc
+++ b/src/cares_wrap.cc
@@ -152,7 +152,7 @@ using node_ares_task_list =
 
 class ChannelWrap : public AsyncWrap {
  public:
-  ChannelWrap(Environment* env, Local<Object> object);
+  ChannelWrap(Environment* env, Local<Object> object, int timeout);
   ~ChannelWrap() override;
 
   static void New(const FunctionCallbackInfo<Value>& args);
@@ -190,18 +190,21 @@ class ChannelWrap : public AsyncWrap {
   bool query_last_ok_;
   bool is_servers_default_;
   bool library_inited_;
+  int timeout_;
   int active_query_count_;
   node_ares_task_list task_list_;
 };
 
 ChannelWrap::ChannelWrap(Environment* env,
-                         Local<Object> object)
+                         Local<Object> object,
+                         int timeout)
   : AsyncWrap(env, object, PROVIDER_DNSCHANNEL),
     timer_handle_(nullptr),
     channel_(nullptr),
     query_last_ok_(true),
     is_servers_default_(true),
     library_inited_(false),
+    timeout_(timeout),
     active_query_count_(0) {
   MakeWeak();
 
@@ -210,10 +213,11 @@ ChannelWrap::ChannelWrap(Environment* env,
 
 void ChannelWrap::New(const FunctionCallbackInfo<Value>& args) {
   CHECK(args.IsConstructCall());
-  CHECK_EQ(args.Length(), 0);
-
+  CHECK_EQ(args.Length(), 1);
+  CHECK(args[0]->IsInt32());
+  const int timeout = args[0].As<Int32>()->Value();
   Environment* env = Environment::GetCurrent(args);
-  new ChannelWrap(env, args.This());
+  new ChannelWrap(env, args.This(), timeout);
 }
 
 class GetAddrInfoReqWrap : public ReqWrap<uv_getaddrinfo_t> {
@@ -462,6 +466,7 @@ void ChannelWrap::Setup() {
   options.flags = ARES_FLAG_NOCHECKRESP;
   options.sock_state_cb = ares_sockstate_cb;
   options.sock_state_cb_data = this;
+  options.timeout = timeout_;
 
   int r;
   if (!library_inited_) {
@@ -474,9 +479,9 @@ void ChannelWrap::Setup() {
   }
 
   /* We do the call to ares_init_option for caller. */
-  r = ares_init_options(&channel_,
-                        &options,
-                        ARES_OPT_FLAGS | ARES_OPT_SOCK_STATE_CB);
+  const int optmask =
+      ARES_OPT_FLAGS | ARES_OPT_TIMEOUTMS | ARES_OPT_SOCK_STATE_CB;
+  r = ares_init_options(&channel_, &options, optmask);
 
   if (r != ARES_SUCCESS) {
     Mutex::ScopedLock lock(ares_library_mutex);
@@ -495,7 +500,10 @@ void ChannelWrap::StartTimer() {
   } else if (uv_is_active(reinterpret_cast<uv_handle_t*>(timer_handle_))) {
     return;
   }
-  uv_timer_start(timer_handle_, AresTimeout, 1000, 1000);
+  int timeout = timeout_;
+  if (timeout == 0) timeout = 1;
+  if (timeout < 0 || timeout > 1000) timeout = 1000;
+  uv_timer_start(timer_handle_, AresTimeout, timeout, timeout);
 }
 
 void ChannelWrap::CloseTimer() {
diff --git a/test/parallel/test-dns-channel-timeout.js b/test/parallel/test-dns-channel-timeout.js
new file mode 100644
index 00000000000000..1e4dac5489730a
--- /dev/null
+++ b/test/parallel/test-dns-channel-timeout.js
@@ -0,0 +1,53 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const dgram = require('dgram');
+const dns = require('dns');
+
+for (const ctor of [dns.Resolver, dns.promises.Resolver]) {
+  for (const timeout of [null, true, false, '', '2']) {
+    assert.throws(() => new ctor({ timeout }), {
+      code: 'ERR_INVALID_ARG_TYPE',
+      name: 'TypeError',
+    });
+  }
+
+  for (const timeout of [-2, 4.2, 2 ** 31]) {
+    assert.throws(() => new ctor({ timeout }), {
+      code: 'ERR_OUT_OF_RANGE',
+      name: 'RangeError',
+    });
+  }
+
+  for (const timeout of [-1, 0, 1]) new ctor({ timeout });  // OK
+}
+
+for (const timeout of [0, 1, 2]) {
+  const server = dgram.createSocket('udp4');
+  server.bind(0, '127.0.0.1', common.mustCall(() => {
+    const resolver = new dns.Resolver({ timeout });
+    resolver.setServers([`127.0.0.1:${server.address().port}`]);
+    resolver.resolve4('nodejs.org', common.mustCall((err) => {
+      assert.throws(() => { throw err; }, {
+        code: 'ETIMEOUT',
+        name: 'Error',
+      });
+      server.close();
+    }));
+  }));
+}
+
+for (const timeout of [0, 1, 2]) {
+  const server = dgram.createSocket('udp4');
+  server.bind(0, '127.0.0.1', common.mustCall(() => {
+    const resolver = new dns.promises.Resolver({ timeout });
+    resolver.setServers([`127.0.0.1:${server.address().port}`]);
+    resolver.resolve4('nodejs.org').catch(common.mustCall((err) => {
+      assert.throws(() => { throw err; }, {
+        code: 'ETIMEOUT',
+        name: 'Error',
+      });
+      server.close();
+    }));
+  }));
+}