fs: guarantee order of callbacks in ws.close

Refactor WriteStream.prototype.close and WriteStream.prototype._destroy to always call the callback passed to close in order. Protects from calling .close() without a callback. Fixes: #17951 See: #15407
nodejs · mcollina · Jan 5, 2018 · Jan 5, 2018 · 0a4854dbec67e5a40c7a2de3ce7b26fc8ec84b39
commit 0a4854dbec67e5a40c7a2de3ce7b26fc8ec84b39
diff --git a/lib/fs.js b/lib/fs.js
@@ -2348,6 +2348,7 @@ function ReadStream(path, options) {
   this.autoClose = options.autoClose === undefined ? true : options.autoClose;
   this.pos = undefined;
   this.bytesRead = 0;
+  this.closed = false;
 
   if (this.start !== undefined) {
     if (typeof this.start !== 'number') {
@@ -2461,20 +2462,12 @@ ReadStream.prototype._read = function(n) {
 };
 
 ReadStream.prototype._destroy = function(err, cb) {
-  if (this.closed || typeof this.fd !== 'number') {
-    if (typeof this.fd !== 'number') {
-      this.once('open', closeFsStream.bind(null, this, cb, err));
-      return;
-    }
-
-    return process.nextTick(() => {
-      cb(err);
-      this.emit('close');
-    });
+  const isOpen = typeof this.fd !== 'number';
+  if (isOpen) {
+    this.once('open', closeFsStream.bind(null, this, cb, err));
+    return;
   }
 
-  this.closed = true;
-
   closeFsStream(this, cb);
   this.fd = null;
 };
@@ -2483,6 +2476,7 @@ function closeFsStream(stream, cb, err) {
   fs.close(stream.fd, (er) => {
     er = er || err;
     cb(er);
+    stream.closed = true;
     if (!er)
       stream.emit('close');
   });
@@ -2515,6 +2509,7 @@ function WriteStream(path, options) {
   this.autoClose = options.autoClose === undefined ? true : !!options.autoClose;
   this.pos = undefined;
   this.bytesWritten = 0;
+  this.closed = false;
 
   if (this.start !== undefined) {
     if (typeof this.start !== 'number') {
@@ -2645,19 +2640,24 @@ WriteStream.prototype._writev = function(data, cb) {
 
 WriteStream.prototype._destroy = ReadStream.prototype._destroy;
 WriteStream.prototype.close = function(cb) {
-  if (this._writableState.ending) {
-    this.on('close', cb);
-    return;
+  if (cb) {
+    if (this.closed) {
+      process.nextTick(cb);
+      return;
+    } else {
+      this.on('close', cb);
+    }
   }
 
-  if (this._writableState.ended) {
-    process.nextTick(cb);
-    return;
+  // If we are not autoClosing, we should call
+  // destroy on 'finish'.
+  if (!this.autoClose) {
+    this.on('finish', this.destroy.bind(this));
   }
 
   // we use end() instead of destroy() because of
   // https://github.com/nodejs/node/issues/2006
-  this.end(cb);
+  this.end();
 };
 
 // There is no shutdown() for files.

diff --git a/test/parallel/test-fs-write-stream-autoclose-option.js b/test/parallel/test-fs-write-stream-autoclose-option.js
@@ -10,8 +10,9 @@ let stream = fs.createWriteStream(file, { flags: 'w+', autoClose: false });
 stream.write('Test1');
 stream.end();
 stream.on('finish', common.mustCall(function() {
+  stream.on('close', common.mustNotCall());
   process.nextTick(common.mustCall(function() {
-    assert.strictEqual(stream.closed, undefined);
+    assert.strictEqual(stream.closed, false);
     assert.notStrictEqual(stream.fd, null);
     next();
   }));
@@ -23,9 +24,12 @@ function next() {
   stream.write('Test2');
   stream.end();
   stream.on('finish', common.mustCall(function() {
-    assert.strictEqual(stream.closed, true);
+    assert.strictEqual(stream.closed, false);
     assert.strictEqual(stream.fd, null);
-    process.nextTick(common.mustCall(next2));
+    stream.on('close', common.mustCall(function() {
+      assert.strictEqual(stream.closed, true);
+      process.nextTick(next2);
+    }));
   }));
 }
 
@@ -44,9 +48,10 @@ function next3() {
   stream.write('Test3');
   stream.end();
   stream.on('finish', common.mustCall(function() {
-    process.nextTick(common.mustCall(function() {
+    assert.strictEqual(stream.closed, false);
+    assert.strictEqual(stream.fd, null);
+    stream.on('close', common.mustCall(function() {
       assert.strictEqual(stream.closed, true);
-      assert.strictEqual(stream.fd, null);
     }));
   }));
 }
diff --git a/test/parallel/test-fs-write-stream-close-without-callback.js b/test/parallel/test-fs-write-stream-close-without-callback.js
@@ -0,0 +1,12 @@
+'use strict';
+
+const common = require('../common');
+const fs = require('fs');
+const path = require('path');
+
+common.refreshTmpDir();
+
+const s = fs.createWriteStream(path.join(common.tmpDir, 'nocallback'));
+
+s.end('hello world');
+s.close();
diff --git a/test/parallel/test-fs-write-stream-double-close.js b/test/parallel/test-fs-write-stream-double-close.js
@@ -1,12 +1,45 @@
 'use strict';
 
 const common = require('../common');
+const assert = require('assert');
 const fs = require('fs');
 const path = require('path');
 
 common.refreshTmpDir();
 
-const s = fs.createWriteStream(path.join(common.tmpDir, 'rw'));
+{
+  const s = fs.createWriteStream(path.join(common.tmpDir, 'rw'));
 
-s.close(common.mustCall());
-s.close(common.mustCall());
+  s.close(common.mustCall());
+  s.close(common.mustCall());
+}
+
+{
+  const s = fs.createWriteStream(path.join(common.tmpDir, 'rw2'));
+
+  let emits = 0;
+  s.on('close', () => {
+    emits++;
+  });
+
+  s.close(common.mustCall(() => {
+    assert.strictEqual(emits, 1);
+    s.close(common.mustCall(() => {
+      assert.strictEqual(emits, 1);
+    }));
+    process.nextTick(() => {
+      s.close(common.mustCall(() => {
+        assert.strictEqual(emits, 1);
+      }));
+    });
+  }));
+}
+
+{
+  const s = fs.createWriteStream(path.join(common.tmpDir, 'rw'), {
+    autoClose: false
+  });
+
+  s.close(common.mustCall());
+  s.close(common.mustCall());
+}