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

npm throws an error on master #1898

Closed
evanlucas opened this issue Jun 5, 2015 · 17 comments
Closed

npm throws an error on master #1898

evanlucas opened this issue Jun 5, 2015 · 17 comments
Labels
npm Issues and PRs related to the npm client dependency or the npm registry.

Comments

@evanlucas
Copy link
Contributor

Looks like a regression introduced in 6d95f4f.

$ npm
module.js:334
    throw err;
          ^
Error: Cannot find module 'internal/smalloc'
    at Function.Module._resolveFilename (module.js:332:15)
    at Function.Module._load (module.js:282:25)
    at Module.require (module.js:361:17)
    at require (module.js:380:17)
    at evalmachine.<anonymous>:22:20
    at Object.<anonymous> (/usr/local/lib/node_modules/npm/node_modules/graceful-fs/fs.js:11:1)
    at Module._compile (module.js:426:26)
    at Object.Module._extensions..js (module.js:444:10)
    at Module.load (module.js:351:32)
    at Function.Module._load (module.js:306:12)

/cc @vkurchatkin

@brendanashworth brendanashworth added npm Issues and PRs related to the npm client dependency or the npm registry. smalloc labels Jun 5, 2015
@targos
Copy link
Member

targos commented Jun 5, 2015

It is caused by this change in fs.js.

It breaks graceful-fs which is evaluating the code from fs in a context where internal modules are not allowed.

@targos
Copy link
Member

targos commented Jun 5, 2015

/cc @isaacs

@vkurchatkin
Copy link
Contributor

// eeeeeevvvvviiiiiiillllll
// more evil than monkey-patching the native builtin?
// Not sure

Well, now we know the answer

vkurchatkin added a commit to vkurchatkin/node that referenced this issue Jun 5, 2015
This allows userland modules to evaluate `fs` source
without access to internals.

Fixes: nodejs#1898
@thefourtheye
Copy link
Contributor

I wonder why this was not caught when I ran make test with this change. This is not covered in testcases?

@bnoordhuis
Copy link
Member

@thefourtheye make test doesn't run make test-npm (yet? there was talk about that recently.)

@thefourtheye
Copy link
Contributor

@bnoordhuis But, requiring fs in other modules should have failed, right?

@bnoordhuis
Copy link
Member

When you say 'other modules', do you mean 'core modules'? Those have access to internal modules so no problem there. require('fs') in user code isn't affected either.

The npm breakage is because graceful-fs does eval(process.binding('natives').fs). It's completely unsupported and normally I'd say 'tough luck'. But a broken npm is bad so we'll have to add a workaround until graceful-fs gets fixed.

vkurchatkin added a commit to vkurchatkin/node that referenced this issue Jun 5, 2015
This allows `graceful-fs` to evaluate `fs` source
without access to internals.

See isaacs/node-graceful-fs#41

This is a temporary workaround that makes npm work.

Fixes: nodejs#1898
vkurchatkin added a commit to vkurchatkin/node that referenced this issue Jun 5, 2015
This allows `graceful-fs` to evaluate `fs` source
without access to internals.

This is a temporary workaround that makes npm work.

See: isaacs/node-graceful-fs#41
Fixes: nodejs#1898
vkurchatkin added a commit that referenced this issue Jun 5, 2015
This allows `graceful-fs` to evaluate `fs` source
without access to internals.

This is a temporary workaround that makes npm work.

See: isaacs/node-graceful-fs#41
Fixes: #1898
PR-URL: #1903
Reviewed-By: Ben Noordhuis <[email protected]>
@vkurchatkin
Copy link
Contributor

Fixed in 2dcef83, though this should be fixed in graceful-fs at some point. Until then we can't use internal modules in fs

@thefourtheye
Copy link
Contributor

@bnoordhuis Thanks for clarifying :)

@silverwind
Copy link
Contributor

we can't use internal modules in fs

Thats indeed not optimal. Were there ever attempts in providing graceful-fs in core directly, or is it not suited?

@isaacs
Copy link
Contributor

isaacs commented Jun 5, 2015

@silverwind There's been talk about having graceful-fs in core. Generally, the vote has come down to a "no" (and I've voted "no" as well).

