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

test: regression for fs module monkey-patching #2026

Closed

Conversation

thefourtheye
Copy link
Contributor

The #2025 breaks graceful-fs module. To catch the accidental breaking like this, this PR introduces a test to make sure that the native fs module is monkey-patchable.

@mscdex mscdex added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. labels Jun 21, 2015
@Fishrock123
Copy link
Contributor

I am really not sure we should add this terrible hack as a test.

@thefourtheye
Copy link
Contributor Author

@Fishrock123 I think we can avoid surprises like #1898, if we have this test, no?

@brendanashworth
Copy link
Contributor

It really is horrible, but we've already decided to support graceful-fs's horrible way of hacking core 😞, so +1 on adding the test.

@trevnorris
Copy link
Contributor

Not that it's and better, but pretty sure the same functionality can be achieved from the process bindings.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 21, 2015

I would recommend against merging this testcase. At least not until #1941 is closed.

The whole situation around graceful-fs is just wrong, there should be a legit (supported in core) way of doing what graceful-fs does, without hacks.

Monkey-patching core and natives-based magic should not be treated as supported, it will never be: it's not possible to support monkey-patching without freezing everything, and supporting the natives magic that graceful-fs currently uses would require to throw out internal/* completely. Supporting that just for fs module is inconsistent.

Once this is declared as supported (which one could assume it would be after merging a dedicated testcase) it would be harder to get this out and solved properly.

@trevnorris
Copy link
Contributor

Agreed. Any short term gain here would be a long term maintenance loss. If this is a feature we should/want to support then let's create a proper API for it.

Testing to make sure that the native `fs` module is monkey-patchable,
as few major libraries like `graceful-fs` rely on this behaviour.
@vkurchatkin
Copy link
Contributor

This is a temporary thing. We don't want to continue breaking graceful-fs again and again. It doesn't mean that this hack is actually supported.

@bnoordhuis
Copy link
Member

we've already decided to support graceful-fs's horrible way of hacking core

We did? What memo did I miss?

@Fishrock123
Copy link
Contributor

we've already decided to support graceful-fs's horrible way of hacking core

We did? What memo did I miss?

I wouldn't use the word support for this.. I can't imagine we would officially support that.

It's more like "ok people use it so we'll delay breaking it".

(That being said I'm still wondering if we couldn't inline internal modules into code returned by process.binding)

@trevnorris
Copy link
Contributor

At what point will the line be drawn to not care that a module has blatantly abused the internal API? It'll get tiresome if every popular module can require us to roll back a change to unsupported API.

@Fishrock123
Copy link
Contributor

 At what point will the line be drawn to not care that a module has blatantly abused the internal API? It'll get tiresome if every popular module can require us to roll back a change to unsupported API.

I'm not sure, maybe this is a case study in that?

This is blatant abuse, but:

  1. Functionality cannot be achieved without hacks properly in core with core
  2. Broad use because functionality is useful

I think #2 is pretty important also. If this sort of thing is used by less than some amount of people, we should just suggest an older version until #1 is fixed.

@trevnorris
Copy link
Contributor

Those are legitimate points, and for these cases we should propose an officially supported API and write tests for that. My main concern going down this "temporary" path is that afterwards it's likely to just be forgotten. Further solidifying the use of the internal API. There's at least one case in core where this has happened.

As long as an alternative and supported API is added, I don't see the need for a depreciation cycle. Even if it's used, it's still internal API. Anyone who uses it knows what that means. At least they'll have a way to update their code.

@vkurchatkin
Copy link
Contributor

Once again, this is a temporary thing to avoid breakage. Imagine the following workflow:

  • someone includes require('internal/') in fs;
  • runs tests;
  • sees this test failing;
  • reads comments, Github issues, etc;
  • removes require('internal/');

Once graceful-fs is fixed it would be clear that this test is not relevant anymore and should be deleted.

At what point will the line be drawn to not care that a module has blatantly abused the internal API?

Apparently, never, as it "displays a lack of empathy that is incompetence bordering on malice" according to @isaacs.

@isaacs
Copy link
Contributor

isaacs commented Jun 21, 2015

If we are going to make this a viable platform for real world users, then that means not breaking large swaths of user code, whether they're "blatantly abusing internal APIs" or not. What is exposed is external, regardless of what the docs say.

If you want this to be a project for experimentation and making a pretty thing for engineers to adore, then fine, but let's have the TSC formally state that that's the purpose of io.js and it's not intended for production or stability.

"Empathy" isn't about being a nice person. It's about acknowledging the reality of your users' experience. A platform without stability isn't a good platform that people will use, and software without users is a waste.

Give me a better way to build graceful-fs, and I'll gladly take advantage of it. Until then, literally 100% of your users depend on npm continuing to work properly, and ignoring that fact is a failure of platform engineering.

@isaacs
Copy link
Contributor

isaacs commented Jun 21, 2015

And yes, it is tiresome. Welcome to the challenges of maintaining a mature open source software platform! The only alternative is obscurity.

@bnoordhuis
Copy link
Member

Functionality cannot be achieved properly

I've seen this mentioned a few times but without explanations why. What's so special about the core fs module that graceful-fs can't shim it without eval hacks?

@trevnorris
Copy link
Contributor

What is exposed is external, regardless of what the docs say.

You know that's a non-applicable argument. JS doesn't have the concept of private members. Even if it's hidden behind a Symbol, it's still possible to retrieve and override. So we depend on conventions. Which you have no trouble violating.

If you want this to be a project for experimentation and making a pretty thing for engineers to adore, then fine, but let's have the TSC formally state that that's the purpose of io.js and it's not intended for production or stability.

Comments such as this are not applicable to the discussion and are borderline harassment. Leave them out.

Give me a better way to build graceful-fs, and I'll gladly take advantage of it.

This is exactly as I suggested. Though it seems you are too preoccupied proving a point than having a discussion.

Show me a ticket that requested this functionality be exposed and I will be sympathetic. Otherwise I have no concern for the developer who lazily uses clearly internal methods and expects them to not break in the future.

@vkurchatkin
Copy link
Contributor

In this case it's not actually about private APIs. The only private API used is process.binding and the code can be easily rewritten without it:

var pre = '(function (exports, require, module, __filename, __dirname) { ';
var post = '});';
var src = pre + require('child_process').execSync('curl https://raw.githubusercontent.com/nodejs/io.js/' + process.version + '/lib/fs.js', { encoding: 'utf8' }) + post;
var vm = require('vm');
var fn = vm.runInThisContext(src);
fn(exports, require, module, __filename, __dirname);

Does exactly the same without any private APIs! But this is just crazy. And actual implementation is not much better.

@Fishrock123
Copy link
Contributor

Those are legitimate points, and for these cases we should propose an officially supported API and write tests for that. My main concern going down this "temporary" path is that afterwards it's likely to just be forgotten. Further solidifying the use of the internal API. There's at least one case in core where this has happened.

As long as an alternative and supported API is added, I don't see the need for a depreciation cycle. Even if it's used, it's still internal API. Anyone who uses it knows what that means. At least they'll have a way to update their code.

Agreed, we'd re-implement private modules into fs as soon as this hits. (Might mean floating a patch for deps/npm/(..)/graceful-fs until the updates bubble up.)

"Empathy" isn't about being a nice person. It's about acknowledging the reality of your users' experience. A platform without stability isn't a good platform that people will use, and software without users is a waste.

Give me a better way to build graceful-fs, and I'll gladly take advantage of it. Until then, literally 100% of your users depend on npm continuing to work properly, and ignoring that fact is a failure of platform engineering.

Dropping the top bit, that is what we are attempting to do. Could we please stop firing shots about this?

@ChALkeR
Copy link
Member

ChALkeR commented Jun 21, 2015

@vkurchatkin I hope that code is a joke =).

@trevnorris
Copy link
Contributor

@ChALkeR
Copy link
Member

ChALkeR commented Jun 21, 2015

@trevnorris I saw that. That is bad, but the curl thing above is far worse =).

@ChALkeR
Copy link
Member

ChALkeR commented Jun 21, 2015

I want to link this here, it's loosely related: npm/npm#8619 (comment)

@isaacs
Copy link
Contributor

isaacs commented Jun 21, 2015

I've seen this mentioned a few times but without explanations why. What's so special about the core fs module that graceful-fs can't shim it without eval hacks?

Try it, and you'll see. File descriptors have to be tracked in a variety of places, and so this means re-implementing fs.ReadStream, fs.readdir, et al, in their entirety. Discussed in more detail in #1941, so I'd rather not rehash it here. See also the entire commit history of https://github.com/isaacs/node-graceful-fs. There have been ample explanations, and I'm not willing to repeat them all on demand.

Since this is functionality that people currently rely on ("correctly" or "lazily" or not), and you want to shift around internals to make the current approach unworkable, either figure out a way to deliver this functionality that allows you to make whatever internal changes you like, without breaking existing userland code. Or decide that user success isn't as important to this project as correctness.

Dropping the top bit, that is what we are attempting to do. Could we please stop firing shots about this?

@Fishrock123 Not everyone is attempting to do that. Thanks for the support, though.

I'm feeling rather exhausted continually trying to make the case for stability. Until there's a clearly defined set of priorities for this project, I'm not comfortable continuing to engage. It's stressful and unproductive.

Send me a PR to make graceful-fs work on io.js, and I'll probably accept it. Or don't.

@bnoordhuis
Copy link
Member

File descriptors have to be tracked in a variety of places, and so this means re-implementing fs.ReadStream, fs.readdir, et al, in their entirety. Discussed in more detail in #1941, so I'd rather not rehash it here. [...]

