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: guarantee order of callbacks in ws.close #18002

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented 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: #17951
See: #15407

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

@mcollina mcollina requested a review from bnoordhuis January 5, 2018 09:46
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jan 5, 2018
@mcollina
Copy link
Member Author

mcollina commented Jan 5, 2018

The fix for #17951 uncovered several other issues, which I fixed here. It ended up being a significant rework of #15407.

There is a little behavior change, which we might or might not want to do. With this change, the .closed property is set to true before emitting 'close', and not within the 'finish' event because the file descriptor is not closed yet. I would note that the .closed property is not documented, but it has been with us for a long time (at least 0.10).

@mcollina
Copy link
Member Author

mcollina commented Jan 5, 2018

lib/fs.js Outdated
if (this._writableState.ended) {
process.nextTick(cb);
return;
// if we are not autoclosing, we should close
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize and punctuate, per piacere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

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

mcollina commented Jan 8, 2018

Landed as acf56be

@mcollina mcollina closed this Jan 8, 2018
@mcollina mcollina deleted the fix-17951 branch January 8, 2018 09:08
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 <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mcollina
Copy link
Member Author

mcollina commented Jan 9, 2018

@MylesBorins this needs to be pulled in v9 for the next release as well.

@MylesBorins
Copy link
Contributor

@mcollina done

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 <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mcollina
Copy link
Member Author

mcollina commented Jan 9, 2018

thanks!

@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
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 <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

createWriteStream on Ubuntu not using default autoClose option
5 participants