From 6757bc05cbb86f90f405145d535f11c49b8df119 Mon Sep 17 00:00:00 2001 From: Luan Devecchi Date: Tue, 10 Aug 2021 03:08:29 -0300 Subject: [PATCH 1/4] 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. --- src/cares_wrap.cc | 16 ++++- test/parallel/test-dns-channel-tries.js | 78 +++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-dns-channel-tries.js diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 9372aa6b7cc3bf..d5339f8efb9d01 100644 --- 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 new file mode 100644 index 00000000000000..43dd15d8b23ba5 --- /dev/null +++ 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(); + })); + })); +} From 32980dac49c7d10e4ad39487801dd60ca78675d2 Mon Sep 17 00:00:00 2001 From: Luan Devecchi Date: Tue, 10 Aug 2021 03:34:29 -0300 Subject: [PATCH 2/4] test: add missing description on `test-dns-channel-tries.js` --- test/parallel/test-dns-channel-tries.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/parallel/test-dns-channel-tries.js b/test/parallel/test-dns-channel-tries.js index 43dd15d8b23ba5..ab166d98b099ea 100644 --- a/test/parallel/test-dns-channel-tries.js +++ b/test/parallel/test-dns-channel-tries.js @@ -7,6 +7,9 @@ const dns = require('dns'); const TRIES = 1; const IMPRECISION_MS = 100; +// Tests for dns.Resolver option `tries`. +// This will roughly test if a single try fails after the set `timeout`. + for (const ctor of [dns.Resolver, dns.promises.Resolver]) { for (const tries of [null, true, false, '', '2']) { assert.throws(() => new ctor({ tries }), { From 495347302718593e1ad2815831e0ddb938088c94 Mon Sep 17 00:00:00 2001 From: Luan Devecchi Date: Tue, 10 Aug 2021 10:46:53 -0300 Subject: [PATCH 3/4] fixup! test allowed imprecision --- test/parallel/test-dns-channel-tries.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-dns-channel-tries.js b/test/parallel/test-dns-channel-tries.js index ab166d98b099ea..99888290da6e35 100644 --- a/test/parallel/test-dns-channel-tries.js +++ b/test/parallel/test-dns-channel-tries.js @@ -5,7 +5,7 @@ const dgram = require('dgram'); const dns = require('dns'); const TRIES = 1; -const IMPRECISION_MS = 100; +const IMPRECISION_MS = 500; // Tests for dns.Resolver option `tries`. // This will roughly test if a single try fails after the set `timeout`. @@ -36,17 +36,16 @@ for (let timeout of [-1, 1000, 1500, 50]) { const timeStart = performance.now(); resolver.resolve4('nodejs.org', common.mustCall((err) => { - const timeEnd = performance.now(); - 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 + // Adding 500ms 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}`); @@ -71,7 +70,7 @@ for (let timeout of [-1, 1000, 1500, 50]) { // c-ares default (-1): 5000ms if (timeout === -1) timeout = 5000; - // Adding 100 due to imprecisions + // Adding 500ms 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}`); From 191b5c66a1c0096e7aca8f70f620ad1bf2493425 Mon Sep 17 00:00:00 2001 From: Luan Devecchi Date: Fri, 13 Aug 2021 12:18:25 -0300 Subject: [PATCH 4/4] dns: adjust timer `repeat` --- src/cares_wrap.cc | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index d5339f8efb9d01..fedd79f21899cf 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -748,19 +748,7 @@ void ChannelWrap::StartTimer() { } int timeout = timeout_; if (timeout == 0) timeout = 1; - /* -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; - } + if (timeout < 0 || timeout >= 500) timeout = 500; uv_timer_start(timer_handle_, AresTimeout, 0, timeout); }