I've read #1941 but I didn't really see where it makes the case for the current graceful-fs implementation, except that any other approach is a lot of work?

@trevnorris
Copy link
Contributor

figure out a way to deliver this functionality that allows you to make whatever internal changes you like

That is completely backwards. We don't need the functionality publicly exposed. You do. So instead of spending so much energy ranting why not open an issue, or probably better a PR, where actual technical discussion can happen.

Or decide that user success isn't as important to this project as correctness.

This type of aggressive rhetoric is unacceptable. Stop with this or leave. It will no longer be tolerated.

I'm feeling rather exhausted continually trying to make the case for stability.

You aren't, but instead fighting for something that has directly affected you. Don't pretend to be a savior for the community. You're belligerent attitude concerning this matter has been in complete contradiction to what you say you're fighting for.

Be like every other developer on this project and create a ticket with a concise explanation of the issue (the one already opened does not meet this criteria) and preferably alternative implementation details that would be helpful.

It is impossible to hide everything in JS. You know that. So we must follow conventions. You broke those conventions and now are expecting us to clean up.

brendanashworth added a commit to brendanashworth/node-graceful-fs that referenced this pull request Jun 21, 2015
Fix the horrible atrocity introduced in
08471b2. Previously it would evaluate
the code directly through process.binding(), and it now creates a new
Object with the prototype being the real fs module, exported by core.

Ref: nodejs/node#2026
@mikeal
Copy link
Contributor

mikeal commented Jun 23, 2015

@trevnorris also, I just realized that this statement in the Stability Policy is from our existing io.js roadmap https://github.com/nodejs/io.js/blob/master/ROADMAP.md#stability-policy which was discussed and ratified all the way back in February https://github.com/nodejs/io.js/blob/master/doc/tc-meetings/2015-02-18.md

@trevnorris
Copy link
Contributor

@mikeal I recall the Stability Policy defined in the roadmap. Which largely deals with native and V8 changes.

As for the vote to joining the foundation, I must have been too involved in the TSC Charter document at the time. Also since the link in that issue pointed to a document that states it is still a work in progress, didn't think too much of it. Thanks for pointing me to the additional information.

As for sending a PR, I'd first like to have a short discussion with the TSC (already opened an issue just for the tsc agenda) and get their thoughts on the matter. Partially because I don't know how the other members feel, and because I suck at writing compared to some other devs like yourself and don't want to accidentally convey the wrong message.

@mikeal
Copy link
Contributor

mikeal commented Jun 23, 2015

As for sending a PR, I'd first like to have a short discussion with the TSC (already opened an issue just for the tsc agenda) and get their thoughts on the matter. Partially because I don't know how the other members feel, and because I suck at writing compared to some other devs like yourself and don't want to accidentally convey the wrong message.

Cool, I guess we'll discuss tomorrow :)

@Fishrock123
Copy link
Contributor

@bnoordhuis's re-write has landed in graceful-fs. Closing.

@SimenB
Copy link
Member

SimenB commented Jun 28, 2015

It was reverted, wasn't it?

isaacs/node-graceful-fs#45 (comment)

@ChALkeR
Copy link
Member

ChALkeR commented Jun 28, 2015

@SimenB No, it is merged in graceful-fs 4.x along with a patch that fixes the 0.8 support.

@isaacs
Copy link
Contributor

isaacs commented Jun 29, 2015

graceful-fs is now passing tests on latest io.js, and Node >= 0.8

@rvagg
Copy link
Member

rvagg commented Jun 29, 2015

sweet, good work @bnoordhuis et. al.

@thefourtheye thefourtheye deleted the fs-monkey-patch-regression branch July 21, 2015 16:23
jbenet added a commit to jbenet/gulp-sourcemaps that referenced this pull request Aug 14, 2015
ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Feb 11, 2016
This is needed to give users a grace period before actually breaking
modules that re-evaluate fs sources from context where internal modules
are not allowed, e.g. older version of graceful-fs module.

To be reverted in Node.js 7.0.

Fixes nodejs#5097, see also nodejs#1898, nodejs#2026, and nodejs#4525.
jasnell pushed a commit that referenced this pull request Feb 13, 2016
This is needed to give users a grace period before actually breaking
modules that re-evaluate fs sources from context where internal modules
are not allowed, e.g. older version of graceful-fs module.

To be reverted in Node.js 7.0

Fixes: #5097, see also #1898, #2026, and #4525.
PR-URL: #5102
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
stefanmb pushed a commit to stefanmb/node that referenced this pull request Feb 23, 2016
This is needed to give users a grace period before actually breaking
modules that re-evaluate fs sources from context where internal modules
are not allowed, e.g. older version of graceful-fs module.

To be reverted in Node.js 7.0

Fixes: nodejs#5097, see also nodejs#1898, nodejs#2026, and nodejs#4525.
PR-URL: nodejs#5102
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.