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: unecessary argument validation #29043

Closed
wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 8, 2019

Writable already assures that only Buffer's are passed to _write. Also this is not the "correct" way to handle errors inside _write.

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

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Aug 8, 2019
@ronag ronag changed the title Fs write check fs: unecessary argument validation Aug 8, 2019
@mscdex
Copy link
Contributor

mscdex commented Aug 8, 2019

It's not ensured because the user's options passed to fs.createWriteStream() is passed directly to Writable(). So if a user has decodeStrings: false in options, _write() will get passed a string.

@ronag
Copy link
Member Author

ronag commented Aug 8, 2019

Should that even be a valid option for fs? That option is for Writable implementors. Not users.

@ronag
Copy link
Member Author

ronag commented Aug 8, 2019

I’ll update the PR if the consensus is to keep the check. But the error needs to be passed to the callback to ‘Writable’ and not directly emitted.

Also do we then also need this check for writev?

@ronag
Copy link
Member Author

ronag commented Aug 8, 2019

Fixed so that decodeStrings is always true.

@ronag ronag mentioned this pull request Aug 8, 2019
11 tasks
@ronag ronag force-pushed the fs-write-check branch 2 times, most recently from 70f1792 to 0e1f130 Compare August 8, 2019 13:23
@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member Author

ronag commented Aug 17, 2019

@Trott this seems ready?

@Trott
Copy link
Member

Trott commented Aug 17, 2019

@Trott this seems ready?

Seems like it. @nodejs/streams @mcollina Do you have any thoughts on this one? (EDIT: Yeah, I'm being super extra cautious, I guess.)

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 17, 2019
@Trott
Copy link
Member

Trott commented Aug 19, 2019

Extra-cautious CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1970/ (queued)

@danbev
Copy link
Contributor

danbev commented Aug 20, 2019

@ronag Would you be able to rebase this and take a look at the conflict? Thanks

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 23, 2019
@ronag
Copy link
Member Author

ronag commented Aug 26, 2019

@danbev: rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants