Skip to content

Commit acf56be

Browse files
committedJan 8, 2018
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 PR-URL: #18002 Fixes: #17951 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent 46f783d commit acf56be

4 files changed

+77
-27
lines changed
 

‎lib/fs.js

+19-19
Original file line numberDiff line numberDiff line change
@@ -2348,6 +2348,7 @@ function ReadStream(path, options) {
23482348
this.autoClose = options.autoClose === undefined ? true : options.autoClose;
23492349
this.pos = undefined;
23502350
this.bytesRead = 0;
2351+
this.closed = false;
23512352

23522353
if (this.start !== undefined) {
23532354
if (typeof this.start !== 'number') {
@@ -2461,20 +2462,12 @@ ReadStream.prototype._read = function(n) {
24612462
};
24622463

24632464
ReadStream.prototype._destroy = function(err, cb) {
2464-
if (this.closed || typeof this.fd !== 'number') {
2465-
if (typeof this.fd !== 'number') {
2466-
this.once('open', closeFsStream.bind(null, this, cb, err));
2467-
return;
2468-
}
2469-
2470-
return process.nextTick(() => {
2471-
cb(err);
2472-
this.emit('close');
2473-
});
2465+
const isOpen = typeof this.fd !== 'number';
2466+
if (isOpen) {
2467+
this.once('open', closeFsStream.bind(null, this, cb, err));
2468+
return;
24742469
}
24752470

2476-
this.closed = true;
2477-
24782471
closeFsStream(this, cb);
24792472
this.fd = null;
24802473
};
@@ -2483,6 +2476,7 @@ function closeFsStream(stream, cb, err) {
24832476
fs.close(stream.fd, (er) => {
24842477
er = er || err;
24852478
cb(er);
2479+
stream.closed = true;
24862480
if (!er)
24872481
stream.emit('close');
24882482
});
@@ -2515,6 +2509,7 @@ function WriteStream(path, options) {
25152509
this.autoClose = options.autoClose === undefined ? true : !!options.autoClose;
25162510
this.pos = undefined;
25172511
this.bytesWritten = 0;
2512+
this.closed = false;
25182513

25192514
if (this.start !== undefined) {
25202515
if (typeof this.start !== 'number') {
@@ -2645,19 +2640,24 @@ WriteStream.prototype._writev = function(data, cb) {
26452640

26462641
WriteStream.prototype._destroy = ReadStream.prototype._destroy;
26472642
WriteStream.prototype.close = function(cb) {
2648-
if (this._writableState.ending) {
2649-
this.on('close', cb);
2650-
return;
2643+
if (cb) {
2644+
if (this.closed) {
2645+
process.nextTick(cb);
2646+
return;
2647+
} else {
2648+
this.on('close', cb);
2649+
}
26512650
}
26522651

2653-
if (this._writableState.ended) {
2654-
process.nextTick(cb);
2655-
return;
2652+
// If we are not autoClosing, we should call
2653+
// destroy on 'finish'.
2654+
if (!this.autoClose) {
2655+
this.on('finish', this.destroy.bind(this));
26562656
}
26572657

26582658
// we use end() instead of destroy() because of
26592659
// https://github.com/nodejs/node/issues/2006
2660-
this.end(cb);
2660+
this.end();
26612661
};
26622662

26632663
// There is no shutdown() for files.

‎test/parallel/test-fs-write-stream-autoclose-option.js

+10-5
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,9 @@ let stream = fs.createWriteStream(file, { flags: 'w+', autoClose: false });
1010
stream.write('Test1');
1111
stream.end();
1212
stream.on('finish', common.mustCall(function() {
13+
stream.on('close', common.mustNotCall());
1314
process.nextTick(common.mustCall(function() {
14-
assert.strictEqual(stream.closed, undefined);
15+
assert.strictEqual(stream.closed, false);
1516
assert.notStrictEqual(stream.fd, null);
1617
next();
1718
}));
@@ -23,9 +24,12 @@ function next() {
2324
stream.write('Test2');
2425
stream.end();
2526
stream.on('finish', common.mustCall(function() {
26-
assert.strictEqual(stream.closed, true);
27+
assert.strictEqual(stream.closed, false);
2728
assert.strictEqual(stream.fd, null);
28-
process.nextTick(common.mustCall(next2));
29+
stream.on('close', common.mustCall(function() {
30+
assert.strictEqual(stream.closed, true);
31+
process.nextTick(next2);
32+
}));
2933
}));
3034
}
3135

@@ -44,9 +48,10 @@ function next3() {
4448
stream.write('Test3');
4549
stream.end();
4650
stream.on('finish', common.mustCall(function() {
47-
process.nextTick(common.mustCall(function() {
51+
assert.strictEqual(stream.closed, false);
52+
assert.strictEqual(stream.fd, null);
53+
stream.on('close', common.mustCall(function() {
4854
assert.strictEqual(stream.closed, true);
49-
assert.strictEqual(stream.fd, null);
5055
}));
5156
}));
5257
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const fs = require('fs');
5+
const path = require('path');
6+
7+
common.refreshTmpDir();
8+
9+
const s = fs.createWriteStream(path.join(common.tmpDir, 'nocallback'));
10+
11+
s.end('hello world');
12+
s.close();
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,45 @@
11
'use strict';
22

33
const common = require('../common');
4+
const assert = require('assert');
45
const fs = require('fs');
56
const path = require('path');
67

78
common.refreshTmpDir();
89

9-
const s = fs.createWriteStream(path.join(common.tmpDir, 'rw'));
10+
{
11+
const s = fs.createWriteStream(path.join(common.tmpDir, 'rw'));
1012

11-
s.close(common.mustCall());
12-
s.close(common.mustCall());
13+
s.close(common.mustCall());
14+
s.close(common.mustCall());
15+
}
16+
17+
{
18+
const s = fs.createWriteStream(path.join(common.tmpDir, 'rw2'));
19+
20+
let emits = 0;
21+
s.on('close', () => {
22+
emits++;
23+
});
24+
25+
s.close(common.mustCall(() => {
26+
assert.strictEqual(emits, 1);
27+
s.close(common.mustCall(() => {
28+
assert.strictEqual(emits, 1);
29+
}));
30+
process.nextTick(() => {
31+
s.close(common.mustCall(() => {
32+
assert.strictEqual(emits, 1);
33+
}));
34+
});
35+
}));
36+
}
37+
38+
{
39+
const s = fs.createWriteStream(path.join(common.tmpDir, 'rw'), {
40+
autoClose: false
41+
});
42+
43+
s.close(common.mustCall());
44+
s.close(common.mustCall());
45+
}

0 commit comments

Comments
 (0)
Please sign in to comment.