fs: make process.stdout and stderr descend from Writable.

[mcollina]

• Version: since dec 2011, a936768#diff-9a205ef7ee967ee32efee02e58b3482dR1342
• Platform: all
• Subsystem: fs, stream

process.stdout and process.stderr are not implementing Stream 2 or 3 if they are redirected to a file. They are a SyncWriteStream:

node/lib/internal/process/stdio.js

Line 149 in dc72779

stream = new fs.SyncWriteStream(fd, { autoClose: false });

.
This is were the choice is made.:

node/lib/internal/process/stdio.js

Lines 147 to 151 in dc72779

	case 'FILE':
	const fs = require('internal/fs');
	stream = new fs.SyncWriteStream(fd, { autoClose: false });
	stream._type = 'fs';
	break;

I know too little of the stdio sync/async situation to identify what should be preferred change. Ideally process.stdout and process.stderr should come from the streams implementations, but I might be wrong.
What are the implications of doing so? Why is this needed in the first place?
As an example, can we use net.Socket instead

node/lib/internal/process/stdio.js

Lines 156 to 160 in dc72779

	stream = new net.Socket({
	fd: fd,
	readable: false,
	writable: true
	});

?

Issues related:
sindresorhus/is-stream#5
pinojs/pino#85
pinojs/pino#86

cc @jasnell @Fishrock123 @jsumners @catdad @sindresorhus @nodejs/streams

[addaleax]

Ideally process.stdout and process.stderr should come from the streams implementations, but I might be wrong.

+1. It also looks like the SyncWriteStream implementation could be simplified a lot when built on top of Writable… I’ll try to put a patch together but it seems hard to estimate the semverness of that.

[jasnell]

+1 on doing something here. As far as I understand it, the SyncWriteStream impl that was there had been intended as a kind of temporary half measure but it was never really finished. It's likely to be a semver-major change but hard to say for sure until we get a better implementation.

[Fishrock123]

Hmmm, I was sure there was an older issue for this.

I think it would be ideal to change it to use net.Socket, there are a lot of subtle issues that come into play with properties not existing if someone pipes to/from a file. I doubt that would be a major change, it fixes the need for workarounds.

[mcollina]

@Fishrock123 maybe there is, I found none mentioning SyncWriteStream directly.

[mcollina]

SyncWriteStream is mainly used within the debug module https://github.com/visionmedia/debug/blob/39ecd87bcc145de5ca1cbea1bf4caed02c34d30a/node.js#L164. debug lifts some chunks of code straight from core.

[addaleax]

Suggested PR (that does nothing net.Socket-wise, so its desirability is probably up for debate): #8830

[silverwind]

Is it related that process.stdin isn't a ReadStream when redirected?

$ node -p "Object.getPrototypeOf(process.stdin)" | head -1
ReadStream { setRawMode: [Function] }
$ node -p "Object.getPrototypeOf(process.stdin)" < <(echo) | head -1
Socket {

[Fishrock123]

see: createWritableStdioStream(fd)