Skip to content

fs: refactor close to use destroy #15407

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.

Sign up for GitHub

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

Jump to bottom
Closed
mcollina wants to merge 1 commit into from
Closed

fs: refactor close to use destroy #15407

mcollina wants to merge 1 commit into from

Conversation

@mcollina
Copy link
Member

@mcollina mcollina commented Sep 14, 2017 •
edited
Loading

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
jasnell
jasnell approved these changes Sep 14, 2017
View reviewed changes
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

mcollina commented Sep 14, 2017

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 mcollina mentioned this pull request Sep 14, 2017
Closed
@mcollina
Copy link
Member Author

mcollina commented Sep 14, 2017

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

@mcollina
Copy link
Member Author

mcollina commented Sep 19, 2017

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
BridgeAR reviewed Sep 19, 2017
View reviewed changes
lib/fs.js Outdated
Copy link
Member

@BridgeAR BridgeAR Sep 19, 2017

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

@mcollina mcollina Sep 20, 2017

Choose a reason for hiding this comment

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

Good spot. Updating.

cjihrig
cjihrig approved these changes Sep 20, 2017
View reviewed changes
test/parallel/test-fs-read-stream-double-close.js Outdated
Copy link
Contributor

@cjihrig cjihrig Sep 20, 2017

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.

@mcollina
Copy link
Member Author

mcollina commented Sep 20, 2017 •
edited
Loading

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/

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

mcollina commented Sep 20, 2017

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 commented Sep 20, 2017

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

mcollina commented Sep 20, 2017

cc @refack can you have a look?

@BridgeAR
Copy link
Member

BridgeAR commented Sep 20, 2017

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

@MylesBorins
Copy link
Contributor

MylesBorins commented Sep 20, 2017

@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

refack
refack approved these changes Sep 20, 2017
View reviewed changes
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

mcollina commented Sep 21, 2017

Landed as e5c290b.

@mcollina mcollina closed this Sep 21, 2017
@mcollina mcollina deleted the fs-destroy branch September 21, 2017 07:09
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@Plamen5kov Plamen5kov mentioned this pull request Dec 12, 2017
Merged
@mcollina mcollina mentioned this pull request Jan 5, 2018
Closed
4 tasks
@mcollina mcollina mentioned this pull request Jan 17, 2018
Closed
@ronag ronag mentioned this pull request Aug 19, 2019
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@bnoordhuis bnoordhuis Awaiting requested review from bnoordhuis

+1 more reviewer

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

7 participants

@mcollina @jasnell @BridgeAR @MylesBorins @refack @cjihrig @nodejs-github-bot