Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: refactor close to use destroy #15407

Closed
wants to merge 1 commit into from
Closed

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Sep 14, 2017

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

@mcollina mcollina added dont-land-on-v6.x fs Issues and PRs related to the fs subsystem / file system. labels Sep 14, 2017
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Sep 14, 2017
@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 14, 2017
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina
Copy link
Member Author

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
Copy link
Member

jasnell commented Sep 14, 2017

Understood

@mcollina
Copy link
Member Author

@mcollina
Copy link
Member Author

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/

lib/fs.js Outdated
return;
}
cb(err);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the callback really be triggered sync?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot. Updating.

{
const s = fs.createReadStream(__filename);

// this is a private API, but it is worth
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Member Author

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
Copy link
Member Author

@mcollina
Copy link
Member Author

cc @refack can you have a look?

@BridgeAR
Copy link
Member

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

@MylesBorins
Copy link
Contributor

@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
Copy link
Contributor

refack commented Sep 20, 2017

@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
Copy link
Contributor

refack commented Sep 20, 2017

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

graceful-fs passes localy

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CitGM is clean

@mcollina
Copy link
Member Author

Landed as e5c290b.

@mcollina mcollina closed this Sep 21, 2017
@mcollina mcollina deleted the fs-destroy branch September 21, 2017 07:09
mcollina added a commit that referenced this pull request Sep 21, 2017
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]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
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]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
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]>
lukaszewczak pushed a commit to lukaszewczak/node that referenced this pull request Sep 23, 2017
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]>
jasnell added a commit that referenced this pull request Oct 17, 2017
* **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)
jasnell added a commit that referenced this pull request Oct 29, 2017
* **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)
jasnell added a commit that referenced this pull request Oct 29, 2017
* **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)
jasnell added a commit that referenced this pull request Oct 29, 2017
* **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)
mcollina added a commit to mcollina/node that referenced this pull request Jan 5, 2018
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
mcollina added a commit that referenced this pull request Jan 8, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Jan 10, 2018
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Closing fs streams early could call close during or before I/O
7 participants