Skip to content

Commit

Permalink
fs: fix WriteStream autoClose order
Browse files Browse the repository at this point in the history
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: #31776

Backport-PR-URL: #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>
  • Loading branch information
ronag authored and MylesBorins committed Mar 11, 2020
1 parent 897b1d2 commit 7eed9d6
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 26 deletions.
14 changes: 6 additions & 8 deletions lib/internal/fs/streams.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -371,10 +376,6 @@ WriteStream.prototype._final = function(callback) {
});
}

if (this.autoClose) {
this.destroy();
}

callback();
};

Expand Down Expand Up @@ -425,9 +426,6 @@ WriteStream.prototype._write = function(data, encoding, cb) {
}

if (er) {
if (this.autoClose) {
this.destroy();
}
return cb(er);
}
this.bytesWritten += bytes;
Expand Down
23 changes: 7 additions & 16 deletions test/parallel/test-file-write-stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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++;
Expand All @@ -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);
});
Expand Down
16 changes: 16 additions & 0 deletions test/parallel/test-fs-read-stream-autoClose.js
Original file line number Diff line number Diff line change
@@ -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');
4 changes: 2 additions & 2 deletions test/parallel/test-fs-write-stream-autoclose-option.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}));
Expand All @@ -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);
}));
}));
Expand Down

0 comments on commit 7eed9d6

Please sign in to comment.