dns: fix uv_timer_start usage

This commit fixes `uv_timer_start` usage on `ChannelWrap::StartTimer`, implements more precise timeout checking and missing tests for the `tries` option.
oluan · Aug 10, 2021 · Aug 10, 2021 · Aug 10, 2021 · Aug 13, 2021 · Aug 10, 2021
commit 6757bc05cbb86f90f405145d535f11c49b8df119
diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc
@@ -748,8 +748,20 @@ void ChannelWrap::StartTimer() {
   }
   int timeout = timeout_;
   if (timeout == 0) timeout = 1;
-  if (timeout < 0 || timeout > 1000) timeout = 1000;
-  uv_timer_start(timer_handle_, AresTimeout, timeout, timeout);
+  /* -1 implies the default c-ares value, that is 5000, thus checking each
+   * second */
+  if (timeout <= -1) timeout = 1000;
+  if (timeout >= 1000) {
+    int thousands = (timeout / 1000) * 1000;
+    /* Pure seconds should be checked every second (for example 1000, 2000,
+     * 5000) */
+    if (timeout - thousands == 0) timeout = 1000;
+    /* If it's not just seconds (for example 1200, 1050, 2500) check for an
+     * entire 100ms */
+    else
+      timeout = 100;
+  }
+  uv_timer_start(timer_handle_, AresTimeout, 0, timeout);
 }
 
 void ChannelWrap::CloseTimer() {

diff --git a/test/parallel/test-dns-channel-tries.js b/test/parallel/test-dns-channel-tries.js
@@ -0,0 +1,78 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const dgram = require('dgram');
+const dns = require('dns');
+
+const TRIES = 1;
+const IMPRECISION_MS = 100;
+
+for (const ctor of [dns.Resolver, dns.promises.Resolver]) {
+  for (const tries of [null, true, false, '', '2']) {
+    assert.throws(() => new ctor({ tries }), {
+      code: 'ERR_INVALID_ARG_TYPE',
+      name: 'TypeError',
+    });
+  }
+
+  for (const tries of [-2, 0, 4.2, 2 ** 31]) {
+    assert.throws(() => new ctor({ tries }), {
+      code: 'ERR_OUT_OF_RANGE',
+      name: 'RangeError',
+    });
+  }
+
+  for (const tries of [2, 1, 5]) new ctor({ tries });
+}
+
+for (let timeout of [-1, 1000, 1500, 50]) {
+  const server = dgram.createSocket('udp4');
+  server.bind(0, '127.0.0.1', common.mustCall(() => {
+    const resolver = new dns.Resolver({ tries: TRIES, timeout });
+    resolver.setServers([`127.0.0.1:${server.address().port}`]);
+
+    const timeStart = performance.now();
+    resolver.resolve4('nodejs.org', common.mustCall((err) => {
+      const timeEnd = performance.now();
+
+      assert.throws(() => { throw err; }, {
+        code: 'ETIMEOUT',
+        name: 'Error',
+      });
+
+      // c-ares default (-1): 5000ms
+      if (timeout === -1) timeout = 5000;
+
+      // Adding 100 due to imprecisions
+      const elapsedMs = (timeEnd - timeStart);
+      assert(elapsedMs <= timeout + IMPRECISION_MS, `The expected timeout did not occur. Expecting: ${timeout + IMPRECISION_MS} But got: ${elapsedMs}`);
+
+      server.close();
+    }));
+  }));
+}
+
+for (let timeout of [-1, 1000, 1500, 50]) {
+  const server = dgram.createSocket('udp4');
+  server.bind(0, '127.0.0.1', common.mustCall(() => {
+    const resolver = new dns.promises.Resolver({ tries: TRIES, timeout });
+    resolver.setServers([`127.0.0.1:${server.address().port}`]);
+    const timeStart = performance.now();
+    resolver.resolve4('nodejs.org').catch(common.mustCall((err) => {
+      assert.throws(() => { throw err; }, {
+        code: 'ETIMEOUT',
+        name: 'Error',
+      });
+      const timeEnd = performance.now();
+
+      // c-ares default (-1): 5000ms
+      if (timeout === -1) timeout = 5000;
+
+      // Adding 100 due to imprecisions
+      const elapsedMs = (timeEnd - timeStart);
+      assert(elapsedMs <= timeout + IMPRECISION_MS, `The expected timeout did not occur. Expecting: ${timeout + IMPRECISION_MS} But got: ${elapsedMs}`);
+
+      server.close();
+    }));
+  }));
+}