From 7eed9d6bcc21778b8315a6c3e85eba1ea71dbd25 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Fri, 14 Feb 2020 13:32:45 +0100
Subject: [PATCH] fs: fix WriteStream autoClose order

WriteStream autoClose was implemented by manually
calling .destroy() instead of using autoDestroy
and callback. This caused some invariants related
to order of events to be broken.

Fixes: https://github.com/nodejs/node/issues/31776

Backport-PR-URL: https://github.com/nodejs/node/pull/32163
PR-URL: #31790
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
---
 lib/internal/fs/streams.js                    | 14 +++++------
 test/parallel/test-file-write-stream.js       | 23 ++++++-------------
 .../parallel/test-fs-read-stream-autoClose.js | 16 +++++++++++++
 .../test-fs-write-stream-autoclose-option.js  |  4 ++--
 4 files changed, 31 insertions(+), 26 deletions(-)
 create mode 100644 test/parallel/test-fs-read-stream-autoClose.js

diff --git a/lib/internal/fs/streams.js b/lib/internal/fs/streams.js
index 14d6ea391d00c2..5ec74ee0b58c5a 100644
--- a/lib/internal/fs/streams.js
+++ b/lib/internal/fs/streams.js
@@ -300,6 +300,11 @@ function WriteStream(path, options) {
     options.emitClose = false;
   }
 
+  if (options.autoDestroy === undefined) {
+    options.autoDestroy = options.autoClose === undefined ?
+      true : (options.autoClose || false);
+  }
+
   this[kFs] = options.fs || fs;
   if (typeof this[kFs].open !== 'function') {
     throw new ERR_INVALID_ARG_TYPE('options.fs.open', 'function',
@@ -343,7 +348,7 @@ function WriteStream(path, options) {
   this.mode = options.mode === undefined ? 0o666 : options.mode;
 
   this.start = options.start;
-  this.autoClose = options.autoClose === undefined ? true : !!options.autoClose;
+  this.autoClose = options.autoDestroy;
   this.pos = undefined;
   this.bytesWritten = 0;
   this.closed = false;
@@ -371,10 +376,6 @@ WriteStream.prototype._final = function(callback) {
     });
   }
 
-  if (this.autoClose) {
-    this.destroy();
-  }
-
   callback();
 };
 
@@ -425,9 +426,6 @@ WriteStream.prototype._write = function(data, encoding, cb) {
     }
 
     if (er) {
-      if (this.autoClose) {
-        this.destroy();
-      }
       return cb(er);
     }
     this.bytesWritten += bytes;
diff --git a/test/parallel/test-file-write-stream.js b/test/parallel/test-file-write-stream.js
index 05a2d5432b5fb7..00148c8dfc8ba9 100644
--- a/test/parallel/test-file-write-stream.js
+++ b/test/parallel/test-file-write-stream.js
@@ -20,7 +20,7 @@
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 'use strict';
-require('../common');
+const common = require('../common');
 const assert = require('assert');
 
 const path = require('path');
@@ -46,9 +46,9 @@ file
     callbacks.open++;
     assert.strictEqual(typeof fd, 'number');
   })
-  .on('error', function(err) {
-    throw err;
-  })
+  .on('error', common.mustCall((err) => {
+    assert.strictEqual(err.code, 'ERR_STREAM_WRITE_AFTER_END');
+  }))
   .on('drain', function() {
     console.error('drain!', callbacks.drain);
     callbacks.drain++;
@@ -61,21 +61,12 @@ file
     }
   })
   .on('close', function() {
-    console.error('close!');
     assert.strictEqual(file.bytesWritten, EXPECTED.length * 2);
 
     callbacks.close++;
-    assert.throws(
-      () => {
-        console.error('write after end should not be allowed');
-        file.write('should not work anymore');
-      },
-      {
-        code: 'ERR_STREAM_WRITE_AFTER_END',
-        name: 'Error',
-        message: 'write after end'
-      }
-    );
+    file.write('should not work anymore', common.mustCall((err) => {
+      assert.strictEqual(err.code, 'ERR_STREAM_WRITE_AFTER_END');
+    }));
 
     fs.unlinkSync(fn);
   });
diff --git a/test/parallel/test-fs-read-stream-autoClose.js b/test/parallel/test-fs-read-stream-autoClose.js
new file mode 100644
index 00000000000000..e7989ee7911264
--- /dev/null
+++ b/test/parallel/test-fs-read-stream-autoClose.js
@@ -0,0 +1,16 @@
+'use strict';
+
+const common = require('../common');
+const fs = require('fs');
+const path = require('path');
+const assert = require('assert');
+const tmpdir = require('../common/tmpdir');
+const writeFile = path.join(tmpdir.path, 'write-autoClose.txt');
+tmpdir.refresh();
+
+const file = fs.createWriteStream(writeFile, { autoClose: true });
+
+file.on('finish', common.mustCall(() => {
+  assert.strictEqual(file.destroyed, false);
+}));
+file.end('asd');
diff --git a/test/parallel/test-fs-write-stream-autoclose-option.js b/test/parallel/test-fs-write-stream-autoclose-option.js
index e39f4d615ab7e0..8d205fbc69fb39 100644
--- a/test/parallel/test-fs-write-stream-autoclose-option.js
+++ b/test/parallel/test-fs-write-stream-autoclose-option.js
@@ -27,8 +27,8 @@ function next() {
   stream.end();
   stream.on('finish', common.mustCall(function() {
     assert.strictEqual(stream.closed, false);
-    assert.strictEqual(stream.fd, null);
     stream.on('close', common.mustCall(function() {
+      assert.strictEqual(stream.fd, null);
       assert.strictEqual(stream.closed, true);
       process.nextTick(next2);
     }));
@@ -51,8 +51,8 @@ function next3() {
   stream.end();
   stream.on('finish', common.mustCall(function() {
     assert.strictEqual(stream.closed, false);
-    assert.strictEqual(stream.fd, null);
     stream.on('close', common.mustCall(function() {
+      assert.strictEqual(stream.fd, null);
       assert.strictEqual(stream.closed, true);
     }));
   }));