fs: promises fixes #20407

ChALkeR · 2018-04-29T18:19:32Z

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

This fixes two errors in fs/promises:

Removes the leftovers from write(fd, string[, position[, encoding]], callback) signature arg-shifting to find callback — that variable was unused and that logic was corrupting encoding.
chown should do chown, not chmod.

Sorry, this doesn't include tests — I would appreciate some help there. In general, fs/promises test coverage is poor, and this PR isn't aimed at that (yet). Also, #19811 aims to increase the test covereage for fs/promises.

Perhaps #20406 need to be resolved first — the first patch fixes technically undocumented behavior and I am not sure we should be testing it or removing it.

mscdex · 2018-04-29T18:25:07Z

lib/fs/promises.js

@@ -402,7 +393,7 @@ async function lchown(path, uid, gid) {
  if (O_SYMLINK !== undefined) {
    const fd = await open(path,
                          O_WRONLY | O_SYMLINK);
-    return fchmod(fd, uid, gid).finally(fd.close.bind(fd));
+    return fchown(fd, uid, gid).finally(fd.close.bind(fd));


Is it possible to add a test for this?

@mscdex In general, fs/promises has poor coverage and this is not the only method that lacks it completely. In the current state, I view that as a separate task. I mentioned that in the PR.

This is not some edge case — the whole public method (among with several others) completely lacks tests and they need to be written.

Ref: https://coverage.nodejs.org/coverage-b55a11d1b17b3e4b/root/fs/promises.js.html

davisjam

Nice catch on fchmod vs fchown.

I think it's appropriate to improve things as you go along, so I think adding a TC for lchown would be good as part of this PR.

benjamingr · 2018-04-30T07:09:38Z

I think it's appropriate to improve things as you go along, so I think adding a TC for lchown would be good as part of this PR.

I don't think it's fair to require this here (requesting is fine though) as this PR is an improvement and a bug fix as is. Plus tests for it could be great for a code & learn.

ChALkeR · 2018-04-30T19:00:46Z

@davisjam @mscdex I opened issue #20435 for tests.
Writing tests for the whole module is out of scope of this PR which is a simple fix.

If there were tests present and this fixed a corner case, I would have included a test for that corner case, but there are just no tests for those functions at all currently (and testing just the patched one won't even improve the situation much).

E.g. #20435 (comment) could be a good approach, but again — out of scope here.
Note how that comment (which I did not predict) demonstrates that the lack of tests should be discussed separately.

davisjam · 2018-04-30T19:19:17Z

@ChALkeR Clearly I overestimated the state of tests. I retract my objection.

mscdex · 2018-04-30T21:14:40Z

I don't understand why tests have to be added separately? I was under the impression that bug fixes should generally come with test(s) to ensure the bug doesn't happen again? To be clear, I wasn't asking for tests for the entire set of fs/promise APIs, just the one being affected in this PR.

ChALkeR · 2018-05-01T04:56:01Z

@mscdex @devsnek Because in the current state, that's a separate problem, meaning that it requires more work and discussion than a mere fix here. E.g. #20439 is a good way to address it.

ChALkeR · 2018-05-06T00:09:48Z

@mscdex @devsnek @BridgeAR Are there objections to landing this as it is now and dealing with the tests separately?

BridgeAR · 2018-05-06T00:20:38Z

I personally think it would still be best to land it with tests. I know we work on getting something else going but until then, the extra test would help.

Nevertheless, I am not going to block this without a test either. So if it is only about me: go ahead and land it.

ChALkeR · 2018-05-06T17:27:53Z

I am going to merge this as it is tomorrow if there would be no explicit concerns.

I still strongly believe that lack of tests for fs/promises should be adressed separately and attemptiing to adress that issue here would just provide a defective result.

ChALkeR · 2018-05-08T08:06:55Z

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

ChALkeR · 2018-05-08T10:08:36Z

stringbytes-external-exceed-max failures on osx look unrelated.

That code expects the last argument to be a callback. When it's not a callback, it shifts arguments, defaulting encoding to 'utf-8', which is clearly broken. Old signature: (fd, string[, position[, encoding]], callback) New signature: (fd, string[, position[, encoding]]) PR-URL: nodejs#20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

This was a clear error. chown should do chown, not chmod. PR-URL: nodejs#20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

ChALkeR · 2018-05-08T10:20:29Z

Landed in dd03709...15f7431.

That code expects the last argument to be a callback. When it's not a callback, it shifts arguments, defaulting encoding to 'utf-8', which is clearly broken. Old signature: (fd, string[, position[, encoding]], callback) New signature: (fd, string[, position[, encoding]]) PR-URL: #20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

This was a clear error. chown should do chown, not chmod. PR-URL: #20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

That code expects the last argument to be a callback. When it's not a callback, it shifts arguments, defaulting encoding to 'utf-8', which is clearly broken. Old signature: (fd, string[, position[, encoding]], callback) New signature: (fd, string[, position[, encoding]]) PR-URL: #20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

This was a clear error. chown should do chown, not chmod. PR-URL: #20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

That code expects the last argument to be a callback. When it's not a callback, it shifts arguments, defaulting encoding to 'utf-8', which is clearly broken. Old signature: (fd, string[, position[, encoding]], callback) New signature: (fd, string[, position[, encoding]]) PR-URL: #20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

This was a clear error. chown should do chown, not chmod. PR-URL: #20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

ChALkeR added the fs Issues and PRs related to the fs subsystem / file system. label Apr 29, 2018

mscdex reviewed Apr 29, 2018

View reviewed changes

vsemozhetbyt added the promises Issues and PRs related to ECMAScript promises. label Apr 29, 2018

ChALkeR mentioned this pull request Apr 29, 2018

fs: promises have undocumented write(string,…) method #20406

Closed

benjamingr approved these changes Apr 29, 2018

View reviewed changes

davisjam suggested changes Apr 29, 2018

View reviewed changes

ChALkeR mentioned this pull request Apr 30, 2018

fs/promises test coverage is poor. #20435

Closed

TimothyGu approved these changes Apr 30, 2018

View reviewed changes

davisjam approved these changes Apr 30, 2018

View reviewed changes

jasnell approved these changes Apr 30, 2018

View reviewed changes

cjihrig approved these changes May 1, 2018

View reviewed changes

cjihrig mentioned this pull request May 6, 2018

fs: refactor promises version of lchown and lchmod #20551

Merged

2 tasks

ChALkeR added the experimental Issues and PRs related to experimental features. label May 8, 2018

ChALkeR force-pushed the fs-promises-fixes branch from 5b8c64d to e2ae643 Compare May 8, 2018 08:05

thefourtheye approved these changes May 8, 2018

View reviewed changes

ChALkeR added 2 commits May 8, 2018 13:10

ChALkeR force-pushed the fs-promises-fixes branch from e2ae643 to 15f7431 Compare May 8, 2018 10:16

ChALkeR merged commit 15f7431 into nodejs:master May 8, 2018

MylesBorins mentioned this pull request May 8, 2018

v10.1.0 proposal #20606

Merged

ChALkeR mentioned this pull request Aug 23, 2018

dns.promises test coverage is poor #22471

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: promises fixes #20407

fs: promises fixes #20407

ChALkeR commented Apr 29, 2018 •

edited

Loading

mscdex Apr 29, 2018

ChALkeR Apr 29, 2018 •

edited

Loading

davisjam left a comment

benjamingr commented Apr 30, 2018

ChALkeR commented Apr 30, 2018 •

edited

Loading

davisjam commented Apr 30, 2018

mscdex commented Apr 30, 2018 •

edited

Loading

ChALkeR commented May 1, 2018 •

edited

Loading

ChALkeR commented May 6, 2018

BridgeAR commented May 6, 2018

ChALkeR commented May 6, 2018

ChALkeR commented May 8, 2018

ChALkeR commented May 8, 2018

ChALkeR commented May 8, 2018 •

edited

Loading

fs: promises fixes #20407

fs: promises fixes #20407

Conversation

ChALkeR commented Apr 29, 2018 • edited Loading

Checklist

mscdex Apr 29, 2018

Choose a reason for hiding this comment

ChALkeR Apr 29, 2018 • edited Loading

Choose a reason for hiding this comment

davisjam left a comment

Choose a reason for hiding this comment

benjamingr commented Apr 30, 2018

ChALkeR commented Apr 30, 2018 • edited Loading

davisjam commented Apr 30, 2018

mscdex commented Apr 30, 2018 • edited Loading

ChALkeR commented May 1, 2018 • edited Loading

ChALkeR commented May 6, 2018

BridgeAR commented May 6, 2018

ChALkeR commented May 6, 2018

ChALkeR commented May 8, 2018

ChALkeR commented May 8, 2018

ChALkeR commented May 8, 2018 • edited Loading

ChALkeR commented Apr 29, 2018 •

edited

Loading

ChALkeR Apr 29, 2018 •

edited

Loading

ChALkeR commented Apr 30, 2018 •

edited

Loading

mscdex commented Apr 30, 2018 •

edited

Loading

ChALkeR commented May 1, 2018 •

edited

Loading

ChALkeR commented May 8, 2018 •

edited

Loading