fs: make `SyncWriteStream` inherit from `Writable` by addaleax · Pull Request #8830 · nodejs/node

addaleax · 2016-09-28T15:03:31Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

fs, process?

Description of change

Make the internal SyncWriteStream a proper stream.Writable subclass. This allows for quite a bit of simplification, since SyncWriteStream predates the streams2/streams3 implementations.

Fixes: #8828

I’ve tried to be conservative with the changes here, e.g. by keeping the explicit readable boolean property and by keeping the .destroy() behaviour of emitting an explicit close event, although I doubt that either of these are relied upon.

Make the internal `SyncWriteStream` a proper `stream.Writable` subclass. This allows for quite a bit of simplification, since `SyncWriteStream` predates the streams2/streams3 implementations. Fixes: nodejs#8828

jasnell · 2016-09-28T15:10:32Z

+// when stdout/stderr point to files.
+
+if (process.argv[2] === 'child') {
+ console.log(JSON.stringify([process.stdout, process.stderr].map((stdio) => ({


Nit: Perhaps a note here quickly indicating that the console.log is intentional and part of the test? There are plenty of places around in the tests where console.log is used without serving much purpose.

jasnell

LGTM with green CI and citgm.

addaleax · 2016-09-28T15:15:02Z

@jasnell Done!

CI: https://ci.nodejs.org/job/node-test-commit/5355/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/399/

Fishrock123

LGTM if tests pass, maybe also run citgm on it.

That being said, I think this is only a step, and we should switch to net.Socket so that people don't reply on things that may not be in Socket, and don;t have to patch for Socket functionality.

Fishrock123 · 2016-09-28T15:43:38Z

Actually, that is due to an understanding of mine that net.Socket inherits from WriteStream. If it does not, we should probably not proceed with this? Checking...

Fishrock123 · 2016-09-28T15:46:16Z

Hmmm, net.Socket inherits from stream.Duplex, does that change anything here?

thefourtheye · 2016-09-28T16:10:08Z

cc @nodejs/streams

addaleax · 2016-09-28T16:21:16Z

Hmmm, net.Socket inherits from stream.Duplex, does that change anything here?

Hmmm. On the one hand, Duplexes are not technically Writables according the prototype chain… on the other hand, we should probably makeinstanceof work properly for Duplexes using Symbol.hasInstance? I guess I’ll open a PR for that in a bit, too.

mcollina · 2016-09-29T07:38:54Z

@Fishrock123

That being said, I think this is only a step, and we should switch to net.Socket so that people don't reply on things that may not be in Socket, and don't have to patch for Socket functionality.

I agree. But that might be a semver-major change.

The debug module depends on SyncWriteStream to be there and working as expected.
@thealphanerd is debug in CITGM?

@addaleax can you verify that debug still works with this change?

I'm flagging this as semver-minor, pending CITGM and the assessment on debug.

addaleax · 2016-09-29T08:01:26Z

@thealphanerd is debug in CITGM?

debug doesn’t appear to have a proper test suite? :(

@addaleax can you verify that debug still works with this change?

Ostensibly, yes. I’m not even sure what they are using it for, so I’d probably have to take the time to look into it in more detail.

mcollina · 2016-09-29T08:13:06Z

Ostensibly, yes. I’m not even sure what they are using it for, so I’d probably have to take the time to look into it in more detail.

They lifted some code from core back then. Not sure exactly why, so feel free to have a look, you might also want to send them a PR. You can just run any Express app with DEBUG=* and redirect the output to a file, if that works, we are grand with this.

LGTM

addaleax · 2016-09-29T08:15:16Z

You can just run any Express app with DEBUG=* and redirect the output to a file, if that works, we are grand with this.

I’ll try that later then. :)

jasnell · 2016-09-30T15:18:23Z

@addaleax ... +1 to exploring if we can fixup instanceof with Duplex separately.
@mcollina and @Fishrock123 ... i definitely think switching to net.Socket later on as a semver-major is the right way to go but we'll have to smoke test that fairly extensively I think. In particular, the fact that socket is a Duplex could throw a few people off given that it would need to be half-closed (e.g. no reading, just writing)

addaleax · 2016-09-30T15:20:09Z

+1 to exploring if we can fixup instanceof with Duplex separately.

Just in case you haven’t seen it, that would be #8834. :)

jasnell · 2016-09-30T15:21:09Z

ha! yeah, hadn't made it that far through the notifications ;-)

Fishrock123 · 2016-09-30T20:00:30Z

In particular, the fact that socket is a Duplex could throw a few people off given that it would need to be half-closed (e.g. no reading, just writing)

Again, doubtful. My experience so far is that people become confused when stdio is suddenly not sockets when dealing with files.

mcollina · 2016-10-01T07:42:28Z

In particular, the fact that socket is a Duplex could throw a few people off given that it would need to be half-closed (e.g. no reading, just writing)
Again, doubtful. My experience so far is that people become confused when stdio is suddenly not sockets when dealing with files.

I agree with you @Fishrock123. This PR improve things anyway.

addaleax · 2016-10-08T11:22:16Z

I’d like to land this on Monday if there are no objections; we can still talk about making SyncWriteStream a proper Socket later.

Make the internal `SyncWriteStream` a proper `stream.Writable` subclass. This allows for quite a bit of simplification, since `SyncWriteStream` predates the streams2/streams3 implementations. Fixes: nodejs#8828 PR-URL: nodejs#8830 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

addaleax · 2016-10-10T16:22:22Z

Landed in a60f607

Make the internal `SyncWriteStream` a proper `stream.Writable` subclass. This allows for quite a bit of simplification, since `SyncWriteStream` predates the streams2/streams3 implementations. Fixes: #8828 PR-URL: #8830 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Fishrock123 · 2016-10-11T16:50:26Z

@addaleax requires internals... think you could backport?

addaleax · 2016-10-11T16:51:44Z

@Fishrock123 will do :)

Make the internal `SyncWriteStream` a proper `stream.Writable` subclass. This allows for quite a bit of simplification, since `SyncWriteStream` predates the streams2/streams3 implementations. Fixes: nodejs#8828 PR-URL: nodejs#8830 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Make the internal `SyncWriteStream` a proper `stream.Writable` subclass. This allows for quite a bit of simplification, since `SyncWriteStream` predates the streams2/streams3 implementations. Fixes: #8828 PR-URL: #8830 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> (backport info) Refs: #9030

* fs: - `SyncWriteStream` now inherits from `Stream.Writable`. (Anna Henningsen) #8830 - Practically, this means that when stdio is piped to a file, stdout and stderr will still be `Writable` streams. - `fs.existsSync()` has been undeprecated. `fs.exists()` remains deprecated. (Dan Fabulich) #8364 * http: `http.request()` now accepts a `timeout` option. (Rene Weber) #8101 * module: The module loader now maintains its own realpath cache. (Anna Henningsen) #8100 * npm: Upgraded to 3.10.8 (Kat Marchán) #8706 * stream: `Duplex` streams now show proper `instanceof Stream.Writable`. (Anna Henningsen) #8834 * timers: Improved `setTimeout`/`Interval` performance by up to 22%. (Brian White) #8661 PR-URL: #9034

* fs: - `SyncWriteStream` now inherits from `Stream.Writable`. (Anna Henningsen) nodejs/node#8830 - Practically, this means that when stdio is piped to a file, stdout and stderr will still be `Writable` streams. - `fs.existsSync()` has been undeprecated. `fs.exists()` remains deprecated. (Dan Fabulich) nodejs/node#8364 * http: `http.request()` now accepts a `timeout` option. (Rene Weber) nodejs/node#8101 * module: The module loader now maintains its own realpath cache. (Anna Henningsen) nodejs/node#8100 * npm: Upgraded to 3.10.8 (Kat Marchan) nodejs/node#8706 * stream: `Duplex` streams now show proper `instanceof Stream.Writable`. (Anna Henningsen) nodejs/node#8834 * timers: Improved `setTimeout`/`Interval` performance by up to 22%. (Brian White) nodejs/node#8661 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

fs: make SyncWriteStream inherit from Writable
e1ef536
Make the internal `SyncWriteStream` a proper `stream.Writable` subclass. This allows for quite a bit of simplification, since `SyncWriteStream` predates the streams2/streams3 implementations. Fixes: nodejs#8828

addaleax added fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem. process Issues and PRs related to the process subsystem. labels Sep 28, 2016

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Sep 28, 2016

addaleax mentioned this pull request Sep 28, 2016
fs: make process.stdout and stderr descend from Writable. #8828
Closed

jasnell reviewed Sep 28, 2016
View reviewed changes

jasnell approved these changes Sep 28, 2016
View reviewed changes

[squash] fix nit
ecfd736

Fishrock123 approved these changes Sep 28, 2016
View reviewed changes

addaleax mentioned this pull request Sep 28, 2016
stream: proper instanceof for Writables #8834
Closed
4 tasks

mcollina added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 29, 2016

mcollina approved these changes Sep 29, 2016
View reviewed changes

Fishrock123 added this to the v6.8.0 milestone Oct 10, 2016

addaleax closed this Oct 10, 2016

addaleax deleted the fs-sws branch October 10, 2016 16:22

Fishrock123 added dont-land-on-v4.x labels Oct 11, 2016

addaleax mentioned this pull request Oct 11, 2016
v6.x backport: fs: make SyncWriteStream inherit from Writable#9030
Closed

Uh oh!

Uh oh!

Conversation

addaleax commented Sep 28, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnellSep 28, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 28, 2016

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Sep 28, 2016

Uh oh!

Fishrock123 commented Sep 28, 2016

Uh oh!

thefourtheye commented Sep 28, 2016

Uh oh!

addaleax commented Sep 28, 2016

Uh oh!

mcollina commented Sep 29, 2016

Uh oh!

addaleax commented Sep 29, 2016

Uh oh!

mcollina commented Sep 29, 2016

Uh oh!

addaleax commented Sep 29, 2016

Uh oh!

jasnell commented Sep 30, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Sep 30, 2016

Uh oh!

jasnell commented Sep 30, 2016

Uh oh!

Fishrock123 commented Sep 30, 2016

Uh oh!

mcollina commented Oct 1, 2016

Uh oh!

addaleax commented Oct 8, 2016

Uh oh!

addaleax commented Oct 10, 2016

Uh oh!

Fishrock123 commented Oct 11, 2016

Uh oh!

addaleax commented Oct 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

addaleax commented Sep 28, 2016•
edited
Loading

jasnell commented Sep 30, 2016•
edited
Loading