fs: refactor close to use destroy

Refactor close() to use destroy() and not vice versa in ReadStream.
Avoid races between WriteStream.close and WriteStream.write, by aliasing
close to end().

Fixes: #2006
PR-URL: #15407
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

Author: mcollina

lib/fs.js

@@ -2090,44 +2090,38 @@ ReadStream.prototype._read = function(n) {
   }
 };
 
-
 ReadStream.prototype._destroy = function(err, cb) {
-  this.close(function(err2) {
-    cb(err || err2);
-  });
-};
-
-
-ReadStream.prototype.close = function(cb) {
-  if (cb)
-    this.once('close', cb);
-
   if (this.closed || typeof this.fd !== 'number') {
     if (typeof this.fd !== 'number') {
-      this.once('open', closeOnOpen);
+      this.once('open', closeFsStream.bind(null, this, cb, err));
       return;
     }
-    return process.nextTick(() => this.emit('close'));
+
+    return process.nextTick(() => {
+      cb(err);
+      this.emit('close');
+    });
   }
 
   this.closed = true;
 
-  fs.close(this.fd, (er) => {
-    if (er)
-      this.emit('error', er);
-    else
-      this.emit('close');
-  });
-
+  closeFsStream(this, cb);
   this.fd = null;
 };
 
-// needed because as it will be called with arguments
-// that does not match this.close() signature
-function closeOnOpen(fd) {
-  this.close();
+function closeFsStream(stream, cb, err) {
+  fs.close(stream.fd, (er) => {
+    er = er || err;
+    cb(er);
+    if (!er)
+      stream.emit('close');
+  });
 }
 
+ReadStream.prototype.close = function(cb) {
+  this.destroy(null, cb);
+};
+
 fs.createWriteStream = function(path, options) {
   return new WriteStream(path, options);
 };
@@ -2179,7 +2173,7 @@ function WriteStream(path, options) {
   // dispose on finish.
   this.once('finish', function() {
     if (this.autoClose) {
-      this.close();
+      this.destroy();
     }
   });
 }
@@ -2276,7 +2270,21 @@ WriteStream.prototype._writev = function(data, cb) {
 
 
 WriteStream.prototype._destroy = ReadStream.prototype._destroy;
-WriteStream.prototype.close = ReadStream.prototype.close;
+WriteStream.prototype.close = function(cb) {
+  if (this._writableState.ending) {
+    this.on('close', cb);
+    return;
+  }
+
+  if (this._writableState.ended) {
+    process.nextTick(cb);
+    return;
+  }
+
+  // we use end() instead of destroy() because of
+  // https://github.com/nodejs/node/issues/2006
+  this.end(cb);
+};
 
 // There is no shutdown() for files.
 WriteStream.prototype.destroySoon = WriteStream.prototype.end;

test/parallel/test-fs-read-stream-double-close.js

@@ -3,7 +3,17 @@
 const common = require('../common');
 const fs = require('fs');
 
-const s = fs.createReadStream(__filename);
+{
+  const s = fs.createReadStream(__filename);
 
-s.close(common.mustCall());
-s.close(common.mustCall());
+  s.close(common.mustCall());
+  s.close(common.mustCall());
+}
+
+{
+  const s = fs.createReadStream(__filename);
+
+  // this is a private API, but it is worth esting. close calls this
+  s.destroy(null, common.mustCall());
+  s.destroy(null, common.mustCall());
+}