From 0405f1491d31d8e385628904752e046da5f4db69 Mon Sep 17 00:00:00 2001 From: Nicolas Herment Date: Mon, 1 Mar 2021 16:12:19 +0100 Subject: [PATCH 1/4] Remove unnecessary timeout check in httpClient delay In addition it's probably better to use a t.setTimeout(..) than assertion for a timeout because it will come with a clear and informative error message. --- test/httpClient.test.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/httpClient.test.js b/test/httpClient.test.js index 7deb8de3..8fb25128 100644 --- a/test/httpClient.test.js +++ b/test/httpClient.test.js @@ -66,7 +66,8 @@ test('client calls a https server twice', (t) => { test('client calculates correct duration when using pipelining', (t) => { t.plan(4) - const lazyServer = helper.startServer({ delayResponse: 500 }) + const delayResponse = 500 + const lazyServer = helper.startServer({ delayResponse }) const opts = lazyServer.address() opts.pipelining = 2 const client = new Client(opts) @@ -74,7 +75,7 @@ test('client calculates correct duration when using pipelining', (t) => { client.on('response', (statusCode, length, duration) => { t.equal(statusCode, 200, 'status code matches') - t.ok(duration > 500 && duration < 800) + t.ok(duration > delayResponse, `Expected response delay > ${delayResponse}ms but got ${duration}ms`) if (++count === 2) { client.destroy() From 0bab9ed83cdbd86901b8fa61afb945e7120782cf Mon Sep 17 00:00:00 2001 From: Nicolas Herment Date: Tue, 2 Mar 2021 08:44:28 +0100 Subject: [PATCH 2/4] Update a couple of tests that don't do well in low CPU env. Avoid relying on node being able to execute its setTimeouts within a reasonable time to account for the low performance VMs of the tests environments. --- test/httpClient.test.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/test/httpClient.test.js b/test/httpClient.test.js index 8fb25128..62553769 100644 --- a/test/httpClient.test.js +++ b/test/httpClient.test.js @@ -581,9 +581,10 @@ test('client should emit a timeout when no response is received', (t) => { client.on('timeout', () => { t.ok(1, 'timeout should have happened') + + // client.destroy must be done async to ensure the correct internal timer is destroyed instead of the one that triggered this timeout + setTimeout(() => client.destroy()) }) - - setTimeout(() => client.destroy(), 1800) }) test('client should emit 2 timeouts when no responses are received', (t) => { @@ -592,12 +593,14 @@ test('client should emit 2 timeouts when no responses are received', (t) => { const opts = timeoutServer.address() opts.timeout = 1 const client = new Client(opts) - + let count = 0 client.on('timeout', () => { t.ok(1, 'timeout should have happened') + if (count++ > 0) { + // client.destroy must be done async to ensure the correct internal timer is destroyed instead of the one that triggered this timeout + setTimeout(() => client.destroy()) + } }) - - setTimeout(() => client.destroy(), 2800) }) test('client should have 2 different requests it iterates over', (t) => { From 7fdffa90e7339254e956951688baf74339f36229 Mon Sep 17 00:00:00 2001 From: Nicolas Herment Date: Tue, 2 Mar 2021 08:49:46 +0100 Subject: [PATCH 3/4] add empty line for readability in a test --- test/httpClient.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/httpClient.test.js b/test/httpClient.test.js index 62553769..c60c8d86 100644 --- a/test/httpClient.test.js +++ b/test/httpClient.test.js @@ -581,7 +581,7 @@ test('client should emit a timeout when no response is received', (t) => { client.on('timeout', () => { t.ok(1, 'timeout should have happened') - + // client.destroy must be done async to ensure the correct internal timer is destroyed instead of the one that triggered this timeout setTimeout(() => client.destroy()) }) From 782c9a7becf6045316cd5842cf3974f169f8373e Mon Sep 17 00:00:00 2001 From: Nicolas Herment Date: Tue, 2 Mar 2021 13:06:43 +0100 Subject: [PATCH 4/4] Allow sync call of httpClient.destroy() in a timeout event --- lib/httpClient.js | 8 ++++---- package.json | 2 +- test/httpClient.test.js | 7 ++----- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/lib/httpClient.js b/lib/httpClient.js index 2f6e04e0..43d950e8 100644 --- a/lib/httpClient.js +++ b/lib/httpClient.js @@ -64,15 +64,15 @@ function Client (opts) { this.opts.setupClient(this) const handleTimeout = () => { - // all pipelined requests have timed out here - this.resData.forEach(() => this.emit('timeout')) this.cer = 0 this._destroyConnection() - // timeout has already occured, need to set a new timeoutTicker - this.timeoutTicker = retimer(handleTimeout, this.timeout) + this.timeoutTicker.reschedule(this.timeout) this._connect() + + // all pipelined requests have timed out here + this.resData.forEach(() => this.emit('timeout')) } if (this.rate) { diff --git a/package.json b/package.json index 15718d87..e138c77a 100644 --- a/package.json +++ b/package.json @@ -65,7 +65,7 @@ "pretty-bytes": "^5.4.1", "progress": "^2.0.3", "reinterval": "^1.1.0", - "retimer": "^2.0.0", + "retimer": "^3.0.0", "semver": "^7.3.2", "timestring": "^6.0.0" } diff --git a/test/httpClient.test.js b/test/httpClient.test.js index c60c8d86..3315512c 100644 --- a/test/httpClient.test.js +++ b/test/httpClient.test.js @@ -581,9 +581,7 @@ test('client should emit a timeout when no response is received', (t) => { client.on('timeout', () => { t.ok(1, 'timeout should have happened') - - // client.destroy must be done async to ensure the correct internal timer is destroyed instead of the one that triggered this timeout - setTimeout(() => client.destroy()) + client.destroy() }) }) @@ -597,8 +595,7 @@ test('client should emit 2 timeouts when no responses are received', (t) => { client.on('timeout', () => { t.ok(1, 'timeout should have happened') if (count++ > 0) { - // client.destroy must be done async to ensure the correct internal timer is destroyed instead of the one that triggered this timeout - setTimeout(() => client.destroy()) + client.destroy() } }) })