From 20b6326928eedab1ea10c947b8261a05ea130ec8 Mon Sep 17 00:00:00 2001 From: Roee Kasher Date: Fri, 30 Jun 2017 23:45:12 +0300 Subject: [PATCH 1/4] http: OutgoingMessage change writable after end When an OutgoingMessage is closed (for example, using the `end` method), its 'writable' property should be changed to false - since it is not writable anymore. The 'writable' property should have the opposite value of the 'finished' property. --- lib/_http_outgoing.js | 1 + .../test-http-outgoing-finish-writable.js | 26 ++++++++++++++ ...tgoing-message-data-emitted-after-ended.js | 35 +++++++++++++++++++ 3 files changed, 62 insertions(+) create mode 100644 test/parallel/test-http-outgoing-finish-writable.js create mode 100644 test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index ccb66f742f91d0..8402bd2b61aaf4 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -793,6 +793,7 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) { this.connection.uncork(); this.finished = true; + this.writable = false; // There is the first message on the outgoing queue, and we've sent // everything to the socket. diff --git a/test/parallel/test-http-outgoing-finish-writable.js b/test/parallel/test-http-outgoing-finish-writable.js new file mode 100644 index 00000000000000..6e1eef398b0012 --- /dev/null +++ b/test/parallel/test-http-outgoing-finish-writable.js @@ -0,0 +1,26 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +const server = http.createServer(common.mustCall(function(req, res) { + assert(res.writable, 'Res should be writable when it is received \ + and opened.'); + assert(!res.finished, 'Res shouldn\'t be finished when it is received \ + and opened.'); + res.end(); + assert(!res.writable, 'Res shouldn\'t be writable after it was closed.'); + assert(res.finished, 'Res should be finished after it was closed.'); + + server.close(); +})); + +server.listen(0); + +server.on('listening', common.mustCall(function() { + http.request({ + port: server.address().port, + method: 'GET', + path: '/' + }).end(); +})); diff --git a/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js b/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js new file mode 100644 index 00000000000000..8b48955c457423 --- /dev/null +++ b/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js @@ -0,0 +1,35 @@ +'use strict'; +const common = require('../common'); +const http = require('http'); +const OutgoingMessage = require('_http_outgoing').OutgoingMessage; + +const server = http.createServer(common.mustCall(function(req, res) { + console.log('Got a request, piping an OutgoingMessage to it.'); + const outgointMessage = new OutgoingMessage(); + outgointMessage.pipe(res); + + setTimeout(() => { + console.log('Closing response.'); + res.end(); + outgointMessage.emit('data', 'some data'); + + // If we got here - 'write after end' wasn't raised and the test passed. + setTimeout(() => server.close(), 10); + }, 10); + + setInterval(() => { + console.log('Emitting some data to outgointMessage'); + outgointMessage.emit('data', 'some data'); + }, 1).unref(); + +})); + +server.listen(0); + +server.on('listening', common.mustCall(function() { + http.request({ + port: server.address().port, + method: 'GET', + path: '/' + }).end(); +})); From 17365ea11c310f29fad743e73d40a1f15b1f5a32 Mon Sep 17 00:00:00 2001 From: Roee Kasher Date: Tue, 11 Jul 2017 18:28:14 +0300 Subject: [PATCH 2/4] Review fixes. --- .../test-http-outgoing-finish-writable.js | 10 +++++-- ...tgoing-message-data-emitted-after-ended.js | 28 +++++++++---------- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/test/parallel/test-http-outgoing-finish-writable.js b/test/parallel/test-http-outgoing-finish-writable.js index 6e1eef398b0012..3ac29b7562c78f 100644 --- a/test/parallel/test-http-outgoing-finish-writable.js +++ b/test/parallel/test-http-outgoing-finish-writable.js @@ -18,9 +18,15 @@ const server = http.createServer(common.mustCall(function(req, res) { server.listen(0); server.on('listening', common.mustCall(function() { - http.request({ + const clientRequest = http.request({ port: server.address().port, method: 'GET', path: '/' - }).end(); + }); + + assert(clientRequest.writable, 'ClientRequest should be writable when \ + it is created.'); + clientRequest.end(); + assert(!clientRequest.writable, 'ClientRequest shouldn\'t be writable \ + after it was closed.'); })); diff --git a/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js b/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js index 8b48955c457423..16ac892899b47f 100644 --- a/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js +++ b/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js @@ -1,27 +1,27 @@ 'use strict'; const common = require('../common'); const http = require('http'); -const OutgoingMessage = require('_http_outgoing').OutgoingMessage; +const util = require('util'); +const stream = require('stream'); + +function MyStream() { + stream.call(this); +} +util.inherits(MyStream, stream); const server = http.createServer(common.mustCall(function(req, res) { - console.log('Got a request, piping an OutgoingMessage to it.'); - const outgointMessage = new OutgoingMessage(); - outgointMessage.pipe(res); + console.log('Got a request, piping a stream to it.'); + const myStream = new MyStream(); + myStream.pipe(res); - setTimeout(() => { + process.nextTick(() => { console.log('Closing response.'); res.end(); - outgointMessage.emit('data', 'some data'); + myStream.emit('data', 'some data'); // If we got here - 'write after end' wasn't raised and the test passed. - setTimeout(() => server.close(), 10); - }, 10); - - setInterval(() => { - console.log('Emitting some data to outgointMessage'); - outgointMessage.emit('data', 'some data'); - }, 1).unref(); - + process.nextTick(() => server.close()); + }); })); server.listen(0); From fd89a7e5707574553af9c75492a6ebb7b91b9293 Mon Sep 17 00:00:00 2001 From: Roee Kasher Date: Tue, 11 Jul 2017 20:02:34 +0300 Subject: [PATCH 3/4] Review fixes 2.0. --- .../test-http-outgoing-finish-writable.js | 19 +++++++++---------- ...tgoing-message-data-emitted-after-ended.js | 8 +++----- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/test/parallel/test-http-outgoing-finish-writable.js b/test/parallel/test-http-outgoing-finish-writable.js index 3ac29b7562c78f..23b456b4354e4d 100644 --- a/test/parallel/test-http-outgoing-finish-writable.js +++ b/test/parallel/test-http-outgoing-finish-writable.js @@ -3,14 +3,15 @@ const common = require('../common'); const assert = require('assert'); const http = require('http'); +// Verify that after calling end() on an `OutgoingMessage` (or a type that +// inherits from `OutgoingMessage`), its `writable` property is set to false. + const server = http.createServer(common.mustCall(function(req, res) { - assert(res.writable, 'Res should be writable when it is received \ - and opened.'); - assert(!res.finished, 'Res shouldn\'t be finished when it is received \ - and opened.'); + assert.strictEqual(res.writable, true); + assert.strictEqual(res.finished, false); res.end(); - assert(!res.writable, 'Res shouldn\'t be writable after it was closed.'); - assert(res.finished, 'Res should be finished after it was closed.'); + assert.strictEqual(res.writable, false); + assert.strictEqual(res.finished, true); server.close(); })); @@ -24,9 +25,7 @@ server.on('listening', common.mustCall(function() { path: '/' }); - assert(clientRequest.writable, 'ClientRequest should be writable when \ - it is created.'); + assert.strictEqual(clientRequest.writable, true); clientRequest.end(); - assert(!clientRequest.writable, 'ClientRequest shouldn\'t be writable \ - after it was closed.'); + assert.strictEqual(clientRequest.writable, false); })); diff --git a/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js b/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js index 16ac892899b47f..7eeecaf78ca688 100644 --- a/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js +++ b/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js @@ -10,18 +10,16 @@ function MyStream() { util.inherits(MyStream, stream); const server = http.createServer(common.mustCall(function(req, res) { - console.log('Got a request, piping a stream to it.'); const myStream = new MyStream(); myStream.pipe(res); - process.nextTick(() => { - console.log('Closing response.'); + process.nextTick(common.mustCall(() => { res.end(); myStream.emit('data', 'some data'); // If we got here - 'write after end' wasn't raised and the test passed. - process.nextTick(() => server.close()); - }); + process.nextTick(common.mustCall(() => server.close())); + })); })); server.listen(0); From 64d17bec19db2dc5c89cf231fdae042f50bacafc Mon Sep 17 00:00:00 2001 From: Roee Kasher Date: Thu, 13 Jul 2017 21:47:33 +0300 Subject: [PATCH 4/4] Review fixed 3.0 --- .../test-pipe-outgoing-message-data-emitted-after-ended.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js b/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js index 7eeecaf78ca688..6a5c170e7121cb 100644 --- a/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js +++ b/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js @@ -4,6 +4,12 @@ const http = require('http'); const util = require('util'); const stream = require('stream'); +// Verify that when piping a stream to an `OutgoingMessage` (or a type that +// inherits from `OutgoingMessage`), if data is emitted after the +// `OutgoingMessage` was closed - no `write after end` error is raised (this +// should be the case when piping - when writing data directly to the +// `OutgoingMessage` this error should be raised). + function MyStream() { stream.call(this); }