Skip to content

Commit d005f49

Browse files
ronagronag
committed
http: cleanup end argument handling
PR-URL: #31818 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
1 parent e50ae7a commit d005f49

File tree

5 files changed

+86
-51
lines changed

5 files changed

+86
-51
lines changed

‎lib/_http_outgoing.js‎

Lines changed: 46 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,9 @@ const {
5656
ERR_METHOD_NOT_IMPLEMENTED,
5757
ERR_STREAM_CANNOT_PIPE,
5858
ERR_STREAM_ALREADY_FINISHED,
59-
ERR_STREAM_WRITE_AFTER_END
59+
ERR_STREAM_WRITE_AFTER_END,
60+
ERR_STREAM_NULL_VALUES,
61+
ERR_STREAM_DESTROYED
6062
},
6163
hideStackFrames
6264
} = require('internal/errors');
@@ -67,6 +69,8 @@ const { CRLF, debug } = common;
6769

6870
const kCorked = Symbol('corked');
6971

72+
function nop() {}
73+
7074
const RE_CONN_CLOSE = /(?:^|\W)close(?:$|\W)/i;
7175
const RE_TE_CHUNKED = common.chunkExpression;
7276

@@ -633,58 +637,81 @@ ObjectDefineProperty(OutgoingMessage.prototype, 'writableEnded', {
633637

634638
const crlf_buf = Buffer.from('\r\n');
635639
OutgoingMessage.prototype.write = function write(chunk, encoding, callback) {
640+
if (typeof encoding === 'function') {
641+
callback = encoding;
642+
encoding = null;
643+
}
644+
636645
const ret = write_(this, chunk, encoding, callback, false);
637646
if (!ret)
638647
this[kNeedDrain] = true;
639648
return ret;
640649
};
641650

642-
function writeAfterEnd(msg, callback) {
643-
const err = new ERR_STREAM_WRITE_AFTER_END();
651+
function onError(msg, err, callback) {
644652
const triggerAsyncId = msg.socket ? msg.socket[async_id_symbol] : undefined;
645653
defaultTriggerAsyncIdScope(triggerAsyncId,
646654
process.nextTick,
647-
writeAfterEndNT,
655+
emitErrorNt,
648656
msg,
649657
err,
650658
callback);
651659
}
652660

661+
function emitErrorNt(msg, err, callback) {
662+
callback(err);
663+
if (typeof msg.emit === 'function') msg.emit('error', err);
664+
}
665+
653666
function write_(msg, chunk, encoding, callback, fromEnd) {
667+
if (typeof callback !== 'function')
668+
callback = nop;
669+
670+
let len;
671+
if (chunk === null) {
672+
throw new ERR_STREAM_NULL_VALUES();
673+
} else if (typeof chunk === 'string') {
674+
len = Buffer.byteLength(chunk, encoding);
675+
} else if (chunk instanceof Buffer) {
676+
len = chunk.length;
677+
} else {
678+
throw new ERR_INVALID_ARG_TYPE(
679+
'chunk', ['string', 'Buffer', 'Uint8Array'], chunk);
680+
}
681+
682+
let err;
654683
if (msg.finished) {
655-
writeAfterEnd(msg, callback);
656-
return true;
684+
err = new ERR_STREAM_WRITE_AFTER_END();
685+
} else if (msg.destroyed) {
686+
err = new ERR_STREAM_DESTROYED('write');
687+
}
688+
689+
if (err) {
690+
onError(msg, err, callback);
691+
return false;
657692
}
658693

659694
if (!msg._header) {
695+
if (fromEnd) {
696+
msg._contentLength = len;
697+
}
660698
msg._implicitHeader();
661699
}
662700

663701
if (!msg._hasBody) {
664702
debug('This type of response MUST NOT have a body. ' +
665703
'Ignoring write() calls.');
666-
if (callback) process.nextTick(callback);
704+
process.nextTick(callback);
667705
return true;
668706
}
669707

670-
if (!fromEnd && typeof chunk !== 'string' && !(chunk instanceof Buffer)) {
671-
throw new ERR_INVALID_ARG_TYPE('first argument',
672-
['string', 'Buffer'], chunk);
673-
}
674-
675708
if (!fromEnd && msg.socket && !msg.socket.writableCorked) {
676709
msg.socket.cork();
677710
process.nextTick(connectionCorkNT, msg.socket);
678711
}
679712

680713
let ret;
681714
if (msg.chunkedEncoding && chunk.length !== 0) {
682-
let len;
683-
if (typeof chunk === 'string')
684-
len = Buffer.byteLength(chunk, encoding);
685-
else
686-
len = chunk.length;
687-
688715
msg._send(len.toString(16), 'latin1', null);
689716
msg._send(crlf_buf, null, null);
690717
msg._send(chunk, encoding, null);
@@ -698,12 +725,6 @@ function write_(msg, chunk, encoding, callback, fromEnd) {
698725
}
699726

700727

701-
function writeAfterEndNT(msg, err, callback) {
702-
msg.emit('error', err);
703-
if (callback) callback(err);
704-
}
705-
706-
707728
function connectionCorkNT(conn) {
708729
conn.uncork();
709730
}
@@ -745,6 +766,7 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {
745766
if (typeof chunk === 'function') {
746767
callback = chunk;
747768
chunk = null;
769+
encoding = null;
748770
} else if (typeof encoding === 'function') {
749771
callback = encoding;
750772
encoding = null;
@@ -755,21 +777,6 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {
755777
}
756778

757779
if (chunk) {
758-
if (typeof chunk !== 'string' && !(chunk instanceof Buffer)) {
759-
throw new ERR_INVALID_ARG_TYPE('chunk', ['string', 'Buffer'], chunk);
760-
}
761-
762-
if (this.finished) {
763-
writeAfterEnd(this, callback);
764-
return this;
765-
}
766-
767-
if (!this._header) {
768-
if (typeof chunk === 'string')
769-
this._contentLength = Buffer.byteLength(chunk, encoding);
770-
else
771-
this._contentLength = chunk.length;
772-
}
773780
write_(this, chunk, encoding, null, true);
774781
} else if (this.finished) {
775782
if (typeof callback === 'function') {

‎test/parallel/test-http-outgoing-destroy.js‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
'use strict';
2+
const common = require('../common');
3+
const assert = require('assert');
4+
5+
const http = require('http');
6+
const OutgoingMessage = http.OutgoingMessage;
7+
8+
{
9+
const msg = new OutgoingMessage();
10+
assert.strictEqual(msg.destroyed, false);
11+
msg.destroy();
12+
assert.strictEqual(msg.destroyed, true);
13+
let callbackCalled = false;
14+
msg.write('asd', common.mustCall((err) => {
15+
assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED');
16+
callbackCalled = true;
17+
}));
18+
msg.on('error', common.mustCall((err) => {
19+
assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED');
20+
assert.strictEqual(callbackCalled, true);
21+
}));
22+
}

‎test/parallel/test-http-outgoing-finish.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
// USE OR OTHER DEALINGS IN THE SOFTWARE.
2121

2222
'use strict';
23-
require('../common');
23+
const common = require('../common');
2424
const assert = require('assert');
2525

2626
const http = require('http');
@@ -49,7 +49,7 @@ function write(out) {
4949
let endCb = false;
5050

5151
// First, write until it gets some backpressure
52-
while (out.write(buf)) {}
52+
while (out.write(buf, common.mustCall())) {}
5353

5454
// Now end, and make sure that we don't get the 'finish' event
5555
// before the tick where the cb gets called. We give it until
@@ -65,12 +65,12 @@ function write(out) {
6565
});
6666
});
6767

68-
out.end(buf, function() {
68+
out.end(buf, common.mustCall(function() {
6969
endCb = true;
7070
console.error(`${name} endCb`);
7171
process.nextTick(function() {
7272
assert(finishEvent, `${name} got endCb event before finishEvent!`);
7373
console.log(`ok - ${name} endCb`);
7474
});
75-
});
75+
}));
7676
}

‎test/parallel/test-http-outgoing-proto.js‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,16 +74,14 @@ assert.throws(() => {
7474
);
7575
}
7676

77-
assert(OutgoingMessage.prototype.write.call({ _header: 'test' }));
78-
7977
assert.throws(() => {
8078
const outgoingMessage = new OutgoingMessage();
8179
outgoingMessage.write.call({ _header: 'test', _hasBody: 'test' });
8280
}, {
8381
code: 'ERR_INVALID_ARG_TYPE',
8482
name: 'TypeError',
85-
message: 'The first argument must be of type string or an instance of ' +
86-
'Buffer. Received undefined'
83+
message: 'The "chunk" argument must be of type string or an instance of ' +
84+
'Buffer or Uint8Array. Received undefined'
8785
});
8886

8987
assert.throws(() => {
@@ -92,8 +90,16 @@ assert.throws(() => {
9290
}, {
9391
code: 'ERR_INVALID_ARG_TYPE',
9492
name: 'TypeError',
95-
message: 'The first argument must be of type string or an instance of ' +
96-
'Buffer. Received type number (1)'
93+
message: 'The "chunk" argument must be of type string or an instance of ' +
94+
'Buffer or Uint8Array. Received type number (1)'
95+
});
96+
97+
assert.throws(() => {
98+
const outgoingMessage = new OutgoingMessage();
99+
outgoingMessage.write.call({ _header: 'test', _hasBody: 'test' }, null);
100+
}, {
101+
code: 'ERR_STREAM_NULL_VALUES',
102+
name: 'TypeError'
97103
});
98104

99105
// addTrailers()

‎test/parallel/test-http-res-write-after-end.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ const server = http.Server(common.mustCall(function(req, res) {
3434
res.end();
3535

3636
const r = res.write('This should raise an error.');
37-
// Write after end should return true
38-
assert.strictEqual(r, true);
37+
// Write after end should return false
38+
assert.strictEqual(r, false);
3939
}));
4040

4141
server.listen(0, function() {

0 commit comments

Comments
 (0)