From d005f490a8de7c4136acdb4d1cd1dc2a7b5e3535 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 16 Feb 2020 10:47:13 +0100 Subject: [PATCH] http: cleanup end argument handling PR-URL: https://github.com/nodejs/node/pull/31818 Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater --- lib/_http_outgoing.js | 85 ++++++++++--------- test/parallel/test-http-outgoing-destroy.js | 22 +++++ test/parallel/test-http-outgoing-finish.js | 8 +- test/parallel/test-http-outgoing-proto.js | 18 ++-- .../parallel/test-http-res-write-after-end.js | 4 +- 5 files changed, 86 insertions(+), 51 deletions(-) create mode 100644 test/parallel/test-http-outgoing-destroy.js diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 5fd462b0194387..f8685c77f56770 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -56,7 +56,9 @@ const { ERR_METHOD_NOT_IMPLEMENTED, ERR_STREAM_CANNOT_PIPE, ERR_STREAM_ALREADY_FINISHED, - ERR_STREAM_WRITE_AFTER_END + ERR_STREAM_WRITE_AFTER_END, + ERR_STREAM_NULL_VALUES, + ERR_STREAM_DESTROYED }, hideStackFrames } = require('internal/errors'); @@ -67,6 +69,8 @@ const { CRLF, debug } = common; const kCorked = Symbol('corked'); +function nop() {} + const RE_CONN_CLOSE = /(?:^|\W)close(?:$|\W)/i; const RE_TE_CHUNKED = common.chunkExpression; @@ -633,45 +637,74 @@ ObjectDefineProperty(OutgoingMessage.prototype, 'writableEnded', { const crlf_buf = Buffer.from('\r\n'); OutgoingMessage.prototype.write = function write(chunk, encoding, callback) { + if (typeof encoding === 'function') { + callback = encoding; + encoding = null; + } + const ret = write_(this, chunk, encoding, callback, false); if (!ret) this[kNeedDrain] = true; return ret; }; -function writeAfterEnd(msg, callback) { - const err = new ERR_STREAM_WRITE_AFTER_END(); +function onError(msg, err, callback) { const triggerAsyncId = msg.socket ? msg.socket[async_id_symbol] : undefined; defaultTriggerAsyncIdScope(triggerAsyncId, process.nextTick, - writeAfterEndNT, + emitErrorNt, msg, err, callback); } +function emitErrorNt(msg, err, callback) { + callback(err); + if (typeof msg.emit === 'function') msg.emit('error', err); +} + function write_(msg, chunk, encoding, callback, fromEnd) { + if (typeof callback !== 'function') + callback = nop; + + let len; + if (chunk === null) { + throw new ERR_STREAM_NULL_VALUES(); + } else if (typeof chunk === 'string') { + len = Buffer.byteLength(chunk, encoding); + } else if (chunk instanceof Buffer) { + len = chunk.length; + } else { + throw new ERR_INVALID_ARG_TYPE( + 'chunk', ['string', 'Buffer', 'Uint8Array'], chunk); + } + + let err; if (msg.finished) { - writeAfterEnd(msg, callback); - return true; + err = new ERR_STREAM_WRITE_AFTER_END(); + } else if (msg.destroyed) { + err = new ERR_STREAM_DESTROYED('write'); + } + + if (err) { + onError(msg, err, callback); + return false; } if (!msg._header) { + if (fromEnd) { + msg._contentLength = len; + } msg._implicitHeader(); } if (!msg._hasBody) { debug('This type of response MUST NOT have a body. ' + 'Ignoring write() calls.'); - if (callback) process.nextTick(callback); + process.nextTick(callback); return true; } - if (!fromEnd && typeof chunk !== 'string' && !(chunk instanceof Buffer)) { - throw new ERR_INVALID_ARG_TYPE('first argument', - ['string', 'Buffer'], chunk); - } - if (!fromEnd && msg.socket && !msg.socket.writableCorked) { msg.socket.cork(); process.nextTick(connectionCorkNT, msg.socket); @@ -679,12 +712,6 @@ function write_(msg, chunk, encoding, callback, fromEnd) { let ret; if (msg.chunkedEncoding && chunk.length !== 0) { - let len; - if (typeof chunk === 'string') - len = Buffer.byteLength(chunk, encoding); - else - len = chunk.length; - msg._send(len.toString(16), 'latin1', null); msg._send(crlf_buf, null, null); msg._send(chunk, encoding, null); @@ -698,12 +725,6 @@ function write_(msg, chunk, encoding, callback, fromEnd) { } -function writeAfterEndNT(msg, err, callback) { - msg.emit('error', err); - if (callback) callback(err); -} - - function connectionCorkNT(conn) { conn.uncork(); } @@ -745,6 +766,7 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) { if (typeof chunk === 'function') { callback = chunk; chunk = null; + encoding = null; } else if (typeof encoding === 'function') { callback = encoding; encoding = null; @@ -755,21 +777,6 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) { } if (chunk) { - if (typeof chunk !== 'string' && !(chunk instanceof Buffer)) { - throw new ERR_INVALID_ARG_TYPE('chunk', ['string', 'Buffer'], chunk); - } - - if (this.finished) { - writeAfterEnd(this, callback); - return this; - } - - if (!this._header) { - if (typeof chunk === 'string') - this._contentLength = Buffer.byteLength(chunk, encoding); - else - this._contentLength = chunk.length; - } write_(this, chunk, encoding, null, true); } else if (this.finished) { if (typeof callback === 'function') { diff --git a/test/parallel/test-http-outgoing-destroy.js b/test/parallel/test-http-outgoing-destroy.js new file mode 100644 index 00000000000000..91c68aa9af3db0 --- /dev/null +++ b/test/parallel/test-http-outgoing-destroy.js @@ -0,0 +1,22 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +const http = require('http'); +const OutgoingMessage = http.OutgoingMessage; + +{ + const msg = new OutgoingMessage(); + assert.strictEqual(msg.destroyed, false); + msg.destroy(); + assert.strictEqual(msg.destroyed, true); + let callbackCalled = false; + msg.write('asd', common.mustCall((err) => { + assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED'); + callbackCalled = true; + })); + msg.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED'); + assert.strictEqual(callbackCalled, true); + })); +} diff --git a/test/parallel/test-http-outgoing-finish.js b/test/parallel/test-http-outgoing-finish.js index d5b94eb0f65b8f..7464dd57ea1f31 100644 --- a/test/parallel/test-http-outgoing-finish.js +++ b/test/parallel/test-http-outgoing-finish.js @@ -20,7 +20,7 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. 'use strict'; -require('../common'); +const common = require('../common'); const assert = require('assert'); const http = require('http'); @@ -49,7 +49,7 @@ function write(out) { let endCb = false; // First, write until it gets some backpressure - while (out.write(buf)) {} + while (out.write(buf, common.mustCall())) {} // Now end, and make sure that we don't get the 'finish' event // before the tick where the cb gets called. We give it until @@ -65,12 +65,12 @@ function write(out) { }); }); - out.end(buf, function() { + out.end(buf, common.mustCall(function() { endCb = true; console.error(`${name} endCb`); process.nextTick(function() { assert(finishEvent, `${name} got endCb event before finishEvent!`); console.log(`ok - ${name} endCb`); }); - }); + })); } diff --git a/test/parallel/test-http-outgoing-proto.js b/test/parallel/test-http-outgoing-proto.js index 4a07d18c601c5c..2d265c7ba64968 100644 --- a/test/parallel/test-http-outgoing-proto.js +++ b/test/parallel/test-http-outgoing-proto.js @@ -74,16 +74,14 @@ assert.throws(() => { ); } -assert(OutgoingMessage.prototype.write.call({ _header: 'test' })); - assert.throws(() => { const outgoingMessage = new OutgoingMessage(); outgoingMessage.write.call({ _header: 'test', _hasBody: 'test' }); }, { code: 'ERR_INVALID_ARG_TYPE', name: 'TypeError', - message: 'The first argument must be of type string or an instance of ' + - 'Buffer. Received undefined' + message: 'The "chunk" argument must be of type string or an instance of ' + + 'Buffer or Uint8Array. Received undefined' }); assert.throws(() => { @@ -92,8 +90,16 @@ assert.throws(() => { }, { code: 'ERR_INVALID_ARG_TYPE', name: 'TypeError', - message: 'The first argument must be of type string or an instance of ' + - 'Buffer. Received type number (1)' + message: 'The "chunk" argument must be of type string or an instance of ' + + 'Buffer or Uint8Array. Received type number (1)' +}); + +assert.throws(() => { + const outgoingMessage = new OutgoingMessage(); + outgoingMessage.write.call({ _header: 'test', _hasBody: 'test' }, null); +}, { + code: 'ERR_STREAM_NULL_VALUES', + name: 'TypeError' }); // addTrailers() diff --git a/test/parallel/test-http-res-write-after-end.js b/test/parallel/test-http-res-write-after-end.js index 70325975c4da0f..285938c8fe4d8c 100644 --- a/test/parallel/test-http-res-write-after-end.js +++ b/test/parallel/test-http-res-write-after-end.js @@ -34,8 +34,8 @@ const server = http.Server(common.mustCall(function(req, res) { res.end(); const r = res.write('This should raise an error.'); - // Write after end should return true - assert.strictEqual(r, true); + // Write after end should return false + assert.strictEqual(r, false); })); server.listen(0, function() {