From 90f7d9f14d8ee26f93189638c22faef6a83c7fe9 Mon Sep 17 00:00:00 2001
From: Matteo Collina <hello@matteocollina.com>
Date: Thu, 14 Sep 2017 13:46:02 +0200
Subject: [PATCH] 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: https://github.com/nodejs/node/issues/2006
PR-URL: https://github.com/nodejs/node/pull/15407
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 lib/fs.js                                     | 60 +++++++++++--------
 .../test-fs-read-stream-double-close.js       | 16 ++++-
 2 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/lib/fs.js b/lib/fs.js
index fb1001c8c09d6f..24af0e58a3dc99 100644
--- 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
index ca337d4f8a576b..5f36c03bdbf243 100644
--- 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());
+}