Some of the features in graceful-fs are totally fine, imo, and do belong in core. There's no reason why fs.lchmod or fs.lchown should be missing. If a platform doesn't treat symlinks and their targets as having separate distinct permissions and owners (ie, linux) then it should still let you go ahead and call fs.lchmod and have it just be identical to fs.chmod on the symlink. For systems that do support it, it should work as expected.

The biggest feature in graceful-fs is that it detects and avoids EMFILE errors that occur when you open too many files at once. When using the fs module directly, the fs.open call will simply fail, and it's up to you to handle the error. With graceful-fs, open calls that fail with EMFILE are placed in a queue, and re-attempted when some other fd is closed.

This is important to programs like npm that are in the business of unpacking a lot of tarballs and reading a lot of files, and want to parallelize to the maximum amount allowed by the operating system.

However, there are situations where this behavior would be surprising, or even trigger problematic deadlocks! Rather than have an immediate failure from an fs.open call, you now have an indefinitely long lag.

Putting that behavior in core is a bad idea. However, this highlights the point that we do need a better way for this to work.

Monkey-patching the core module caused a lot of problems because builtin modules are globals, so the change 8 levels deep in the tree could have strange impacts on consumers. (isaacs/node-graceful-fs#28)

I don't have a great solution in mind.

@Fishrock123
Copy link
Contributor

Maybe we could have a --queue-at-max-fd sort of option? It seems like we may be able to get the best of both worlds there. I can imagine a lot of programs that would rather not have errors for that, and other ones that would.

I'm not sure how npm would set something like that though...

@isaacs
Copy link
Contributor

isaacs commented Jun 5, 2015

@Fishrock123 Really, what you want is a fs.open that queues until a close if EMFILE is raised, and another fs.open that merely keeps track of how many open FDs there are. The reason why graceful-fs had to switch from a monkey-patch of require('fs') to a clone of it was precisely because you don't want this happening at the global level.

But, then you need to make sure that fs.readdir handles EMFILE and tracks fd counts as well, and that all the calls to open from fs.readFile and fs.writeFile and the stream classes all do the right thing.

One approach would be for it to lock the code down at a particular version of process.binding('natives').fs, but then if the internals change, there's a higher likelihood of other more subtle errors cropping up.

Maybe we ought to split fs out into a separate module entirely, and vendor it into node core?

@silverwind
Copy link
Contributor

Wow, this discussion is a bit over my head, but my prime reason for using graceful-fs is the EACCESS/EPERM handling on Windows, related to locked files by other programs which I recall were randomly happening even without using any AV software present and which were breaking my programs in all sort of unexpected ways.

It surprises me a bit that we don't get any bug reports in core regarding Windows file locking, maybe the issue isn't as widespread as I thought it is. Anyways, I'd probably be happy if we can get this feature alone into core, if it has no drawbacks (which it probably has).

@bnoordhuis
Copy link
Member

Anyways, I'd probably be happy if we can get this feature alone into core, if it has no drawbacks (which it probably has).

Just the first thing that pops into my mind: an application that silently stalls because of a deferred file operation will be much, much harder to debug than one that simply throws an error.

On a philosophical level, graceful-fs-like behavior in core is way too much policy in the "mechanism, not policy" sense.

Maybe we ought to split fs out into a separate module entirely, and vendor it into node core?

Please God, no.

@isaacs
Copy link
Contributor

isaacs commented Jun 6, 2015

Just the first thing that pops into my mind: an application that silently stalls because of a deferred file operation will be much, much harder to debug than one that simply throws an error.

Well, you need to be sure that when this application opens files, it will close them promptly when it's done with them.

Though, the same could be said of getting EMFILE or ENFILE errors on sockets, and we handle those gracefully. Why do it there and not for files?

@bnoordhuis
Copy link
Member

Though, the same could be said of getting EMFILE or ENFILE errors on sockets, and we handle those gracefully.

I'm not sure what you mean. The fd-stashed-away-for-hard-times trick? That's really just a hack to avoid busy looping with level-triggered listen sockets. Comparing that with graceful-fs is giving it way too much credit. :-)

ChALkeR added a commit to ChALkeR/io.js that referenced this issue 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 issue 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 issue 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
npm Issues and PRs related to the npm client dependency or the npm registry.
Projects
None yet
Development

No branches or pull requests

9 participants