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: do not pass Buffer when toString() fails #9670

Merged
merged 1 commit into from Dec 14, 2016

Conversation

Projects
None yet
8 participants
@mscdex
Contributor

mscdex commented Nov 18, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
  • fs
Description of change

Even though an Error object is passed to the callback when readFile() fails due to toString() failing, it is a bit strange to still see data passed as the second argument. This commit changes that and only
passes the Error object in that case.

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

@targos

targos approved these changes Nov 18, 2016

LGTM

@sam-github

Isn't the bufer useful? Particularly here, converting the Buffer to a string failed, but the buffer was sucessfully read, so why not return the data?

Show outdated Hide outdated lib/fs.js
@@ -406,7 +406,7 @@ function readFileAfterClose(err) {
else
buffer = context.buffer;
if (err) return callback(err, buffer);
if (err) return callback(err);

This comment has been minimized.

@sam-github

sam-github Nov 18, 2016

Member

Shouldn't this move to line 401, before the buffer object (which is no longer being passed), goes through the code to create/change it?

@sam-github

sam-github Nov 18, 2016

Member

Shouldn't this move to line 401, before the buffer object (which is no longer being passed), goes through the code to create/change it?

This comment has been minimized.

@mscdex

mscdex Nov 18, 2016

Contributor

Done.

@mscdex

mscdex Nov 18, 2016

Contributor

Done.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Nov 18, 2016

Contributor

@sam-github I think it's more confusing not only because we don't do that for any other node core API (that I am aware of) but because if you asked for a string, I would never expect a Buffer -- whether there was an error or not. Also, the Buffer is probably useless in most cases since there is usually a reason why a string was requested.

Contributor

mscdex commented Nov 18, 2016

@sam-github I think it's more confusing not only because we don't do that for any other node core API (that I am aware of) but because if you asked for a string, I would never expect a Buffer -- whether there was an error or not. Also, the Buffer is probably useless in most cases since there is usually a reason why a string was requested.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

This comment has been minimized.

Show comment
Hide comment
Contributor

mscdex commented Dec 14, 2016

fs: do not pass Buffer when toString() fails
Even though an Error object is passed to the callback when readFile()
fails due to toString() failing, it is a bit strange to still see
data passed as the second argument. This commit changes that and only
passes the Error object in that case.

PR-URL: #9670
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@mscdex mscdex removed the lts-watch-v6.x label Dec 14, 2016

@mscdex mscdex merged commit f3cf8e9 into nodejs:master Dec 14, 2016

@mscdex mscdex deleted the mscdex:fs-tostring-failure-callback branch Dec 14, 2016

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017

fs: do not pass Buffer when toString() fails
Even though an Error object is passed to the callback when readFile()
fails due to toString() failing, it is a bit strange to still see
data passed as the second argument. This commit changes that and only
passes the Error object in that case.

PR-URL: nodejs#9670
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment