Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed OutgoingMessage so that it inherits from stream.Writable instead of stream #45834

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions lib/_http_outgoing.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ function isCookieField(s) {
}

function OutgoingMessage() {
Stream.call(this);

// Queue that holds all currently pending data, until the response will be
// assigned to the socket (until it will its turn in the HTTP pipeline).
Expand All @@ -114,8 +113,8 @@ function OutgoingMessage() {
// TCP socket and HTTP Parser and thus handle the backpressure.
this.outputSize = 0;

this.writable = true;
this.destroyed = false;
this.writableState = true;
this.destroyedState = false;

this._last = false;
this.chunkedEncoding = false;
Expand Down Expand Up @@ -148,24 +147,30 @@ function OutgoingMessage() {
this._keepAliveTimeout = 0;

this._onPendingData = nop;

this[kErrored] = null;
Stream.Writable.call(this);
}
ObjectSetPrototypeOf(OutgoingMessage.prototype, Stream.prototype);
ObjectSetPrototypeOf(OutgoingMessage, Stream);

ObjectDefineProperty(OutgoingMessage.prototype, 'errored', {
ObjectSetPrototypeOf(OutgoingMessage.prototype, Stream.Writable.prototype);
ObjectSetPrototypeOf(OutgoingMessage, Stream.Writable);

ObjectDefineProperty(OutgoingMessage.prototype, 'writable', {
__proto__: null,
get() {
return this[kErrored];
return this.writableState;
},
set(val) {
this.writableState = val;
}
});

ObjectDefineProperty(OutgoingMessage.prototype, 'closed', {
ObjectDefineProperty(OutgoingMessage.prototype, 'destroyed', {
__proto__: null,
get() {
return this._closed;
return this.destroyedState;
},
set(val) {
this.destroyedState = val;
}
});

ObjectDefineProperty(OutgoingMessage.prototype, 'writableFinished', {
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/streams/writable.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ const {
getHighWaterMark,
getDefaultHighWaterMark
} = require('internal/streams/state');

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These newline only file changes should be removed (including the one in the test file).

const {
ERR_INVALID_ARG_TYPE,
ERR_METHOD_NOT_IMPLEMENTED,
Expand Down Expand Up @@ -795,6 +796,7 @@ ObjectDefineProperties(Writable.prototype, {
// where the writable side was disabled upon construction.
// Compat. The user might manually disable writable side through
// deprecated setter.

return !!w && w.writable !== false && !w.destroyed && !w.errored &&
!w.ending && !w.ended;
},
Expand Down
13 changes: 13 additions & 0 deletions script.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to remove this file


cd ~/node/test/parallel

mkdir test-stream-writable-dir

for file in ./*.js
do
if [[ "$file" =~ .*"test-stream-writable".* ]]; then
cp $file "./test-stream-writable-dir/$file"
echo "{$file} copied"
fi
done
1 change: 1 addition & 0 deletions test/parallel/test-http-writable-true-after-close.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const server = createServer(common.mustCall((req, res) => {
}));
}).listen(0, () => {
external = get(`http://127.0.0.1:${server.address().port}`);

external.on('error', common.mustCall(() => {
server.close();
internal.close();
Expand Down
46 changes: 46 additions & 0 deletions test/parallel/test-stream-writable-acceptance.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const http = require('http');

const { stream, Writable } = require('stream');
const { once } = require('events');

async function test() {
const o = new http.OutgoingMessage();
assert.strictEqual(o instanceof http.OutgoingMessage, true);
assert.strictEqual(o instanceof Writable, true);

let external;

const server = http.createServer(async (req, nodeStreamResponse) => {
const isOutgoingMessage = nodeStreamResponse instanceof http.OutgoingMessage;
//const isStream = stream.isInstance(nodeStreamResponse);
const isWritable = nodeStreamResponse instanceof Writable;

assert.strictEqual(nodeStreamResponse.writable, true);
assert.strictEqual(isOutgoingMessage, true);
assert.strictEqual(isWritable, true);


const b = typeof nodeStreamResponse?._writableState
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and other lines won't pass the linter. Make sure you run make test locally to make sure all other tests pass in addition to the linting.


assert.strictEqual(b, 'object')

const webStreamResponse = Writable.toWeb(nodeStreamResponse);

setImmediate(common.mustCall(() => {
external.abort();
nodeStreamResponse.end('Hello World\n');
}));
});

server.listen(0, common.mustCall(() => {
external = http.get(`http://127.0.0.1:${server.address().port}`);
external.on('error', common.mustCall(() => {
server.close();
}));
}));
}

test();