Revert fs changes #7846

MylesBorins · 2016-07-22T23:46:39Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

fs

Description of change

9359de9 and c86c1ee are two semver major changes to the fs module that landed in the last 24 hours. When trying to run CITGM it was discovered that npm set was throwing an error

0 info it worked if it ends with ok
1 verbose cli [ '/usr/local/bin/node',
1 verbose cli   '/usr/local/bin/npm',
1 verbose cli   'set',
1 verbose cli   'progress=false' ]
2 info using [email protected]
3 info using [email protected]
4 info config set "progress" "false"
5 verbose stack TypeError: cb is not a function
5 verbose stack     at /usr/local/lib/node_modules/npm/node_modules/graceful-fs/polyfills.js:210:7
5 verbose stack     at FSReqWrap.oncomplete (fs.js:103:15)
6 verbose cwd /Users/thealphanerd/code/node/master
7 error Darwin 14.5.0
8 error argv "/usr/local/bin/node" "/usr/local/bin/npm" "set" "progress=false"
9 error node v7.0.0-pre
10 error npm  v3.10.3
11 error cb is not a function
12 error If you need help, you may report this error at:
12 error     <https://github.com/npm/npm/issues>
13 verbose exit [ 1, true ]

I think that we should revert asap and revisit these two commits in a new PR to figure out what is going on here

edit: it looks like the problem may be in graceful-fs. I'm going to dig into finding a fix for it, I will also look into adding it to citgm, as it has definitely been a module that has caused pain in the past

MylesBorins · 2016-07-22T23:48:39Z

/cc @thefourtheye @trevnorris

trevnorris · 2016-07-24T06:12:44Z

@thealphanerd: i've gotten bored being tied down by a dependency of a dependency. graceful-fs reverted the proper fix that would have prevented this b/c of wanting ancient node support. though iirc npm no longer supports that version of node, so the fix can land yet again.

it's pathetic these need to be reverted. let's prioritize fixing our broken dependency then landing these improvements again.

ChALkeR · 2016-07-24T06:24:00Z

graceful-fs reverted the proper fix that would have prevented this b/c of wanting ancient node support.

…

let's prioritize fixing our broken dependency then landing these improvements again.

Uh-oh. I hope this won't result in a major graceful-fs version bump to fix this.

jasnell · 2016-07-25T16:23:33Z

to be honest I'm not that thrilled by this either. Let's get this on the @nodejs/ctc agenda for discussion

MylesBorins · 2016-07-26T00:38:44Z

So I did a bit more digging today.

There was indeed an issue in graceful-fs regarding how chown was being polyfilled, there is currently a PR in to fix this

Even with this fix patched onto npm we are still encountering failures due to other modules in the graceful-fs dep tree that make async fs calls without providing a callback. Due to the way graceful-fs shims chmod, the stack traces are not super useful. It has been suggested in the thread that there may be a more graceful way to handle that situation, by providing a no-op callback to chown. This PR will likely fix the pain point in graceful-fs, but it may take a bit of time.

While it may be easy to consider this "another graceful-fs" problem I think it is a sign of wider usage of async functions without a callback in the ecosystem. While doing so is obviously not a best practice, it is happening in the wild, and this change is breaking stuff.

@ChALkeR and I were discussing this on IRC and are going to attempt to figure out a list of modules that will break with this change, and will be working on getting updates across the ecosystem to make this change less disruptive.

We should revert until the fix to graceful-fs lands, the fix is downstreamed to npm, and we have made a decent effort to make sure the ecosystem breakages are minimal.

ChALkeR · 2016-07-26T05:25:15Z

@jasnell, @trevnorris This isn't only graceful-fs fault this time. Even with graceful-fs being patched, there are still errors — it looks like 9359de9 breaks too many modules. Will post the actual results a bit later.

jasnell · 2016-07-26T05:36:12Z

Yep, understood. I had a chat with @thealphanerd about it earlier this
evening. I'm +1 on reverting until it's sorted out more

On Monday, July 25, 2016, Сковорода Никита Андреевич <
[email protected]> wrote:

