From 771c2ac7c295a773445d86e1dcd6d2722aff2137 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 14 Sep 2017 11:05:10 +0200 Subject: [PATCH] http: revert #14024 writable is never set to false Setting writable = false in IncomingMessage.end made some errors being swallowed in some very popular OSS libraries that we must support. This commit add some of those use cases to the tests, so we avoid further regressions. We should reevaluate how to set writable = false in IncomingMessage in a way that does not break the ecosystem. See: https://github.com/nodejs/node/pull/14024 Fixes: https://github.com/nodejs/node/issues/15029 PR-URL: https://github.com/nodejs/node/pull/15404 Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Ruben Bridgewater --- lib/_http_outgoing.js | 1 - .../test-http-outgoing-finish-writable.js | 13 +++++-- .../test-http-writable-true-after-close.js | 35 +++++++++++++++++++ ...tgoing-message-data-emitted-after-ended.js | 9 ++--- 4 files changed, 50 insertions(+), 8 deletions(-) create mode 100644 test/parallel/test-http-writable-true-after-close.js diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 03a121ce7edfaa..eedda7c8cb11ed 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -769,7 +769,6 @@ 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 index 23b456b4354e4d..033e9b6bc1a7db 100644 --- a/test/parallel/test-http-outgoing-finish-writable.js +++ b/test/parallel/test-http-outgoing-finish-writable.js @@ -4,13 +4,16 @@ 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. +// inherits from `OutgoingMessage`), its `writable` property is not set to false const server = http.createServer(common.mustCall(function(req, res) { assert.strictEqual(res.writable, true); assert.strictEqual(res.finished, false); res.end(); - assert.strictEqual(res.writable, false); + + // res.writable is set to false after it has finished sending + // Ref: https://github.com/nodejs/node/issues/15029 + assert.strictEqual(res.writable, true); assert.strictEqual(res.finished, true); server.close(); @@ -27,5 +30,9 @@ server.on('listening', common.mustCall(function() { assert.strictEqual(clientRequest.writable, true); clientRequest.end(); - assert.strictEqual(clientRequest.writable, false); + + // writable is still true when close + // THIS IS LEGACY, we cannot change it + // unless we break error detection + assert.strictEqual(clientRequest.writable, true); })); diff --git a/test/parallel/test-http-writable-true-after-close.js b/test/parallel/test-http-writable-true-after-close.js new file mode 100644 index 00000000000000..3cac10bddedc05 --- /dev/null +++ b/test/parallel/test-http-writable-true-after-close.js @@ -0,0 +1,35 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { get, createServer } = require('http'); + +// res.writable should not be set to false after it has finished sending +// Ref: https://github.com/nodejs/node/issues/15029 + +let external; + +// Http server +const internal = createServer((req, res) => { + res.writeHead(200); + setImmediate(common.mustCall(() => { + external.abort(); + res.end('Hello World\n'); + })); +}).listen(0); + +// Proxy server +const server = createServer(common.mustCall((req, res) => { + get(`http://127.0.0.1:${internal.address().port}`, common.mustCall((inner) => { + res.on('close', common.mustCall(() => { + assert.strictEqual(res.writable, true); + })); + inner.pipe(res); + })); +})).listen(0, () => { + external = get(`http://127.0.0.1:${server.address().port}`); + external.on('error', common.mustCall((err) => { + server.close(); + internal.close(); + })); +}); 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 6a5c170e7121cb..1614955be829c8 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,14 +1,13 @@ 'use strict'; const common = require('../common'); const http = require('http'); +const assert = require('assert'); 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). +// `OutgoingMessage` was closed - a `write after end` error is raised function MyStream() { stream.call(this); @@ -22,8 +21,10 @@ const server = http.createServer(common.mustCall(function(req, res) { process.nextTick(common.mustCall(() => { res.end(); myStream.emit('data', 'some data'); + res.on('error', common.mustCall(function(err) { + assert.strictEqual(err.message, 'write after end'); + })); - // If we got here - 'write after end' wasn't raised and the test passed. process.nextTick(common.mustCall(() => server.close())); })); }));