fs: refactor close to use destroy #15407

mcollina · 2017-09-14T11:50:41Z

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

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

fs, stream

jasnell

LGTM

mcollina · 2017-09-14T11:55:26Z

I was not able to write a test for the given issue, but I am fairly certain that this solves the problem.

I am not really able to write a test to reproduce the change of behavior, which should stay fairly the same. I have flagged this as semver-major as I fear it might introduce bugs and regressions.
All our tests are passing anyhow.

A CITGM run might help.

jasnell · 2017-09-14T11:56:22Z

Understood

mcollina · 2017-09-14T11:58:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10075/

mcollina · 2017-09-19T17:25:39Z

I would like this to land in Node 9 if possible.

cc @nodejs/tsc for another approval as it's semver-major.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/991/

BridgeAR · 2017-09-19T19:31:23Z

lib/fs.js

      return;
    }
+    cb(err);


Should the callback really be triggered sync?

Good spot. Updating.

cjihrig · 2017-09-20T00:41:49Z

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

+{
+  const s = fs.createReadStream(__filename);
+
+  // this is a private API, but it is worth


Minor nit: This is a private API, but it is worth testing. close() calls this. fits on one line.

mcollina · 2017-09-20T09:22:05Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/992/
CI: https://ci.nodejs.org/job/node-test-pull-request/10157/
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/993/

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: nodejs#2006

mcollina · 2017-09-20T14:38:54Z

Can somebody that understand what's the status on citgm verify if this is causing issues or not? It seems a lot of red in citgm is transient.

I have rebased against master.

mcollina · 2017-09-20T14:39:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10162/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/994/

mcollina · 2017-09-20T16:43:19Z

cc @refack can you have a look?

BridgeAR · 2017-09-20T16:53:31Z

@mcollina as far as I can read the citgm results by now I would say it is a "green" build 😆

MylesBorins · 2017-09-20T16:56:48Z

@mcollina the citgm results between this PR and master don't look too far off, but I will say that there is now a huge delta between 8.x and 9.x on citgm which is a bit disturbing. Can't dig into it today but we should figure out what shifted on master to break so many things

refack · 2017-09-20T18:32:39Z

@mcollina I didn't see anything new in CitGM
Except https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/994/nodes=ubuntu1604-64/testReport/(root)/citgm/readable_stream_v2_3_3/ 🤣 (but that's obviusly an infra issue)
There is also one thing I want to follow up on with graceful-fs @ Windows

refack · 2017-09-20T20:30:27Z

There is also one thing I want to follow up on with graceful-fs @ Windows

graceful-fs passes localy

refack

CitGM is clean

mcollina · 2017-09-21T07:09:42Z

Landed as e5c290b.

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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: nodejs/node#2006 PR-URL: nodejs/node#15407 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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: nodejs#2006 PR-URL: nodejs#15407 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

* **async_hooks** * Older experimental `async_hooks` APIs have been removed [[`d731369b1d`](d731369b1d)] **(SEMVER-MAJOR)** [#14414](#14414) * **Errors** * Multiple built in modules have been migrated to use static error codes * **Domains** * The long deprecated `.dispose()` method has been removed [[`602fd36d95`](602fd36d95)] **(SEMVER-MAJOR)** [#15412](#15412) * **File system** * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()` [[`e5c290bed9`](e5c290bed9)] **(SEMVER-MAJOR)** [#15407](#15407) * `fs` callbacks are now invoked with an undefined `this` context [[`2249234fee`](2249234fee)] **(SEMVER-MAJOR)** [#14645](#14645) * **HTTP** * Socket timeout is set when the socket connects [[`10be20a0e8`](10be20a0e8)] **(SEMVER-MAJOR)** [#8895](#8895) * A bug causing the request `error` event to fire twice has been fixed [[`620ba41694`](620ba41694)] **(SEMVER-MAJOR)** [#14659](#14659) * The `pipe` method on `OutgoingMessage` has been disabled [[`156549d8ff`](156549d8ff)] **(SEMVER-MAJOR)** [#14358](#14358) * **HTTP/2** * The `--expose-http2` command-line argument is no longer required [[`f55ee6e24a`](f55ee6e24a)] **(SEMVER-MAJOR)** [#15535](#15535) * **Internationalization** * The `Intl.v8BreakIterator` class has been removed [[`668ad44922`](668ad44922)] **(SEMVER-MAJOR)** [#15238](#15238) * **OS** * `os.EOL` is now read-only [[`f6caeb9526`](f6caeb9526)] **(SEMVER-MAJOR)** [#14622](#14622) * **Process** * It is now possible to pass additional flags to `dlopen` [[`5f22375922`](5f22375922)] **(SEMVER-MAJOR)** [#12794](#12794) * **Timers** * Using a timeout duration larger than 32-bits will now emit a warning [[`ce3586da31`](ce3586da31)] **(SEMVER-MAJOR)** [#15627](#15627) * **TLS** * `parseCertString` has been deprecated [[`468110b327`](468110b327)] **(SEMVER-MAJOR)** [#14249](#14249) * Type-checking for `key`, `cert`, and `ca` options has been added [[`a7dccd040d`](a7dccd040d)] **(SEMVER-MAJOR)** [#14807](#14807)

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: nodejs#17951 See: nodejs#15407

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]>

mcollina added dont-land-on-v6.x fs Issues and PRs related to the fs subsystem / file system. labels Sep 14, 2017

mcollina requested review from bnoordhuis and jasnell September 14, 2017 11:50

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Sep 14, 2017

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 14, 2017

jasnell approved these changes Sep 14, 2017

View reviewed changes

mcollina mentioned this pull request Sep 14, 2017

Closing fs streams early could call close during or before I/O #2006

Closed

BridgeAR reviewed Sep 19, 2017

View reviewed changes

cjihrig approved these changes Sep 20, 2017

View reviewed changes

mcollina force-pushed the fs-destroy branch from b577e46 to 276ef74 Compare September 20, 2017 09:20

fs: refactor close to use destroy

2985aca

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: nodejs#2006

mcollina force-pushed the fs-destroy branch from 276ef74 to 2985aca Compare September 20, 2017 14:37

refack approved these changes Sep 20, 2017

View reviewed changes

mcollina closed this Sep 21, 2017

mcollina deleted the fs-destroy branch September 21, 2017 07:09

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

Plamen5kov mentioned this pull request Dec 12, 2017

fix(snapshot): use autoclose option on writestream NativeScript/nativescript-dev-webpack#345

Merged

mcollina mentioned this pull request Jan 5, 2018

fs: guarantee order of callbacks in ws.close #18002

Closed

4 tasks

mcollina mentioned this pull request Jan 17, 2018

Do not rely on Node.js internals gulpjs/vinyl-fs#296

Closed

ronag mentioned this pull request Aug 19, 2019

[x] stream: don't emit finish after destroy() #29205

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: refactor close to use destroy #15407

fs: refactor close to use destroy #15407

mcollina commented Sep 14, 2017 •

edited

Loading

jasnell left a comment

mcollina commented Sep 14, 2017

jasnell commented Sep 14, 2017

mcollina commented Sep 14, 2017

mcollina commented Sep 19, 2017

BridgeAR Sep 19, 2017

mcollina Sep 20, 2017

cjihrig Sep 20, 2017

mcollina commented Sep 20, 2017 •

edited

Loading

mcollina commented Sep 20, 2017

mcollina commented Sep 20, 2017

mcollina commented Sep 20, 2017

BridgeAR commented Sep 20, 2017

MylesBorins commented Sep 20, 2017

refack commented Sep 20, 2017

refack commented Sep 20, 2017

refack left a comment

mcollina commented Sep 21, 2017

fs: refactor close to use destroy #15407

fs: refactor close to use destroy #15407

Conversation

mcollina commented Sep 14, 2017 • edited Loading

Checklist

Affected core subsystem(s)

jasnell left a comment

Choose a reason for hiding this comment

mcollina commented Sep 14, 2017

jasnell commented Sep 14, 2017

mcollina commented Sep 14, 2017

mcollina commented Sep 19, 2017

BridgeAR Sep 19, 2017

Choose a reason for hiding this comment

mcollina Sep 20, 2017

Choose a reason for hiding this comment

cjihrig Sep 20, 2017

Choose a reason for hiding this comment

mcollina commented Sep 20, 2017 • edited Loading

mcollina commented Sep 20, 2017

mcollina commented Sep 20, 2017

mcollina commented Sep 20, 2017

BridgeAR commented Sep 20, 2017

MylesBorins commented Sep 20, 2017

refack commented Sep 20, 2017

refack commented Sep 20, 2017

refack left a comment

Choose a reason for hiding this comment

mcollina commented Sep 21, 2017

mcollina commented Sep 14, 2017 •

edited

Loading

mcollina commented Sep 20, 2017 •

edited

Loading