From 1c6135f4315f6d16a43222b48dc95e1902e168a3 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 19 Jul 2017 23:12:32 -0700 Subject: [PATCH] test: fix flaky http(s)-set-server-timeout tests The tests include a callback that might not be invoked but is wrapped in common.mustCall(). Remove the common.mustCall() wrapper and add a comment explaining that it should not be added. Add a new test case that sets the timeout to 1ms and waits for both the connection handler and the timeout handler to be invoked. This version keeps the common.mustCall() wrapper intact around the connection handler (although it's mostly semantic and not necessary for the test as the test will certainly fail or time out if that handler isn't invoked). PR-URL: https://github.com/nodejs/node/pull/14380 Fixes: https://github.com/nodejs/node/issues/11768 Reviewed-By: Colin Ihrig Reviewed-By: Refael Ackermann Reviewed-By: James M Snell --- test/parallel/test-http-set-timeout-server.js | 59 +++++++++++++--- .../parallel/test-https-set-timeout-server.js | 67 +++++++++++++++---- 2 files changed, 102 insertions(+), 24 deletions(-) diff --git a/test/parallel/test-http-set-timeout-server.js b/test/parallel/test-http-set-timeout-server.js index 4eb41d01f96b26..7984fdfc0ffbf0 100644 --- a/test/parallel/test-http-set-timeout-server.js +++ b/test/parallel/test-http-set-timeout-server.js @@ -42,10 +42,7 @@ function run() { } test(function serverTimeout(cb) { - const server = http.createServer((req, res) => { - // Do nothing. We should get a timeout event. - // Might not be invoked. Do not wrap in common.mustCall(). - }); + const server = http.createServer(); server.listen(common.mustCall(() => { const s = server.setTimeout(50, common.mustCall((socket) => { socket.destroy(); @@ -125,11 +122,14 @@ test(function serverResponseTimeoutWithPipeline(cb) { const server = http.createServer((req, res) => { if (req.url === '/2') secReceived = true; + if (req.url === '/1') { + res.end(); + return; + } const s = res.setTimeout(50, () => { caughtTimeout += req.url; }); assert.ok(s instanceof http.OutgoingMessage); - if (req.url === '/1') res.end(); }); server.on('timeout', common.mustCall((socket) => { if (secReceived) { @@ -152,17 +152,56 @@ test(function serverResponseTimeoutWithPipeline(cb) { }); test(function idleTimeout(cb) { - const server = http.createServer(common.mustCall((req, res) => { - req.on('timeout', common.mustNotCall()); - res.on('timeout', common.mustNotCall()); - res.end(); - })); + // Test that the an idle connection invokes the timeout callback. + const server = http.createServer(); const s = server.setTimeout(50, common.mustCall((socket) => { socket.destroy(); server.close(); cb(); })); assert.ok(s instanceof http.Server); + server.listen(common.mustCall(() => { + const options = { + port: server.address().port, + allowHalfOpen: true, + }; + const c = net.connect(options, () => { + // ECONNRESET could happen on a heavily-loaded server. + c.on('error', (e) => { + if (e.message !== 'read ECONNRESET') + throw e; + }); + c.write('GET /1 HTTP/1.1\r\nHost: localhost\r\n\r\n'); + // Keep-Alive + }); + })); +}); + +test(function fastTimeout(cb) { + let connectionHandlerInvoked = false; + let timeoutHandlerInvoked = false; + let connectionSocket; + + function invokeCallbackIfDone() { + if (connectionHandlerInvoked && timeoutHandlerInvoked) { + connectionSocket.destroy(); + server.close(); + cb(); + } + } + + const server = http.createServer(common.mustCall((req, res) => { + req.on('timeout', common.mustNotCall()); + res.end(); + connectionHandlerInvoked = true; + invokeCallbackIfDone(); + })); + const s = server.setTimeout(1, common.mustCall((socket) => { + connectionSocket = socket; + timeoutHandlerInvoked = true; + invokeCallbackIfDone(); + })); + assert.ok(s instanceof http.Server); server.listen(common.mustCall(() => { const options = { port: server.address().port, diff --git a/test/parallel/test-https-set-timeout-server.js b/test/parallel/test-https-set-timeout-server.js index 76425385233dee..b5cc8a6f5a2664 100644 --- a/test/parallel/test-https-set-timeout-server.js +++ b/test/parallel/test-https-set-timeout-server.js @@ -52,12 +52,7 @@ function run() { } test(function serverTimeout(cb) { - const server = https.createServer( - serverOptions, - (req, res) => { - // Do nothing. We should get a timeout event. - // Might not be invoked. Do not wrap in common.mustCall(). - }); + const server = https.createServer(serverOptions); server.listen(common.mustCall(() => { const s = server.setTimeout(50, common.mustCall((socket) => { socket.destroy(); @@ -147,11 +142,14 @@ test(function serverResponseTimeoutWithPipeline(cb) { const server = https.createServer(serverOptions, (req, res) => { if (req.url === '/2') secReceived = true; + if (req.url === '/1') { + res.end(); + return; + } const s = res.setTimeout(50, () => { caughtTimeout += req.url; }); assert.ok(s instanceof http.OutgoingMessage); - if (req.url === '/1') res.end(); }); server.on('timeout', common.mustCall((socket) => { if (secReceived) { @@ -175,19 +173,60 @@ test(function serverResponseTimeoutWithPipeline(cb) { }); test(function idleTimeout(cb) { - const server = https.createServer( - serverOptions, - common.mustCall((req, res) => { - req.on('timeout', common.mustNotCall()); - res.on('timeout', common.mustNotCall()); - res.end(); - })); + // Test that the an idle connection invokes the timeout callback. + const server = https.createServer(serverOptions); const s = server.setTimeout(50, common.mustCall((socket) => { socket.destroy(); server.close(); cb(); })); assert.ok(s instanceof https.Server); + server.listen(common.mustCall(() => { + const options = { + port: server.address().port, + allowHalfOpen: true, + rejectUnauthorized: false + }; + const c = tls.connect(options, () => { + // ECONNRESET could happen on a heavily-loaded server. + c.on('error', (e) => { + if (e.message !== 'read ECONNRESET') + throw e; + }); + c.write('GET /1 HTTP/1.1\r\nHost: localhost\r\n\r\n'); + // Keep-Alive + }); + })); +}); + +test(function fastTimeout(cb) { + // Test that the socket timeout fires but no timeout fires for the request. + let connectionHandlerInvoked = false; + let timeoutHandlerInvoked = false; + let connectionSocket; + + function invokeCallbackIfDone() { + if (connectionHandlerInvoked && timeoutHandlerInvoked) { + connectionSocket.destroy(); + server.close(); + cb(); + } + } + + const server = https.createServer(serverOptions, common.mustCall( + (req, res) => { + req.on('timeout', common.mustNotCall()); + res.end(); + connectionHandlerInvoked = true; + invokeCallbackIfDone(); + } + )); + const s = server.setTimeout(1, common.mustCall((socket) => { + connectionSocket = socket; + timeoutHandlerInvoked = true; + invokeCallbackIfDone(); + })); + assert.ok(s instanceof https.Server); server.listen(common.mustCall(() => { const options = { port: server.address().port,