@jasnell https://github.com/jasnell, @trevnorris
https://github.com/trevnorris This isn't only graceful-fs fault this
time. Even with graceful-fs being patched, there are still errors — it
looks like 9359de9
9359de9
breaks too many modules. Will post the actual results a bit later.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7846 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eVPeIJQ0k60tmcsLvOxyXRNSTlHcks5qZZpCgaJpZM4JTOGq
.

evanlucas · 2016-07-26T10:10:54Z

+1 on reverting. I think the realpath situation has shown us we really need to be careful with the impact that breaking changes have on the ecosystem

MylesBorins · 2016-07-26T18:53:20Z

For reference isaacs/node-graceful-fs#71 has landed in graceful-fs

graceful-fs had a bug fix, isaacs/node-graceful-fs#71, which would fix the problem in Node.js nodejs/node#7846

bnoordhuis · 2016-07-27T12:58:29Z

These changes would be released in v7, correct? That is a major release and therefore allowed to be backwards incompatible.

While we should not be introducing backwards incompatible changes just for the sake of it, code that omits the callback is borderline broken and I think it's good to turn that into an early error. It's one less avenue for time bombs to sneak into a code base.

We shouldn't be afraid to land changes that expose bugs, provided there is an easy way to fix broken code, even when that causes some initial breakage and churn in the ecosystem. Churn because of bugs fixed is good churn.

ChALkeR · 2016-07-27T15:25:34Z

@bnoordhuis It should still go through a proper deprecation cycle. Printing a warning at first would be better, I suppose.

ChALkeR · 2016-07-27T15:26:49Z

Could we print warnings for now and turn those into errors in v8.0?

jasnell · 2016-07-27T20:21:06Z

Can we please split this into two separate PRs, one for each revert commit. /cc @nodejs/ctc

MylesBorins · 2016-07-27T22:59:06Z

@jasnell the original commit cannot be reverted cleanly without the other commit being reverted first

isaacs · 2016-07-28T00:17:11Z

This project has a process for making planned breaking changes to core features.

It starts with documentation and deprecation warnings, and then the breaking change comes later.

Introducing this sort of thing without a warning to the user is irresponsible. Please do not do that.

I understand that it's frustrating to work within the constraints imposed by not breaking users' programs. It's frustrating to be responsible sometimes. There are lots of projects out there that don't require this level of responsibility because they aren't popular platforms, but that's what's required to do a good job with this one.

These processes are in place to protect the stability of the platform because stability is what matters most for a platform.

addaleax · 2016-08-03T21:12:26Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/344/

(I think that’s what was requested on the CTC meeting, and it won’t hurt anyway.)

jasnell · 2016-08-03T21:14:49Z

Woo! saved me a step, I was just heading off to do exactly that...

This reverts commit 9359de9. Original Commit Message: The "fs" module has two functions called `maybeCallback` and `makeCallback`, as of now. The `maybeCallback` creates a default function to report errors, if the parameter passed is not a function object. Basically, if the callback is omitted in some cases, this function is used to create a default callback function. The `makeCallback`, OTOH, creates a default function only if the parameter passed is `undefined`, and if it is not a function object it will throw an `Error`. This patch removes the `maybeCallback` function and makes the callback function argument mandatory for all the async functions. PR-URL: nodejs#7168 Reviewed-By: Trevor Norris <[email protected]>

MylesBorins · 2016-08-04T16:51:24Z

so there are failures in citgm... especially for

request
async
vinyl-fs
eslint

All of eslint failures are infra related, they are using a module that doesn't support node v7. Async / request has been known flaky for a while, we just have not been seeing the errors due to a bug in citgm. I'll be digging more into them next week. vinyl-fs has had some problems this past week at all.

When I get back next week I'll do a deep dive across all versions and update the flaky status for all modules for all lines

MylesBorins · 2016-08-04T17:29:18Z

Can people throw some LGTM's on this? Without this reverted npm is broken on master (has been for almost 2 weeks now)

addaleax · 2016-08-04T17:30:53Z

Ping @nodejs/ctc
(Mine still stands, fwiw)

jasnell · 2016-08-04T17:32:33Z

The CTC decision yesterday was to go ahead with the revert so... LGTM.

