process.stdout is not detected as writable when redirecting output to a file

[catdad]

Sample code:

var isStream = require('is-stream');
console.log('is writable:', isStream.writable(process.stdout));

I have so far only tested this on Windows, here's the output of that test:

C:\Users\Kiril\Desktop\repro>node -v
v4.4.4

C:\Users\Kiril\Desktop\repro>type repro.js
var isStream = require('is-stream');
console.log('is writable:', isStream.writable(process.stdout));

C:\Users\Kiril\Desktop\repro>node repro.js
is writable: true

C:\Users\Kiril\Desktop\repro>node repro.js > file.log

C:\Users\Kiril\Desktop\repro>type file.log
is writable: false

C:\Users\Kiril\Desktop\repro>

When running from the command line, everything is fine and it reports as true. However, when I redirect the same command line to a file, it reports as false. When I checked, both _write and _writableState are undefined. But, of course, you can see that writing to stdout still works and redirects to the file, since console.log works just fine.

[catdad]

Same thing happens on Ubuntu:

kiril@ubuntu:~/Downloads$ node -v
v4.2.6
kiril@ubuntu:~/Downloads$ cat repro.js 
var isStream = require('is-stream');
console.log('is writable:', isStream.writable(process.stdout));
kiril@ubuntu:~/Downloads$ node repro.js 
is writable: true
kiril@ubuntu:~/Downloads$ node repro.js > file.log
kiril@ubuntu:~/Downloads$ cat file.log 
is writable: false
kiril@ubuntu:~/Downloads$ 

[sindresorhus]

Hmm, weird, it changes to a different kind of stream when piped:

var isStream = require('is-stream');
console.log(process.stdout);
console.log('is writable:', isStream.writable(process.stdout));

SyncWriteStream {
  domain: null,
  _events: {},
  _eventsCount: 0,
  _maxListeners: undefined,
  fd: 1,
  writable: true,
  readable: false,
  autoClose: false,
  _type: 'fs',
  _isStdio: true,
  destroySoon: [Function],
  destroy: [Function] }
is writable: false

SyncWriteStream seems to be a temporary hack that never got fixed. It was added back in 2011. The weird part is that it inherits directly from Stream, not WritableStream, hence, not a proper write stream.

A temporary workaround could be to do a process.stdout instanceof SyncWriteStream check and allow that as a write stream too, but not sure it make sense, since it's not a proper write stream.

@silverwind @jasnell Do you think it makes sense to change it to inherit from WritableStream instead?

[jasnell]

To be honest, I'm not sure. @mcollina ... what do you think?

[mcollina]

So, this is extremely complex, and a duplicate of pinojs/pino#85 pinojs/pino#86.
It's something we should discuss in core, and it is likely to be semver-major, so nothing that we can get fixed soon. I do not think we should keep SyncWriteStream  there.

From node.js docs, a stream is writable  if stream.writable is true. SyncWriteStream follow this API. Now, I know this is far from perfect, I am just describing what is the current state.

cc @mafintosh

[catdad]

I had considered doing an MR to special case stdout and stderr, but I am not actually sure I like that. Plus, that leaves the question of other SyncWriteStreams.

[mcollina]

@catdad SyncWriteStream is internal to Node, and only used in one place: https://github.com/nodejs/node/blob/dc7277909b0d842ea3f559628d6d7c7887bfd05b/lib/internal/process/stdio.js#L149.

[jasnell]

Yeah, in fact we've started the process of moving SyncWriteStream to the hidden internal js with an eye towards deprecating the public facing class soon.

[catdad]

@mcollina, that's unfortunately only kind of true. It's intended to be internal, but it is definitely public, I can definitely create one, and it definitely works as I would expect it to work. So at least to me, that seems like something I have to consider as possible for a stream library to handle.

[jasnell]

See: https://github.com/nodejs/node/blob/dc7277909b0d842ea3f559628d6d7c7887bfd05b/lib/fs.js#L2145

[catdad]

@mcollina @jasnell Since this is sort of coming up now, I and many of the people I work with consider the Node documentation to be sparse and often missing information, so we regularly read the Node source code in order to work with various of the deeper APIs. It's very commonplace in my dev community to read the source code of open-source projects we use.

[jasnell]

Understood. In the case of SyncWriteStream, it was never documented because it was always intended to be internal and unsupported. The comments around the class even explicitly warned people away from using it. The "docs only" deprecation is a first step. In Node.js v8 that can be bumped up to a runtime deprecation (printed warning), and completely removed as early as Node.js v9. We'll see tho as those decisions aren't yet final.

[mcollina]

@catdad:

that's unfortunately only kind of true. It's intended to be internal, but it is definitely public, I can definitely create one, and it definitely works as I would expect it to work. So at least to me, that seems like something I have to consider as possible for a stream library to handle.

In my opinion, the public API is the "reference", meaning that if it is not there you are relying on a behavior that a) is not covered by Node.js tests and b) can change at any time, without any notice.
It is basically not safe to be used outside of core.

anyway, SyncWriteStream technically isn't a Writable, because it does not have any of the Writable behavior. SyncWriteStream is a Stream v1. It is not 'safe' to use as a Writable instance. IMHO the correct behavior is correct.

That thing 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.

[catdad]

@mcollina

IMHO the correct behavior is correct.

😉

This is the original bug that led me here: catdad/grandma#96

In this case, when writing to terminal, all is well. process.stdout is a real writable stream. When redirecting it to a file, it becomes not a writable stream and things don't work (because I validate all my inputs).

I would argue that SyncWriteStream, while inheriting from Stream and not technically having all the Writable things it needs, is a stream, and I can write to it, and therefore is a "writable stream". Further, as this is the current Node implementation... whereby it takes something that is documented as a writable stream, and makes it something that is technically a stream and technically writable but technically not a real Writable, then it de facto should be considered a "writable stream" even though it is not a Writable stream.

[mcollina]

@catdad

IMHO the correct behavior is correct.
😉

This is the original bug that led me here: catdad/grandma#96

LOL, I meant the current behavior is correct.

I would argue that SyncWriteStream, while inheriting from Stream and not technically having all the Writable things it needs, is a stream, and I can write to it, and therefore is a "writable stream". Further, as this is the current Node implementation... whereby it takes something that is documented as a writable stream, and makes it something that is technically a stream and technically writable but technically not a real Writable, then it de facto should be considered a "writable stream" even though it is not a Writable stream.

The bug is in core. It is documented as being a Writable , but it is not a Writable in a specific case. isStream.writable() explicitly check if the stream is a Writable, not if it is writable. If you just need a write() method, you should probably check just that.

[sindresorhus]

Closing as this was fixed in Node.js core: nodejs/node#8828