fs: make SyncWriteStream inherit from Writable

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>

Author: addaleax

lib/internal/fs.js

@@ -1,7 +1,7 @@
 'use strict';
 
 const Buffer = require('buffer').Buffer;
-const Stream = require('stream').Stream;
+const Writable = require('stream').Writable;
 const fs = require('fs');
 const util = require('util');
 
@@ -14,65 +14,41 @@ exports.assertEncoding = assertEncoding;
 
 // Temporary hack for process.stdout and process.stderr when piped to files.
 function SyncWriteStream(fd, options) {
-  Stream.call(this);
+  Writable.call(this);
 
   options = options || {};
 
   this.fd = fd;
-  this.writable = true;
   this.readable = false;
   this.autoClose = options.autoClose === undefined ? true : options.autoClose;
-}
-
-util.inherits(SyncWriteStream, Stream);
-
-SyncWriteStream.prototype.write = function(data, arg1, arg2) {
-  var encoding, cb;
-
-  // parse arguments
-  if (arg1) {
-    if (typeof arg1 === 'string') {
-      encoding = arg1;
-      cb = arg2;
-    } else if (typeof arg1 === 'function') {
-      cb = arg1;
-    } else {
-      throw new Error('Bad arguments');
-    }
-  }
-  assertEncoding(encoding);
 
-  // Change strings to buffers. SLOW
-  if (typeof data === 'string') {
-    data = Buffer.from(data, encoding);
-  }
+  this.on('end', () => this._destroy());
+}
 
-  fs.writeSync(this.fd, data, 0, data.length);
-
-  if (cb) {
-    process.nextTick(cb);
-  }
+util.inherits(SyncWriteStream, Writable);
 
+SyncWriteStream.prototype._write = function(chunk, encoding, cb) {
+  fs.writeSync(this.fd, chunk, 0, chunk.length);
+  cb();
   return true;
 };
 
+SyncWriteStream.prototype._destroy = function() {
+  if (this.fd === null) // already destroy()ed
+    return;
 
-SyncWriteStream.prototype.end = function(data, arg1, arg2) {
-  if (data) {
-    this.write(data, arg1, arg2);
-  }
-  this.destroy();
-};
-
-
-SyncWriteStream.prototype.destroy = function() {
   if (this.autoClose)
     fs.closeSync(this.fd);
+
   this.fd = null;
-  this.emit('close');
   return true;
 };
 
-SyncWriteStream.prototype.destroySoon = SyncWriteStream.prototype.destroy;
+SyncWriteStream.prototype.destroySoon =
+SyncWriteStream.prototype.destroy = function() {
+  this._destroy();
+  this.emit('close');
+  return true;
+};
 
 exports.SyncWriteStream = SyncWriteStream;

test/parallel/test-fs-syncwritestream.js

@@ -0,0 +1,40 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const spawn = require('child_process').spawn;
+const stream = require('stream');
+const fs = require('fs');
+const path = require('path');
+
+// require('internal/fs').SyncWriteStream is used as a stdio implementation
+// when stdout/stderr point to files.
+
+if (process.argv[2] === 'child') {
+  // Note: Calling console.log() is part of this test as it exercises the
+  // SyncWriteStream#_write() code path.
+  console.log(JSON.stringify([process.stdout, process.stderr].map((stdio) => ({
+    instance: stdio instanceof stream.Writable,
+    readable: stdio.readable,
+    writable: stdio.writable,
+  }))));
+
+  return;
+}
+
+common.refreshTmpDir();
+
+const filename = path.join(common.tmpDir, 'stdout');
+const stdoutFd = fs.openSync(filename, 'w');
+
+const proc = spawn(process.execPath, [__filename, 'child'], {
+  stdio: ['inherit', stdoutFd, stdoutFd ]
+});
+
+proc.on('close', common.mustCall(() => {
+  fs.closeSync(stdoutFd);
+
+  assert.deepStrictEqual(JSON.parse(fs.readFileSync(filename, 'utf8')), [
+    { instance: true, readable: false, writable: true },
+    { instance: true, readable: false, writable: true }
+  ]);
+}));