The bias, however, was towards finding a way for the revert to be temporary. Specifically, the overall opinion appears to be that we want these APIs to throw if a callback is not provided, but we'll just take a bit more time to get there. Emitting either a deprecation warning or process warning (process.emitWarning()) seemed to be the best approach (I think we landed on deprecation warning yesterday but I'm not 100% sure)

addaleax · 2016-08-04T22:15:32Z

Landed in 21b0a27

This reverts commit 9359de9. Original Commit Message: The "fs" module has two functions called `maybeCallback` and `makeCallback`, as of now. The `maybeCallback` creates a default function to report errors, if the parameter passed is not a function object. Basically, if the callback is omitted in some cases, this function is used to create a default callback function. The `makeCallback`, OTOH, creates a default function only if the parameter passed is `undefined`, and if it is not a function object it will throw an `Error`. This patch removes the `maybeCallback` function and makes the callback function argument mandatory for all the async functions. PR-URL: #7168 Reviewed-By: Trevor Norris <[email protected]> PR-URL: #7846 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

cjihrig · 2016-08-10T16:45:13Z

Adding the don't land on v6.x label, as this reverts a semver major change.

jasnell · 2016-08-10T16:49:32Z

@cjihrig ... I believe reverting these in v6 is something that we need to do and was discussed on the @nodejs/ctc call. Specifically, the plan is to back off throwing when the callback is not given and to issue a deprecation warning instead. Throwing just caused too much breakage in the ecosystem.

@nodejs/ctc @thealphanerd ... does this match your recollection?

cjihrig · 2016-08-10T16:53:03Z

@jasnell this reverts PR #7168, which is semver major, and doesn't appear to have landed on v6. So, I think this only needed to be reverted in master.

jasnell · 2016-08-10T16:54:36Z

-.- ... heh, you're right, of course. I keep thinking that this one had landed in v6 but I keep confusing it with the other fs changes.

cjihrig · 2016-08-10T16:55:28Z

Yea, fs is a bit of a mess right now :-)

@thefourtheye

