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

@jasnell

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

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.

+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.

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.

@Fishrock123

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

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.

fs: make `SyncWriteStream` inherit from `Writable`

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

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 {

see: createWritableStdioStream(fd)

fs: make `SyncWriteStream` inherit from `Writable`

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

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

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

Description

Activity

addaleax commented on Sep 28, 2016

jasnell commented on Sep 28, 2016

Fishrock123 commented on Sep 28, 2016

mcollina commented on Sep 28, 2016

mcollina commented on Sep 28, 2016

addaleax commented on Sep 28, 2016

silverwind commented on Sep 28, 2016

Fishrock123 commented on Sep 28, 2016

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions