Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit e5c290b

Browse filesBrowse files
committed
fs: refactor close to use destroy
Refactor close() to use destroy() and not vice versa in ReadStream. Avoid races between WriteStream.close and WriteStream.write, by aliasing close to end(). Fixes: #2006 PR-URL: #15407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
1 parent f27b5e4 commit e5c290b
Copy full SHA for e5c290b

File tree

Expand file treeCollapse file tree

2 files changed

+47
-29
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+47
-29
lines changed
Open diff view settings
Collapse file

‎lib/fs.js‎

Copy file name to clipboardExpand all lines: lib/fs.js
+34-26Lines changed: 34 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2090,44 +2090,38 @@ ReadStream.prototype._read = function(n) {
20902090
}
20912091
};
20922092

2093-
20942093
ReadStream.prototype._destroy = function(err, cb) {
2095-
this.close(function(err2) {
2096-
cb(err || err2);
2097-
});
2098-
};
2099-
2100-
2101-
ReadStream.prototype.close = function(cb) {
2102-
if (cb)
2103-
this.once('close', cb);
2104-
21052094
if (this.closed || typeof this.fd !== 'number') {
21062095
if (typeof this.fd !== 'number') {
2107-
this.once('open', closeOnOpen);
2096+
this.once('open', closeFsStream.bind(null, this, cb, err));
21082097
return;
21092098
}
2110-
return process.nextTick(() => this.emit('close'));
2099+
2100+
return process.nextTick(() => {
2101+
cb(err);
2102+
this.emit('close');
2103+
});
21112104
}
21122105

21132106
this.closed = true;
21142107

2115-
fs.close(this.fd, (er) => {
2116-
if (er)
2117-
this.emit('error', er);
2118-
else
2119-
this.emit('close');
2120-
});
2121-
2108+
closeFsStream(this, cb);
21222109
this.fd = null;
21232110
};
21242111

2125-
// needed because as it will be called with arguments
2126-
// that does not match this.close() signature
2127-
function closeOnOpen(fd) {
2128-
this.close();
2112+
function closeFsStream(stream, cb, err) {
2113+
fs.close(stream.fd, (er) => {
2114+
er = er || err;
2115+
cb(er);
2116+
if (!er)
2117+
stream.emit('close');
2118+
});
21292119
}
21302120

2121+
ReadStream.prototype.close = function(cb) {
2122+
this.destroy(null, cb);
2123+
};
2124+
21312125
fs.createWriteStream = function(path, options) {
21322126
return new WriteStream(path, options);
21332127
};
@@ -2179,7 +2173,7 @@ function WriteStream(path, options) {
21792173
// dispose on finish.
21802174
this.once('finish', function() {
21812175
if (this.autoClose) {
2182-
this.close();
2176+
this.destroy();
21832177
}
21842178
});
21852179
}
@@ -2276,7 +2270,21 @@ WriteStream.prototype._writev = function(data, cb) {
22762270

22772271

22782272
WriteStream.prototype._destroy = ReadStream.prototype._destroy;
2279-
WriteStream.prototype.close = ReadStream.prototype.close;
2273+
WriteStream.prototype.close = function(cb) {
2274+
if (this._writableState.ending) {
2275+
this.on('close', cb);
2276+
return;
2277+
}
2278+
2279+
if (this._writableState.ended) {
2280+
process.nextTick(cb);
2281+
return;
2282+
}
2283+
2284+
// we use end() instead of destroy() because of
2285+
// https://github.com/nodejs/node/issues/2006
2286+
this.end(cb);
2287+
};
22802288

22812289
// There is no shutdown() for files.
22822290
WriteStream.prototype.destroySoon = WriteStream.prototype.end;
Collapse file

‎test/parallel/test-fs-read-stream-double-close.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-fs-read-stream-double-close.js
+13-3Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,17 @@
33
const common = require('../common');
44
const fs = require('fs');
55

6-
const s = fs.createReadStream(__filename);
6+
{
7+
const s = fs.createReadStream(__filename);
78

8-
s.close(common.mustCall());
9-
s.close(common.mustCall());
9+
s.close(common.mustCall());
10+
s.close(common.mustCall());
11+
}
12+
13+
{
14+
const s = fs.createReadStream(__filename);
15+
16+
// this is a private API, but it is worth esting. close calls this
17+
s.destroy(null, common.mustCall());
18+
s.destroy(null, common.mustCall());
19+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.