From 1047db40277adbf7930a24c664f5edfad283f759 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 29 Nov 2022 11:42:35 +0100 Subject: [PATCH 01/10] http: Make OutgoingMessage more streamlike Implement missing getters error & closed. Add support for proper "writable" check through isWritable helper. We cannot fix the OutgoingMessage.writable propery as that will break the ecosystem. --- lib/_http_outgoing.js | 24 +++++++++++++++++++++++- lib/internal/streams/utils.js | 3 +++ lib/stream.js | 1 + 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index a5b3863abaeebb..fa6faf3ea634d5 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -45,6 +45,7 @@ const assert = require('internal/assert'); const EE = require('events'); const Stream = require('stream'); const internalUtil = require('internal/util'); +const { kIsWritable } = require('internal/streams/utils'); const { kOutHeaders, utcDate, kNeedDrain } = require('internal/http'); const { Buffer } = require('buffer'); const { @@ -88,6 +89,7 @@ const kCorked = Symbol('corked'); const kUniqueHeaders = Symbol('kUniqueHeaders'); const kBytesWritten = Symbol('kBytesWritten'); const kEndCalled = Symbol('kEndCalled'); +const kErrored = Symbol('errored'); const nop = () => {}; @@ -146,11 +148,29 @@ function OutgoingMessage() { this._keepAliveTimeout = 0; - this._onPendingData = nop; + this[kErrored] = null; } ObjectSetPrototypeOf(OutgoingMessage.prototype, Stream.prototype); ObjectSetPrototypeOf(OutgoingMessage, Stream); +ObjectDefineProperty(OutgoingMessage.prototype, kIsWritable, { + get() { + return !this.destroyed && !this[kErrored] && !this.finished; + } +}); + +ObjectDefineProperty(OutgoingMessage.prototype, 'errored', { + get() { + return this[kErrored]; + } +}); + +ObjectDefineProperty(OutgoingMessage.prototype, 'closed', { + get() { + return this._closed; + } +}); + ObjectDefineProperty(OutgoingMessage.prototype, 'writableFinished', { __proto__: null, get() { @@ -320,6 +340,8 @@ OutgoingMessage.prototype.destroy = function destroy(error) { } this.destroyed = true; + this[kErrored] = error; + if (this.socket) { this.socket.destroy(error); } else { diff --git a/lib/internal/streams/utils.js b/lib/internal/streams/utils.js index 4d4f00ab456fa7..c8a86f2dd2cfed 100644 --- a/lib/internal/streams/utils.js +++ b/lib/internal/streams/utils.js @@ -9,6 +9,7 @@ const { const kDestroyed = Symbol('kDestroyed'); const kIsErrored = Symbol('kIsErrored'); const kIsReadable = Symbol('kIsReadable'); +const kIsWritable = Symbol('kIsWritable'); const kIsDisturbed = Symbol('kIsDisturbed'); function isReadableNodeStream(obj, strict = false) { @@ -126,6 +127,7 @@ function isReadable(stream) { } function isWritable(stream) { + if (stream && stream[kIsWritable] != null) return stream[kIsWritable]; if (typeof stream?.writable !== 'boolean') return null; if (isDestroyed(stream)) return false; return isWritableNodeStream(stream) && @@ -262,6 +264,7 @@ function isErrored(stream) { } module.exports = { + kIsWritable, kDestroyed, isDisturbed, kIsDisturbed, diff --git a/lib/stream.js b/lib/stream.js index e8a38120cb2ceb..57fa47bf94d89b 100644 --- a/lib/stream.js +++ b/lib/stream.js @@ -54,6 +54,7 @@ const Stream = module.exports = require('internal/streams/legacy').Stream; Stream.isDisturbed = utils.isDisturbed; Stream.isErrored = utils.isErrored; Stream.isReadable = utils.isReadable; +Stream.isWritable = utils.isWritable; Stream.Readable = require('internal/streams/readable'); for (const key of ObjectKeys(streamReturningOperators)) { const op = streamReturningOperators[key]; From 7813900cec1dd90d8edf35ac6bdbf710df08c9f0 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 29 Nov 2022 13:13:53 +0100 Subject: [PATCH 02/10] fixup --- lib/_http_outgoing.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index fa6faf3ea634d5..8b97a4ff02475c 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -148,6 +148,8 @@ function OutgoingMessage() { this._keepAliveTimeout = 0; + this._onPendingData = nop; + this[kErrored] = null; } ObjectSetPrototypeOf(OutgoingMessage.prototype, Stream.prototype); From 8d247a548df8a72dd80d94a80e52ef516e789226 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 29 Nov 2022 13:14:35 +0100 Subject: [PATCH 03/10] fixup --- lib/_http_outgoing.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 8b97a4ff02475c..e5cb82ae3f3613 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -156,18 +156,21 @@ ObjectSetPrototypeOf(OutgoingMessage.prototype, Stream.prototype); ObjectSetPrototypeOf(OutgoingMessage, Stream); ObjectDefineProperty(OutgoingMessage.prototype, kIsWritable, { + __proto__: null, get() { return !this.destroyed && !this[kErrored] && !this.finished; } }); ObjectDefineProperty(OutgoingMessage.prototype, 'errored', { + __proto__: null, get() { return this[kErrored]; } }); ObjectDefineProperty(OutgoingMessage.prototype, 'closed', { + __proto__: null, get() { return this._closed; } From e0a4a310daf952f65b2b25fd06eebbbd315297a9 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 29 Nov 2022 16:04:22 +0100 Subject: [PATCH 04/10] fixuP --- lib/_http_outgoing.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index e5cb82ae3f3613..14738638c2c913 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -45,7 +45,6 @@ const assert = require('internal/assert'); const EE = require('events'); const Stream = require('stream'); const internalUtil = require('internal/util'); -const { kIsWritable } = require('internal/streams/utils'); const { kOutHeaders, utcDate, kNeedDrain } = require('internal/http'); const { Buffer } = require('buffer'); const { @@ -155,13 +154,6 @@ function OutgoingMessage() { ObjectSetPrototypeOf(OutgoingMessage.prototype, Stream.prototype); ObjectSetPrototypeOf(OutgoingMessage, Stream); -ObjectDefineProperty(OutgoingMessage.prototype, kIsWritable, { - __proto__: null, - get() { - return !this.destroyed && !this[kErrored] && !this.finished; - } -}); - ObjectDefineProperty(OutgoingMessage.prototype, 'errored', { __proto__: null, get() { From c9a76bac3c84fe7f71ba7cc2530ade407600681e Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 29 Nov 2022 16:05:28 +0100 Subject: [PATCH 05/10] fixuP --- lib/internal/streams/utils.js | 3 --- lib/stream.js | 1 - 2 files changed, 4 deletions(-) diff --git a/lib/internal/streams/utils.js b/lib/internal/streams/utils.js index c8a86f2dd2cfed..4d4f00ab456fa7 100644 --- a/lib/internal/streams/utils.js +++ b/lib/internal/streams/utils.js @@ -9,7 +9,6 @@ const { const kDestroyed = Symbol('kDestroyed'); const kIsErrored = Symbol('kIsErrored'); const kIsReadable = Symbol('kIsReadable'); -const kIsWritable = Symbol('kIsWritable'); const kIsDisturbed = Symbol('kIsDisturbed'); function isReadableNodeStream(obj, strict = false) { @@ -127,7 +126,6 @@ function isReadable(stream) { } function isWritable(stream) { - if (stream && stream[kIsWritable] != null) return stream[kIsWritable]; if (typeof stream?.writable !== 'boolean') return null; if (isDestroyed(stream)) return false; return isWritableNodeStream(stream) && @@ -264,7 +262,6 @@ function isErrored(stream) { } module.exports = { - kIsWritable, kDestroyed, isDisturbed, kIsDisturbed, diff --git a/lib/stream.js b/lib/stream.js index 57fa47bf94d89b..e8a38120cb2ceb 100644 --- a/lib/stream.js +++ b/lib/stream.js @@ -54,7 +54,6 @@ const Stream = module.exports = require('internal/streams/legacy').Stream; Stream.isDisturbed = utils.isDisturbed; Stream.isErrored = utils.isErrored; Stream.isReadable = utils.isReadable; -Stream.isWritable = utils.isWritable; Stream.Readable = require('internal/streams/readable'); for (const key of ObjectKeys(streamReturningOperators)) { const op = streamReturningOperators[key]; From 2a412af52a034f6dc1004d4b21219cc671798552 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 29 Nov 2022 16:22:46 +0100 Subject: [PATCH 06/10] fixup: tests --- .../parallel/test-http-client-incomingmessage-destroy.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-http-client-incomingmessage-destroy.js b/test/parallel/test-http-client-incomingmessage-destroy.js index a0823d37786365..40b1672e870424 100644 --- a/test/parallel/test-http-client-incomingmessage-destroy.js +++ b/test/parallel/test-http-client-incomingmessage-destroy.js @@ -20,6 +20,13 @@ server.listen(0, () => { get({ port: server.address().port }, common.mustCall((res) => { - res.destroy(new Error('Destroy test')); + const err = new Error('Destroy test') + assert.strictEqual(res.errored, null); + res.destroy(err); + assert.strictEqual(res.closed, false); + assert.strictEqual(res.errored, err); + res.on('close', () => { + assert.strictEqual(res.closed, true); + }) })); }); From 3b3fbb5d460df3750c4c1c063bfc7d33df563318 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 29 Nov 2022 16:25:08 +0100 Subject: [PATCH 07/10] fiuxp --- test/parallel/test-http-outgoing-destroyed.js | 73 ++++++++++++++----- 1 file changed, 53 insertions(+), 20 deletions(-) diff --git a/test/parallel/test-http-outgoing-destroyed.js b/test/parallel/test-http-outgoing-destroyed.js index e0199a1cd5ef23..4faa60b30be962 100644 --- a/test/parallel/test-http-outgoing-destroyed.js +++ b/test/parallel/test-http-outgoing-destroyed.js @@ -1,24 +1,57 @@ 'use strict'; const common = require('../common'); const http = require('http'); +const assert = require('assert'); -const server = http.createServer(common.mustCall((req, res) => { - req.pipe(res); - res.on('error', common.mustNotCall()); - res.on('close', common.mustCall(() => { - res.end('asd'); - process.nextTick(() => { - server.close(); - }); - })); -})).listen(0, () => { - http - .request({ - port: server.address().port, - method: 'PUT' - }) - .on('response', (res) => { - res.destroy(); - }) - .write('asd'); -}); +{ + const server = http.createServer(common.mustCall((req, res) => { + assert.strictEqual(res.closed, false); + req.pipe(res); + res.on('error', common.mustNotCall()); + res.on('close', common.mustCall(() => { + assert.strictEqual(res.closed, true); + res.end('asd'); + process.nextTick(() => { + server.close(); + }); + })); + })).listen(0, () => { + http + .request({ + port: server.address().port, + method: 'PUT' + }) + .on('response', (res) => { + res.destroy(); + }) + .write('asd'); + }); +} + +{ + const server = http.createServer(common.mustCall((req, res) => { + assert.strictEqual(res.closed, true); + req.pipe(res); + res.on('error', common.mustNotCall()); + res.on('close', common.mustCall(() => { + assert.strictEqual(res.closed, true); + process.nextTick(() => { + server.close(); + }); + })); + const err = new Error('Destroy test'); + res.destroy(err); + assert.strictEqual(res.errored, err); + })).listen(0, () => { + http + .request({ + port: server.address().port, + method: 'PUT' + }) + .on('response', (res) => { + res.destroy(); + }) + .write('asd'); + }); + +} From 25d952b1a43698ad0eb507739be32e8b5861b9b1 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 29 Nov 2022 16:25:37 +0100 Subject: [PATCH 08/10] fiuxp --- test/parallel/test-http-outgoing-destroyed.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/parallel/test-http-outgoing-destroyed.js b/test/parallel/test-http-outgoing-destroyed.js index 4faa60b30be962..ba3c81b118ed13 100644 --- a/test/parallel/test-http-outgoing-destroyed.js +++ b/test/parallel/test-http-outgoing-destroyed.js @@ -48,9 +48,7 @@ const assert = require('assert'); port: server.address().port, method: 'PUT' }) - .on('response', (res) => { - res.destroy(); - }) + .on('error', common.mustCall()) .write('asd'); }); From 305ade03b34c4a5942b9f56da5dfd8a6b8199bde Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 30 Nov 2022 11:44:14 +0100 Subject: [PATCH 09/10] Apply suggestions from code review Co-authored-by: Antoine du Hamel --- lib/_http_outgoing.js | 4 ++-- test/parallel/test-http-client-incomingmessage-destroy.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 14738638c2c913..8c80eabaec9e74 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -158,14 +158,14 @@ ObjectDefineProperty(OutgoingMessage.prototype, 'errored', { __proto__: null, get() { return this[kErrored]; - } + }, }); ObjectDefineProperty(OutgoingMessage.prototype, 'closed', { __proto__: null, get() { return this._closed; - } + }, }); ObjectDefineProperty(OutgoingMessage.prototype, 'writableFinished', { diff --git a/test/parallel/test-http-client-incomingmessage-destroy.js b/test/parallel/test-http-client-incomingmessage-destroy.js index 40b1672e870424..64b95dc2ff273f 100644 --- a/test/parallel/test-http-client-incomingmessage-destroy.js +++ b/test/parallel/test-http-client-incomingmessage-destroy.js @@ -20,13 +20,13 @@ server.listen(0, () => { get({ port: server.address().port }, common.mustCall((res) => { - const err = new Error('Destroy test') + const err = new Error('Destroy test'); assert.strictEqual(res.errored, null); res.destroy(err); assert.strictEqual(res.closed, false); assert.strictEqual(res.errored, err); res.on('close', () => { assert.strictEqual(res.closed, true); - }) + }); })); }); From fd82016ea05f147d0c5bc722a6ced3d9f748d0cb Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 30 Nov 2022 11:46:09 +0100 Subject: [PATCH 10/10] fixup --- test/parallel/test-http-outgoing-destroyed.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-http-outgoing-destroyed.js b/test/parallel/test-http-outgoing-destroyed.js index ba3c81b118ed13..2dd3d76fde7d40 100644 --- a/test/parallel/test-http-outgoing-destroyed.js +++ b/test/parallel/test-http-outgoing-destroyed.js @@ -30,7 +30,7 @@ const assert = require('assert'); { const server = http.createServer(common.mustCall((req, res) => { - assert.strictEqual(res.closed, true); + assert.strictEqual(res.closed, false); req.pipe(res); res.on('error', common.mustNotCall()); res.on('close', common.mustCall(() => {