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: nodejs#31776 PR-URL: nodejs#31790 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Backport-PR-URL: nodejs#32163
nxtedition · Mar 10, 2020 · c515e98 · c515e98
1 parent 6122620
commit c515e98
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 26 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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);
     }));
   }));