fs: refactor close to use destroy by mcollina · Pull Request #15407 · nodejs/node · GitHub

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

LGTM

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.

Understood

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

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/

Should the callback really be triggered sync?

Good spot. Updating.

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

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

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.

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

cc @refack can you have a look?

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

@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

@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

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

graceful-fs passes localy

CitGM is clean

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 <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

* **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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

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

fs: refactor close to use destroy by mcollina · Pull Request #15407 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: refactor close to use destroy - #15407

fs: refactor close to use destroy#15407mcollina wants to merge 1 commit intonodejs:masternodejs/node:masterfrom mcollina:fs-destroymcollina/node:fs-destroyCopy head branch name to clipboard

Conversation

mcollina commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Sep 14, 2017

Uh oh!

jasnell commented Sep 14, 2017

Uh oh!

mcollina commented Sep 14, 2017

Uh oh!

mcollina commented Sep 19, 2017

Uh oh!

BridgeAR Sep 19, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Sep 20, 2017

Uh oh!

mcollina commented Sep 20, 2017

Uh oh!

mcollina commented Sep 20, 2017

Uh oh!

BridgeAR commented Sep 20, 2017

Uh oh!

MylesBorins commented Sep 20, 2017

Uh oh!

refack commented Sep 20, 2017

Uh oh!

refack commented Sep 20, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Sep 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fs: refactor close to use destroy#15407
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:fs-destroy

mcollina commented Sep 14, 2017 •
edited
Loading

mcollina commented Sep 20, 2017 •
edited
Loading