From eedefc7be32138a34a8cc186d37755d2a55155aa Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 30 May 2017 13:06:52 -0700 Subject: [PATCH 1/3] test: improve test-https-server-keep-alive-timeout The test is flaky under load. These changes greatly improve reliability. * Use a recurring interval to determine when the test should end rather than a timer. * Increase server timeout to 500ms to allow for events being delayed by system load Changing to an interval has the added benefit of reducing the test run time from over 2 seconds to under 1 second. Fixes: https://github.com/nodejs/node/issues/13307 --- .../test-https-server-keep-alive-timeout.js | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/test/parallel/test-https-server-keep-alive-timeout.js b/test/parallel/test-https-server-keep-alive-timeout.js index c89041489e0aef..96d81b4812b716 100644 --- a/test/parallel/test-https-server-keep-alive-timeout.js +++ b/test/parallel/test-https-server-keep-alive-timeout.js @@ -31,20 +31,18 @@ function run() { test(function serverKeepAliveTimeoutWithPipeline(cb) { let socket; - let destroyedSockets = 0; let timeoutCount = 0; let requestCount = 0; process.on('exit', function() { assert.strictEqual(timeoutCount, 1); assert.strictEqual(requestCount, 3); - assert.strictEqual(destroyedSockets, 1); }); const server = https.createServer(serverOptions, (req, res) => { socket = req.socket; requestCount++; res.end(); }); - server.setTimeout(200, (socket) => { + server.setTimeout(500, (socket) => { timeoutCount++; socket.destroy(); }); @@ -60,29 +58,29 @@ test(function serverKeepAliveTimeoutWithPipeline(cb) { c.write('GET /2 HTTP/1.1\r\nHost: localhost\r\n\r\n'); c.write('GET /3 HTTP/1.1\r\nHost: localhost\r\n\r\n'); }); - setTimeout(() => { - server.close(); - if (socket.destroyed) destroyedSockets++; - cb(); - }, 1000); + const interval = setInterval(() => { + if (socket && socket.destroyed && requestCount === 3) { + clearInterval(interval); + server.close(); + cb(); + } + }, 1); })); }); test(function serverNoEndKeepAliveTimeoutWithPipeline(cb) { let socket; - let destroyedSockets = 0; let timeoutCount = 0; let requestCount = 0; process.on('exit', () => { assert.strictEqual(timeoutCount, 1); assert.strictEqual(requestCount, 3); - assert.strictEqual(destroyedSockets, 1); }); const server = https.createServer(serverOptions, (req, res) => { socket = req.socket; requestCount++; }); - server.setTimeout(200, (socket) => { + server.setTimeout(500, (socket) => { timeoutCount++; socket.destroy(); }); @@ -98,10 +96,12 @@ test(function serverNoEndKeepAliveTimeoutWithPipeline(cb) { c.write('GET /2 HTTP/1.1\r\nHost: localhost\r\n\r\n'); c.write('GET /3 HTTP/1.1\r\nHost: localhost\r\n\r\n'); }); - setTimeout(() => { - server.close(); - if (socket && socket.destroyed) destroyedSockets++; - cb(); - }, 1000); + const interval = setInterval(() => { + if (socket && socket.destroyed && requestCount === 3) { + clearInterval(interval); + server.close(); + cb(); + } + }, 1); })); }); From 80c1f44a263caff60212cfc6d0b020f929b2dc29 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 31 May 2017 10:03:41 -0700 Subject: [PATCH 2/3] squash: additional refactor per lpinca --- .../test-https-server-keep-alive-timeout.js | 36 +++++-------------- 1 file changed, 8 insertions(+), 28 deletions(-) diff --git a/test/parallel/test-https-server-keep-alive-timeout.js b/test/parallel/test-https-server-keep-alive-timeout.js index 96d81b4812b716..5529727011f40b 100644 --- a/test/parallel/test-https-server-keep-alive-timeout.js +++ b/test/parallel/test-https-server-keep-alive-timeout.js @@ -30,22 +30,19 @@ function run() { } test(function serverKeepAliveTimeoutWithPipeline(cb) { - let socket; - let timeoutCount = 0; let requestCount = 0; process.on('exit', function() { - assert.strictEqual(timeoutCount, 1); assert.strictEqual(requestCount, 3); }); const server = https.createServer(serverOptions, (req, res) => { - socket = req.socket; requestCount++; res.end(); }); - server.setTimeout(500, (socket) => { - timeoutCount++; + server.setTimeout(500, common.mustCall((socket) => { socket.destroy(); - }); + server.close(); + cb(); + })); server.keepAliveTimeout = 50; server.listen(0, common.mustCall(() => { const options = { @@ -58,32 +55,22 @@ test(function serverKeepAliveTimeoutWithPipeline(cb) { c.write('GET /2 HTTP/1.1\r\nHost: localhost\r\n\r\n'); c.write('GET /3 HTTP/1.1\r\nHost: localhost\r\n\r\n'); }); - const interval = setInterval(() => { - if (socket && socket.destroyed && requestCount === 3) { - clearInterval(interval); - server.close(); - cb(); - } - }, 1); })); }); test(function serverNoEndKeepAliveTimeoutWithPipeline(cb) { - let socket; - let timeoutCount = 0; let requestCount = 0; process.on('exit', () => { - assert.strictEqual(timeoutCount, 1); assert.strictEqual(requestCount, 3); }); const server = https.createServer(serverOptions, (req, res) => { - socket = req.socket; requestCount++; }); - server.setTimeout(500, (socket) => { - timeoutCount++; + server.setTimeout(500, common.mustCall((socket) => { socket.destroy(); - }); + server.close(); + cb(); + })); server.keepAliveTimeout = 50; server.listen(0, common.mustCall(() => { const options = { @@ -96,12 +83,5 @@ test(function serverNoEndKeepAliveTimeoutWithPipeline(cb) { c.write('GET /2 HTTP/1.1\r\nHost: localhost\r\n\r\n'); c.write('GET /3 HTTP/1.1\r\nHost: localhost\r\n\r\n'); }); - const interval = setInterval(() => { - if (socket && socket.destroyed && requestCount === 3) { - clearInterval(interval); - server.close(); - cb(); - } - }, 1); })); }); From e89d8ca863223835329105eaf8d0475e247358c7 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 1 Jun 2017 11:16:37 -0700 Subject: [PATCH 3/3] squash: nits from refack --- test/parallel/test-https-server-keep-alive-timeout.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/parallel/test-https-server-keep-alive-timeout.js b/test/parallel/test-https-server-keep-alive-timeout.js index 5529727011f40b..d1e9ed67889ac6 100644 --- a/test/parallel/test-https-server-keep-alive-timeout.js +++ b/test/parallel/test-https-server-keep-alive-timeout.js @@ -39,6 +39,7 @@ test(function serverKeepAliveTimeoutWithPipeline(cb) { res.end(); }); server.setTimeout(500, common.mustCall((socket) => { + // End this test and call `run()` for the next test (if any). socket.destroy(); server.close(); cb(); @@ -67,6 +68,7 @@ test(function serverNoEndKeepAliveTimeoutWithPipeline(cb) { requestCount++; }); server.setTimeout(500, common.mustCall((socket) => { + // End this test and call `run()` for the next test (if any). socket.destroy(); server.close(); cb();