From 78a7cbea494816102536a201351dd148599b610a Mon Sep 17 00:00:00 2001 From: Andrew Johnston Date: Sat, 17 Feb 2018 19:03:12 +0100 Subject: [PATCH 1/4] http: prevent aborted event when request is complete When socket is closed on a response for a request that is being piped to a stream there is a condition where aborted event will be fired to http client when socket is closing and the incomingMessage stream is still set to readable. We need a check for request being complete and to only raise the 'aborted' event on the http client if we have not yet completed reading the response from the server. Fixes: https://github.com/nodejs/node/issues/18756 --- lib/_http_client.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index cab11ae8f296c0..d351ded05518d8 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -321,7 +321,7 @@ function socketCloseListener() { var parser = socket.parser; if (req.res && req.res.readable) { // Socket closed before we emitted 'end' below. - req.res.emit('aborted'); + if (!req.res.complete) req.res.emit('aborted'); var res = req.res; res.on('end', function() { res.emit('close'); From 68850efe7427d6619dda4a28c03d926def362a67 Mon Sep 17 00:00:00 2001 From: Andrew Johnston Date: Sat, 17 Feb 2018 19:04:40 +0100 Subject: [PATCH 2/4] http: prevent aborted event when request is complete Tests in progress to reproduce issue consistently. Fixes: https://github.com/nodejs/node/issues/18756 --- .../test-http-client-spurious-aborted.js | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 test/parallel/test-http-client-spurious-aborted.js diff --git a/test/parallel/test-http-client-spurious-aborted.js b/test/parallel/test-http-client-spurious-aborted.js new file mode 100644 index 00000000000000..8a8d7d17e4ac54 --- /dev/null +++ b/test/parallel/test-http-client-spurious-aborted.js @@ -0,0 +1,93 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const assert = require('assert'); +const fs = require('fs'); +const Countdown = require('../common/countdown'); +const { URL } = require('url'); + +function cleanup(fname) { + try { + if (fs.statSync(fname)) fs.unlinkSync(fname); + } catch (err) {} +} + +const N = 1; +const fname = '/dev/null'; +const keepAlive = false; +const useRemote = process.env.SPURIOUS_ABORTED_REMOTE === 'on'; +let url = new URL('http://www.pdf995.com/samples/pdf.pdf'); +let server; + +const keepAliveAgent = new http.Agent({ keepAlive: true }); +const reqCountdown = new Countdown(N, common.mustCall()); + +function download() { + const opts = { + host: url.hostname, + path: url.pathname, + port: url.port + }; + if (keepAlive) opts.agent = keepAliveAgent; + const req = http.get(opts); + req.on('error', common.mustNotCall()); + req.on('response', (res) => { + assert.strictEqual(res.statusCode, 200); + if (keepAlive) { + assert.strictEqual(res.headers.connection, 'keep-alive'); + } else { + assert.strictEqual(res.headers.connection, 'close'); + } + let aborted = false; + const fstream = fs.createWriteStream(fname); + res.pipe(fstream); + res.on('end', common.mustCall(() => { + reqCountdown.dec(); + })); + res.on('aborted', () => { + aborted = true; + }); + res.on('error', common.mustNotCall()); + fstream.on('finish', () => { + assert.strictEqual(aborted, false); + cleanup(fname); + finishCountdown.dec(); + if (finishCountdown.remaining === 0) return; + download(); + }); + }); + req.end(); +} + +const finishCountdown = new Countdown(N, common.mustCall(() => { + if (!useRemote) server.close(); +})); + +if (useRemote) { + cleanup(fname); + download(); +} else { + server = http.Server(common.mustCall((req, res) => { + const headers = { 'Content-Type': 'text/plain' }; + if (req.url === '/content-length') { + headers['Content-Length'] = 50; + } + const socket = res.socket; + res.writeHead(200, headers); + setTimeout(() => res.write('aaaaaaaaaa'), 100); + setTimeout(() => res.write('bbbbbbbbbb'), 200); + setTimeout(() => res.write('cccccccccc'), 300); + setTimeout(() => res.write('dddddddddd'), 400); + setTimeout(() => { + res.end('eeeeeeeeee'); + const endAfter = Math.floor(Math.random() * 400); + setTimeout(() => socket.destroy(), endAfter); + }, 600); + }, N)); + server.listen(0, common.mustCall(() => { + url = new URL(`http://127.0.0.1:${server.address().port}/content-length`); + cleanup(fname); + download(); + })); +} From cfc03aa515ea59529c97f6b42e1c15b94dd26e25 Mon Sep 17 00:00:00 2001 From: Andrew Johnston Date: Sun, 18 Feb 2018 19:28:16 +0100 Subject: [PATCH 3/4] working --- .../test-http-client-spurious-aborted.js | 88 ++++++++----------- 1 file changed, 39 insertions(+), 49 deletions(-) diff --git a/test/parallel/test-http-client-spurious-aborted.js b/test/parallel/test-http-client-spurious-aborted.js index 8a8d7d17e4ac54..1b5f6301a65928 100644 --- a/test/parallel/test-http-client-spurious-aborted.js +++ b/test/parallel/test-http-client-spurious-aborted.js @@ -5,7 +5,6 @@ const http = require('http'); const assert = require('assert'); const fs = require('fs'); const Countdown = require('../common/countdown'); -const { URL } = require('url'); function cleanup(fname) { try { @@ -13,35 +12,57 @@ function cleanup(fname) { } catch (err) {} } -const N = 1; +const N = 2; const fname = '/dev/null'; -const keepAlive = false; -const useRemote = process.env.SPURIOUS_ABORTED_REMOTE === 'on'; -let url = new URL('http://www.pdf995.com/samples/pdf.pdf'); -let server; +let abortRequest = true; -const keepAliveAgent = new http.Agent({ keepAlive: true }); +const server = http.Server(common.mustCall((req, res) => { + const headers = { 'Content-Type': 'text/plain' }; + headers['Content-Length'] = 50; + const socket = res.socket; + res.writeHead(200, headers); + setTimeout(() => res.write('aaaaaaaaaa'), 100); + setTimeout(() => res.write('bbbbbbbbbb'), 200); + setTimeout(() => res.write('cccccccccc'), 300); + setTimeout(() => res.write('dddddddddd'), 400); + if (abortRequest) { + setTimeout(() => socket.destroy(), 600); + } else { + setTimeout(() => res.end('eeeeeeeeee'), 1000); + } +}, N)); + +server.listen(0, common.mustCall(() => { + cleanup(fname); + download(); +})); + +const finishCountdown = new Countdown(N, common.mustCall(() => { + server.close(); +})); const reqCountdown = new Countdown(N, common.mustCall()); function download() { const opts = { - host: url.hostname, - path: url.pathname, - port: url.port + port: server.address().port, + path: '/', }; - if (keepAlive) opts.agent = keepAliveAgent; const req = http.get(opts); req.on('error', common.mustNotCall()); req.on('response', (res) => { assert.strictEqual(res.statusCode, 200); - if (keepAlive) { - assert.strictEqual(res.headers.connection, 'keep-alive'); - } else { - assert.strictEqual(res.headers.connection, 'close'); - } + assert.strictEqual(res.headers.connection, 'close'); let aborted = false; const fstream = fs.createWriteStream(fname); res.pipe(fstream); + const _handle = res.socket._handle; + _handle._close = res.socket._handle.close; + _handle.close = function(callback) { + _handle._close(); + // set readable to true event though request is complete + if (res.complete) res.readable = true; + callback(); + }; res.on('end', common.mustCall(() => { reqCountdown.dec(); })); @@ -50,44 +71,13 @@ function download() { }); res.on('error', common.mustNotCall()); fstream.on('finish', () => { - assert.strictEqual(aborted, false); + assert.strictEqual(aborted, abortRequest); cleanup(fname); finishCountdown.dec(); if (finishCountdown.remaining === 0) return; + abortRequest = false; // next one should be a good response download(); }); }); req.end(); } - -const finishCountdown = new Countdown(N, common.mustCall(() => { - if (!useRemote) server.close(); -})); - -if (useRemote) { - cleanup(fname); - download(); -} else { - server = http.Server(common.mustCall((req, res) => { - const headers = { 'Content-Type': 'text/plain' }; - if (req.url === '/content-length') { - headers['Content-Length'] = 50; - } - const socket = res.socket; - res.writeHead(200, headers); - setTimeout(() => res.write('aaaaaaaaaa'), 100); - setTimeout(() => res.write('bbbbbbbbbb'), 200); - setTimeout(() => res.write('cccccccccc'), 300); - setTimeout(() => res.write('dddddddddd'), 400); - setTimeout(() => { - res.end('eeeeeeeeee'); - const endAfter = Math.floor(Math.random() * 400); - setTimeout(() => socket.destroy(), endAfter); - }, 600); - }, N)); - server.listen(0, common.mustCall(() => { - url = new URL(`http://127.0.0.1:${server.address().port}/content-length`); - cleanup(fname); - download(); - })); -} From 24e575519a170ad59fc62dc62b8c6308e0f88499 Mon Sep 17 00:00:00 2001 From: Andrew Johnston Date: Tue, 27 Feb 2018 02:17:00 +0100 Subject: [PATCH 4/4] fix(test): refactor test after review --- .../test-http-client-spurious-aborted.js | 32 +++++++------------ 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/test/parallel/test-http-client-spurious-aborted.js b/test/parallel/test-http-client-spurious-aborted.js index 1b5f6301a65928..58a2f92de94054 100644 --- a/test/parallel/test-http-client-spurious-aborted.js +++ b/test/parallel/test-http-client-spurious-aborted.js @@ -3,17 +3,10 @@ const common = require('../common'); const http = require('http'); const assert = require('assert'); -const fs = require('fs'); +const { Writable } = require('stream'); const Countdown = require('../common/countdown'); -function cleanup(fname) { - try { - if (fs.statSync(fname)) fs.unlinkSync(fname); - } catch (err) {} -} - const N = 2; -const fname = '/dev/null'; let abortRequest = true; const server = http.Server(common.mustCall((req, res) => { @@ -21,19 +14,15 @@ const server = http.Server(common.mustCall((req, res) => { headers['Content-Length'] = 50; const socket = res.socket; res.writeHead(200, headers); - setTimeout(() => res.write('aaaaaaaaaa'), 100); - setTimeout(() => res.write('bbbbbbbbbb'), 200); - setTimeout(() => res.write('cccccccccc'), 300); - setTimeout(() => res.write('dddddddddd'), 400); + res.write('aaaaaaaaaabbbbbbbbbbccccccccccdddddddddd'); if (abortRequest) { - setTimeout(() => socket.destroy(), 600); + process.nextTick(() => socket.destroy()); } else { - setTimeout(() => res.end('eeeeeeeeee'), 1000); + process.nextTick(() => res.end('eeeeeeeeee')); } }, N)); server.listen(0, common.mustCall(() => { - cleanup(fname); download(); })); @@ -53,13 +42,17 @@ function download() { assert.strictEqual(res.statusCode, 200); assert.strictEqual(res.headers.connection, 'close'); let aborted = false; - const fstream = fs.createWriteStream(fname); - res.pipe(fstream); + const writable = new Writable({ + write(chunk, encoding, callback) { + callback(); + } + }); + res.pipe(writable); const _handle = res.socket._handle; _handle._close = res.socket._handle.close; _handle.close = function(callback) { _handle._close(); - // set readable to true event though request is complete + // set readable to true even though request is complete if (res.complete) res.readable = true; callback(); }; @@ -70,9 +63,8 @@ function download() { aborted = true; }); res.on('error', common.mustNotCall()); - fstream.on('finish', () => { + writable.on('finish', () => { assert.strictEqual(aborted, abortRequest); - cleanup(fname); finishCountdown.dec(); if (finishCountdown.remaining === 0) return; abortRequest = false; // next one should be a good response