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: nodejs#2006 PR-URL: nodejs#15407 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
lukaszewczak · Sep 23, 2017 · 90f7d9f · 90f7d9f
1 parent 494d53f
commit 90f7d9f
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 29 deletions.
diff --git a/lib/fs.js b/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;

diff --git a/test/parallel/test-fs-read-stream-double-close.js b/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());
+}