From 8612e364a9df7308c08e938e784fae91d4b2fb6f Mon Sep 17 00:00:00 2001 From: Devin Nakamura Date: Mon, 14 Sep 2015 14:29:49 -0400 Subject: [PATCH 1/2] test: fix flaky test test-http-pipeline-flood --- test/sequential/test-http-pipeline-flood.js | 39 ++++++++++++++------- 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/test/sequential/test-http-pipeline-flood.js b/test/sequential/test-http-pipeline-flood.js index cb9fc97a865b70..f8de38c7a84a3a 100644 --- a/test/sequential/test-http-pipeline-flood.js +++ b/test/sequential/test-http-pipeline-flood.js @@ -2,6 +2,16 @@ var common = require('../common'); var assert = require('assert'); +// Here we are testing the HTTP server module's flood prevention mechanism. +// When writeable.write returns false (ie the underlying send() indicated the +// native buffer is full), the HTTP server cork()s the readable part of the +// stream. This means that new requests will not be read (however request which +// have already been read, but are awaiting processing will still be +// processed). + +// Normally when the writable stream emits a 'drain' event, the server then +// uncorks the readable stream, although we arent testing that part here. + switch (process.argv[2]) { case undefined: return parent(); @@ -18,22 +28,31 @@ function parent() { var childClosed = false; var requests = 0; var connections = 0; + var backloggedReqs = 0; var server = http.createServer(function(req, res) { requests++; res.setHeader('content-length', bigResponse.length); - res.end(bigResponse); + if (!res.write(bigResponse)) { + if (backloggedReqs == 0) { + // Once the native buffer fills (ie write() returns false), the flood + // prevention should kick in. + // This means the stream should emit no more 'data' events. However we + // may still be asked to process more requests if they were read before + // mechanism activated. + req.socket.on('data', function() { assert(false); }); + } + backloggedReqs++; + } + res.end(); }); server.on('connection', function(conn) { connections++; }); - // kill the connection after a bit, verifying that the - // flood of requests was eventually halted. server.setTimeout(200, function(conn) { gotTimeout = true; - conn.destroy(); }); server.listen(common.PORT, function() { @@ -53,9 +72,9 @@ function parent() { assert.equal(connections, 1); // The number of requests we end up processing before the outgoing // connection backs up and requires a drain is implementation-dependent. - // We can safely assume is more than 250. console.log('server got %d requests', requests); - assert(requests >= 250); + console.log('server sent %d backlogged requests', backloggedReqs); + console.log('ok'); }); } @@ -63,7 +82,6 @@ function parent() { function child() { var net = require('net'); - var gotEpipe = false; var conn = net.connect({ port: common.PORT }); var req = 'GET / HTTP/1.1\r\nHost: localhost:' + @@ -72,17 +90,14 @@ function child() { req = new Array(10241).join(req); conn.on('connect', function() { + //kill child after 1s of flooding + setTimeout(function() { conn.destroy(); }, 1000); write(); }); conn.on('drain', write); - conn.on('error', function(er) { - gotEpipe = true; - }); - process.on('exit', function() { - assert(gotEpipe); console.log('ok - child'); }); From f003c846c16e7761217e0c1f43fe4d8dfe1099a4 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 2 Nov 2015 15:39:21 -0800 Subject: [PATCH 2/2] test: refactor test-http-pipeline-flood --- test/sequential/sequential.status | 1 - test/sequential/test-http-pipeline-flood.js | 56 +++++++++------------ 2 files changed, 24 insertions(+), 33 deletions(-) diff --git a/test/sequential/sequential.status b/test/sequential/sequential.status index 06057555d7d98d..7d5e6f1c1e624f 100644 --- a/test/sequential/sequential.status +++ b/test/sequential/sequential.status @@ -7,7 +7,6 @@ prefix sequential [true] # This section applies to all platforms [$system==win32] -test-http-pipeline-flood : PASS,FLAKY [$system==linux] test-vm-syntax-error-stderr : PASS,FLAKY diff --git a/test/sequential/test-http-pipeline-flood.js b/test/sequential/test-http-pipeline-flood.js index f8de38c7a84a3a..571636ffed2e5f 100644 --- a/test/sequential/test-http-pipeline-flood.js +++ b/test/sequential/test-http-pipeline-flood.js @@ -1,6 +1,6 @@ 'use strict'; -var common = require('../common'); -var assert = require('assert'); +const common = require('../common'); +const assert = require('assert'); // Here we are testing the HTTP server module's flood prevention mechanism. // When writeable.write returns false (ie the underlying send() indicated the @@ -18,29 +18,31 @@ switch (process.argv[2]) { case 'child': return child(); default: - throw new Error('wtf'); + throw new Error(`Unexpected value: ${process.argv[2]}`); } function parent() { - var http = require('http'); - var bigResponse = new Buffer(10240).fill('x'); + const http = require('http'); + const bigResponse = new Buffer(10240).fill('x'); var gotTimeout = false; var childClosed = false; var requests = 0; var connections = 0; var backloggedReqs = 0; - var server = http.createServer(function(req, res) { + const server = http.createServer(function(req, res) { requests++; res.setHeader('content-length', bigResponse.length); if (!res.write(bigResponse)) { - if (backloggedReqs == 0) { + if (backloggedReqs === 0) { // Once the native buffer fills (ie write() returns false), the flood // prevention should kick in. // This means the stream should emit no more 'data' events. However we // may still be asked to process more requests if they were read before - // mechanism activated. - req.socket.on('data', function() { assert(false); }); + // the flood-prevention mechanism activated. + setImmediate(() => { + req.socket.on('data', () => common.fail('Unexpected data received')); + }); } backloggedReqs++; } @@ -51,38 +53,32 @@ function parent() { connections++; }); - server.setTimeout(200, function(conn) { - gotTimeout = true; - }); - server.listen(common.PORT, function() { - var spawn = require('child_process').spawn; - var args = [__filename, 'child']; - var child = spawn(process.execPath, args, { stdio: 'inherit' }); - child.on('close', function(code) { - assert(!code); + const spawn = require('child_process').spawn; + const args = [__filename, 'child']; + const child = spawn(process.execPath, args, { stdio: 'inherit' }); + child.on('close', function() { childClosed = true; server.close(); }); + + server.setTimeout(common.platformTimeout(200), function(conn) { + gotTimeout = true; + child.kill(); + }); }); process.on('exit', function() { assert(gotTimeout); assert(childClosed); assert.equal(connections, 1); - // The number of requests we end up processing before the outgoing - // connection backs up and requires a drain is implementation-dependent. - console.log('server got %d requests', requests); - console.log('server sent %d backlogged requests', backloggedReqs); - - console.log('ok'); }); } function child() { - var net = require('net'); + const net = require('net'); - var conn = net.connect({ port: common.PORT }); + const conn = net.connect({ port: common.PORT }); var req = 'GET / HTTP/1.1\r\nHost: localhost:' + common.PORT + '\r\nAccept: */*\r\n\r\n'; @@ -90,17 +86,13 @@ function child() { req = new Array(10241).join(req); conn.on('connect', function() { - //kill child after 1s of flooding - setTimeout(function() { conn.destroy(); }, 1000); + // Terminate child after flooding. + setTimeout(function() { conn.destroy(); }, common.platformTimeout(1000)); write(); }); conn.on('drain', write); - process.on('exit', function() { - console.log('ok - child'); - }); - function write() { while (false !== conn.write(req, 'ascii')); }