fs: fix WriteStream autoClose order

ronag · ronag · commit c515e989ebe3 · 2020-03-10T13:18:29.000+01:00
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 <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Backport-PR-URL: nodejs#32163
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);
     }));
   }));