Fix bug in chmod wrapper that was tickled by recent node.js changes. (nodejs/node#7846) PR-URL: isaacs/node-graceful-fs#71 Credit: @thefourtheye Reviewed-By: @isaacs

@thefourtheye

graceful-fs had a bug fix, isaacs/node-graceful-fs#71, which would fix the problem in Node.js nodejs/node#7846 PR-URL: #13497 Credit: @thefourtheye Reviewed-By: @iarna

MylesBorins added confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. labels Jul 22, 2016

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 22, 2016

cjihrig mentioned this pull request Jul 23, 2016

src: remove unused IsInt64() function #7839

Closed

2 tasks

Trott mentioned this pull request Jul 23, 2016

Role of CTC in semver-major changes needs clarification #7848

Closed

thefourtheye mentioned this pull request Jul 23, 2016

fix chmod function arguments isaacs/node-graceful-fs#71

Merged

Trott mentioned this pull request Jul 23, 2016

repl: deprecate unused function convertToContext #7829

Closed

2 tasks

MylesBorins mentioned this pull request Jul 25, 2016

[WIP] Update test-npm target #7867

Closed

4 tasks

jasnell added the ctc-agenda label Jul 25, 2016

Trott mentioned this pull request Jul 26, 2016

Node.js Foundation Core Technical Committee (CTC) Meeting 2016-07-27 #7881

Closed

thefourtheye added a commit to thefourtheye/npm that referenced this pull request Jul 27, 2016

[email protected]

45164cc

graceful-fs had a bug fix, isaacs/node-graceful-fs#71, which would fix the problem in Node.js nodejs/node#7846

thefourtheye mentioned this pull request Jul 27, 2016

[email protected] npm/npm#13497

Closed

thefourtheye mentioned this pull request Jul 27, 2016

fs: warn instead of throwing if no callback #7897

Closed

2 tasks

bzoz mentioned this pull request Jul 27, 2016

fs: restore javascript realpath implementation #7899

Closed

2 tasks

MylesBorins force-pushed the revert-fs-changes branch from 436a055 to 94e7af6 Compare August 4, 2016 16:38

MylesBorins closed this Aug 4, 2016

MylesBorins force-pushed the revert-fs-changes branch from 94e7af6 to c5a18e7 Compare August 4, 2016 16:44

MylesBorins reopened this Aug 4, 2016

addaleax closed this Aug 4, 2016

cjihrig mentioned this pull request Aug 8, 2016

v6.4.0 Release Proposal #8020

Closed

cjihrig added the dont-land-on-v6.x label Aug 10, 2016

iarna added a commit to npm/npm that referenced this pull request Aug 11, 2016

[email protected]

51d49d2

Fix bug in chmod wrapper that was tickled by recent node.js changes. (nodejs/node#7846) PR-URL: isaacs/node-graceful-fs#71 Credit: @thefourtheye Reviewed-By: @isaacs

iarna pushed a commit to npm/npm that referenced this pull request Aug 11, 2016

[email protected]

7e0cdff

graceful-fs had a bug fix, isaacs/node-graceful-fs#71, which would fix the problem in Node.js nodejs/node#7846 PR-URL: #13497 Credit: @thefourtheye Reviewed-By: @iarna

zkat mentioned this pull request Aug 12, 2016

cb is not a function error npm/npm#13457

Closed

13 tasks

zkat mentioned this pull request Sep 22, 2016

deps: upgrade npm to 3.10.8 #8706

Closed

4 tasks

MylesBorins added the dont-land-on-v4.x label Sep 30, 2016

Trott removed the ctc-agenda label Oct 20, 2016

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins deleted the revert-fs-changes branch November 14, 2017 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert fs changes #7846

Revert fs changes #7846

MylesBorins commented Jul 22, 2016 •

edited

Loading

MylesBorins commented Jul 22, 2016

trevnorris commented Jul 24, 2016

ChALkeR commented Jul 24, 2016 •

edited

Loading

jasnell commented Jul 25, 2016

MylesBorins commented Jul 26, 2016

ChALkeR commented Jul 26, 2016

jasnell commented Jul 26, 2016

evanlucas commented Jul 26, 2016

MylesBorins commented Jul 26, 2016

bnoordhuis commented Jul 27, 2016

ChALkeR commented Jul 27, 2016 •

edited

Loading

ChALkeR commented Jul 27, 2016

jasnell commented Jul 27, 2016

MylesBorins commented Jul 27, 2016

isaacs commented Jul 28, 2016

addaleax commented Aug 3, 2016

jasnell commented Aug 3, 2016

MylesBorins commented Aug 4, 2016

MylesBorins commented Aug 4, 2016

addaleax commented Aug 4, 2016

jasnell commented Aug 4, 2016

addaleax commented Aug 4, 2016

cjihrig commented Aug 10, 2016

jasnell commented Aug 10, 2016

cjihrig commented Aug 10, 2016

jasnell commented Aug 10, 2016

cjihrig commented Aug 10, 2016

Revert fs changes #7846

Revert fs changes #7846

Conversation

MylesBorins commented Jul 22, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

MylesBorins commented Jul 22, 2016

trevnorris commented Jul 24, 2016

ChALkeR commented Jul 24, 2016 • edited Loading

jasnell commented Jul 25, 2016

MylesBorins commented Jul 26, 2016

ChALkeR commented Jul 26, 2016

jasnell commented Jul 26, 2016

evanlucas commented Jul 26, 2016

MylesBorins commented Jul 26, 2016

bnoordhuis commented Jul 27, 2016

ChALkeR commented Jul 27, 2016 • edited Loading

ChALkeR commented Jul 27, 2016

jasnell commented Jul 27, 2016

MylesBorins commented Jul 27, 2016

isaacs commented Jul 28, 2016

addaleax commented Aug 3, 2016

jasnell commented Aug 3, 2016

MylesBorins commented Aug 4, 2016

MylesBorins commented Aug 4, 2016

addaleax commented Aug 4, 2016

jasnell commented Aug 4, 2016

addaleax commented Aug 4, 2016

cjihrig commented Aug 10, 2016

jasnell commented Aug 10, 2016

cjihrig commented Aug 10, 2016

jasnell commented Aug 10, 2016

cjihrig commented Aug 10, 2016

MylesBorins commented Jul 22, 2016 •

edited

Loading

ChALkeR commented Jul 24, 2016 •

edited

Loading

ChALkeR commented Jul 27, 2016 •

edited

Loading