circular file: dependencies cause `npm install` to hang

[diachedelic]

What / Why

I suspect this is an issue which is causing npm pack to hang, so I created a minimal reproduction and found it also affects npm install.

When

When running npm install on a package structure which uses circular file: dependencies, npm install enters an infinite loop.

// package.json
{
  "name": "program",
  "version": "1.0.0",
  "dependencies": {
    "foo": "../foo"
  }
}

// ../foo/package.json
{
  "name": "foo",
  "version": "1.0.0",
  "dependencies": {
    "bar": "../bar"
  }
}

// ../bar/package.json
{
  "name": "bar",
  "version": "1.0.0",
  "dependencies": {
    "foo": "../foo"
  }
}

$ npm install --loglevel silly
npm info it worked if it ends with ok
npm verb cli [
npm verb cli   '/Users/me/.nvm/versions/node/v14.4.0/bin/node',
npm verb cli   '/Users/me/.nvm/versions/node/v14.4.0/bin/npm',
npm verb cli   'install',
npm verb cli   '--loglevel',
npm verb cli   'silly'
npm verb cli ]
npm info using npm@6.14.7
npm info using node@v14.4.0
npm verb npm-session 94ac1df1bcf60230
npm sill install runPreinstallTopLevelLifecycles
npm sill preinstall program@1.0.0
npm info lifecycle program@1.0.0~preinstall: program@1.0.0
npm sill install loadCurrentTree
npm sill install readLocalPackageData
npm timing stage:loadCurrentTree Completed in 9ms
npm sill install loadIdealTree
npm sill install cloneCurrentTreeToIdealTree
npm timing stage:loadIdealTree:cloneCurrentTree Completed in 0ms
npm sill install loadShrinkwrap
npm timing stage:loadIdealTree:loadShrinkwrap Completed in 3ms
npm sill install loadAllDepsIntoIdealTree
npm sill pacote directory manifest for foo@file:../foo fetched in 9ms
npm sill resolveWithNewModule foo@1.0.0 checking installable status
npm sill pacote directory manifest for bar@file:../bar fetched in 1ms
npm sill resolveWithNewModule bar@1.0.0 checking installable status
npm sill pacote directory manifest for foo@file:../foo fetched in 1ms
npm sill resolveWithNewModule foo@1.0.0 checking installable status
npm sill pacote directory manifest for bar@file:../bar fetched in 1ms
npm sill resolveWithNewModule bar@1.0.0 checking installable status
npm sill pacote directory manifest for foo@file:../foo fetched in 1ms
npm sill resolveWithNewModule foo@1.0.0 checking installable status
npm sill pacote directory manifest for bar@file:../bar fetched in 0ms
npm sill resolveWithNewModule bar@1.0.0 checking installable status
...

Where

Local machine

Steps to Reproduce

git clone git@github.com:diachedelic/npm-hang-repro.git && \
  cd npm-hang-repro/program && \
  npm install --loglevel silly

[darcyclarke]

npm v6 is no longer in active development; We will continue to push security releases to v6 at our team's discretion as-per our Support Policy.

If your bug is preproducible on v7, please re-file this issue using our new issue template.

If your issue was a feature request, please consider opening a new RRFC or RFC. If your issue was a question or other idea that was not CLI-specific, consider opening a discussion on our feedback repo

Closing: This is an automated message.

[diachedelic]

It's fixed on NPM v7, thanks.

[mmomtchev]

In fact, this is a very stupid bug, there is a cyclic dependency check in npm, but someone has mistakenly used Object.create instead of Object.assign...

cli/lib/install/is-extraneous.js

Line 24 in 6996eca

return isNotExtraneous(node, Object.create(isCycle))

Anyway it is too late now, even if there is a new release, no one will replace existing npm versions - and this is the real problem - cyclic packages can't get installed on platforms where npm hasn't been updated...

[ljharb]

That’s not a mistake; Object.assign with only one argument wouldn’t do anything, and Object.create is correctly used there to “copy” the isCycle object.

[mmomtchev]

> Object.create({a: 1})
{}
> Object.create({a: 1}).__proto__
{ a: 1 }
> 

Object.assign is what you need - obviously with the right arguments.

[mmomtchev]

No, in fact, it creates a static property:

> Object.create({a: 1}).a
1

[ljharb]

That is exactly what that line of code is meant to do - it creates an object that inherits from inCycle, so the new object can be safely mutated.

[mmomtchev]

@ljharb, yes you are correct, I thought that it was losing fields after two iterations - it simply creates huge __proto__ chains. This is not an infinite loop, it is a n! loop - which in the case of @stdlib with its 13 packages is a lot. This explains why we haven't seen it more often - it requires more than a simple cyclic dependency - it requires a cyclic dependency with lots of packages.