src: remove 2nd `undefined` argument in node_file.cc #20629

dankang · 2018-05-09T14:50:00Z

In the document for fs, there are several functions that state "No
arguments other than a possible exception are given to the completion
callback." (ex> fs.access, fs.chmod, fs.close, ..)

But, the functions are invoking the callback with two parameters (err,
undefined)

It causes problems in using several API like
async.waterfall.

Fixes: #20335

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows [commit guidelines]

mscdex · 2018-05-09T15:00:10Z

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

mscdex · 2018-05-09T15:09:55Z

src/node_file.cc

-  MakeCallback(env()->oncomplete_string(), arraysize(argv), argv);
+  int len = arraysize(argv);
+  MakeCallback(env()->oncomplete_string(),
+               len == 2 && value->IsUndefined() ? 1 : len,


Isn't the length check here redundant as it will always be 2? Why not just check the undefined-ness?

Yes, right. I fixed.
I thought somebody can modify the function to multiple arguments, but too much worry.

mscdex · 2018-05-09T15:10:20Z

The newly added test seems to be causing a hard crash.

dankang · 2018-05-09T15:29:49Z

@mscdex I divided the test case into 2 different cases.
(test-fs-close-cb-parameters.js is from #20335)
I can't test them right now because of my battery, will check them later if it is ok.

mscdex · 2018-05-09T15:59:53Z

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

mscdex · 2018-05-09T17:20:54Z

It seems sequential/test-fs-close-cb-parameters.js is the one crashing now.

dankang · 2018-05-10T01:38:36Z

@mscdex I modified the test cases.
Could you please start a CI run?

use existing test case for fs.access()
- remove sequential/test-fs-access-cb-parameters.js
- add parameter checking code in parallel/test-fs-access.js
move test case for fs.close()
- remove sequential/test-fs-close-cb-parameters.js
- add a simple test case to parallel/test-fs-close.js

mscdex · 2018-05-10T04:04:35Z

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

dankang · 2018-05-10T05:15:45Z

Tests failed in several linux versions.
I will set up one of the linux systems and run tests on it.

dankang · 2018-05-10T08:21:29Z

I set an ubuntu1710-x64 system on AWS that had failed in the CI node-test-commit-linux.
But, the test is succesful in my system.

Also, I tested it on debian9-64 on AWS, it was successful.
I couldn't test it on ubuntu1604-32, because there is no image on AWS.

ubuntu1710-64: Successful
debian9-64: Successful
ubuntu1604-32: not tested

Could you please tell me what I should do more in this case?

mscdex · 2018-05-10T10:41:39Z

I think those are just unrelated, flaky failures.

dankang · 2018-05-10T13:32:41Z

I rebased master branch and tested on 3 different linux systems.
Can you run a CI test again?

joyeecheung · 2018-05-10T15:11:39Z

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

dankang · 2018-05-10T21:37:11Z

In this time, the CI run failed at fetching git repo step in osx.

dankang · 2018-05-11T15:09:32Z

I rebased master and merged my commits.
And, I ran test in below 4 environments.

osx
ubuntu 1604-64
ubuntu 1710-64
debian9-64

Can anybody run a CI test?
Or, anybody comment about my commit?

joyeecheung · 2018-05-14T08:03:40Z

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

In the document for fs, there are several functions that state "No arguments other than a possible exception are given to the completion callback." (ex> fs.access, fs.chmod, fs.close, ..) But, the functions are invoking the callback with two parameters (err, undefined) It causes problems in using several API like [async.waterfall](https://caolan.github.io/async/docs.html#waterfall). Fixes: nodejs#20335

BridgeAR · 2018-05-18T01:32:57Z

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

In the document for fs, there are several functions that state "No arguments other than a possible exception are given to the completion callback." (ex> fs.access, fs.chmod, fs.close, ..) But, the functions are invoking the callback with two parameters (err, undefined) It causes problems in using several API like [async.waterfall](https://caolan.github.io/async/docs.html#waterfall). PR-URL: nodejs#20629 Fixes: nodejs#20335 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

BridgeAR · 2018-05-18T13:52:01Z

Landed in 60cc8ff

@dankang congratulations on your first commit to Node.js! 🎉 You are now officially a contributor :-)

dankang · 2018-05-19T01:12:10Z

Thanks a lot! 👍

In the document for fs, there are several functions that state "No arguments other than a possible exception are given to the completion callback." (ex> fs.access, fs.chmod, fs.close, ..) But, the functions are invoking the callback with two parameters (err, undefined) It causes problems in using several API like [async.waterfall](https://caolan.github.io/async/docs.html#waterfall). PR-URL: #20629 Fixes: #20335 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels May 9, 2018

mscdex reviewed May 9, 2018

View reviewed changes

dankang force-pushed the fs-callback branch from 49f4b09 to c3ab50c Compare May 10, 2018 08:18

dankang force-pushed the fs-callback branch from c3ab50c to 8aa9c17 Compare May 10, 2018 12:21

dankang closed this May 10, 2018

dankang reopened this May 10, 2018

dankang force-pushed the fs-callback branch 5 times, most recently from 3835724 to 7b958c3 Compare May 11, 2018 14:22

jasnell requested review from bnoordhuis and addaleax May 12, 2018 23:18

jasnell approved these changes May 12, 2018

View reviewed changes

joyeecheung approved these changes May 14, 2018

View reviewed changes

dankang force-pushed the fs-callback branch from 7b958c3 to b7ab367 Compare May 15, 2018 02:59

BridgeAR approved these changes May 18, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 18, 2018

BridgeAR closed this May 18, 2018

richardlau mentioned this pull request May 22, 2018

fs methods returning an additional, undefined value in callback #20872

Closed

addaleax mentioned this pull request May 22, 2018

v10.2.0 proposal #20724

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: remove 2nd `undefined` argument in node_file.cc #20629

src: remove 2nd `undefined` argument in node_file.cc #20629

dankang commented May 9, 2018

mscdex commented May 9, 2018

mscdex May 9, 2018

dankang May 9, 2018

mscdex commented May 9, 2018

dankang commented May 9, 2018 •

edited

Loading

mscdex commented May 9, 2018

mscdex commented May 9, 2018

dankang commented May 10, 2018

mscdex commented May 10, 2018

dankang commented May 10, 2018

dankang commented May 10, 2018 •

edited

Loading

mscdex commented May 10, 2018 •

edited

Loading

dankang commented May 10, 2018

joyeecheung commented May 10, 2018

dankang commented May 10, 2018

dankang commented May 11, 2018 •

edited

Loading

joyeecheung commented May 14, 2018

BridgeAR commented May 18, 2018

BridgeAR commented May 18, 2018

dankang commented May 19, 2018

src: remove 2nd undefined argument in node_file.cc #20629

src: remove 2nd undefined argument in node_file.cc #20629

Conversation

dankang commented May 9, 2018

Checklist

mscdex commented May 9, 2018

mscdex May 9, 2018

Choose a reason for hiding this comment

dankang May 9, 2018

Choose a reason for hiding this comment

mscdex commented May 9, 2018

dankang commented May 9, 2018 • edited Loading

mscdex commented May 9, 2018

mscdex commented May 9, 2018

dankang commented May 10, 2018

mscdex commented May 10, 2018

dankang commented May 10, 2018

dankang commented May 10, 2018 • edited Loading

mscdex commented May 10, 2018 • edited Loading

dankang commented May 10, 2018

joyeecheung commented May 10, 2018

dankang commented May 10, 2018

dankang commented May 11, 2018 • edited Loading

joyeecheung commented May 14, 2018

BridgeAR commented May 18, 2018

BridgeAR commented May 18, 2018

dankang commented May 19, 2018

src: remove 2nd `undefined` argument in node_file.cc #20629

src: remove 2nd `undefined` argument in node_file.cc #20629

dankang commented May 9, 2018 •

edited

Loading

dankang commented May 10, 2018 •

edited

Loading

mscdex commented May 10, 2018 •

edited

Loading

dankang commented May 11, 2018 •

edited

Loading