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: create callback before nullCheck call #7167

Closed

Conversation

thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Jun 5, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

fs

Description of change

The callback function creation should happen before the nullCheck,
because it might invoke the callback to indicate errors.

This was pointed out by @cjihrig, in #7068 (comment)

cc @nodejs/fs

The callback function creation should happen before the `nullCheck`,
because it might invoke the callback to indicate errors.
@thefourtheye thefourtheye added the fs Issues and PRs related to the fs subsystem / file system. label Jun 5, 2016
@thefourtheye
Copy link
Contributor Author


assert.throws(function() {
fs.access(__filename, fs.F_OK, {});
}, /"callback" argument must be a function/);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this behavior should change. Calling fs.access() without a callback arguably never makes sense.

@bnoordhuis
Copy link
Member

This change is not really necessary. makeCallback() ensures that the callback doesn't run with this === wrapperObject (something we can fix in node_file.cc these days) but that's not an issue with nullCheck, it calls process.nextTick().

@thefourtheye
Copy link
Contributor Author

@bnoordhuis Correct. This change will not do any good.

@thefourtheye thefourtheye deleted the fs-consistent-callback branch June 6, 2016 12:14
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.

2 participants