fix(pnp): Avoid infinite loop in findApiPathFor #1298
Conversation
|
Oh interesting! I think a better fix might be to call |
|
yeah, just saw your message, took a while to clone :) Will do a test. |
|
BTW when done what should I pick for |
* Add failing test
* Fix infinite loop
This is definitely a corner case. The error only manifests when
`findApiPathFor` is called with a `paths` option that includes the empty
string when the CWD does not contain a `.pnp.js`. The way that the
directory tree is to use `path.dirname`, with the root (`/`) as the
terminating case. `path.dirname('')` returns `'.'` as does
`path.dirname('.')`, so the loop never ends.
The fix is to resolve the path before trying to walk the tree.
* Bump dependent versions
keithlayne
changed the title
|
@arcanis Apparently this fix doesn't work on windows. Plus, the integration tests seem to be timing out correctly - when you run them locally (before the fix) they time out and jest exits. In CI it seems like (from the cancelled windows jobs) that everything runs fine, fails with a timeout, and then never terminates. I thought it was fairly safe to trigger the infinite loop in Jest because of it's timeout stuff. Anyway, this fix doesn't seem to work on windows, and I don't know of an easy way for me to debug that. Logically though, |
|
@arcanis I think maybe the right thing to do here is: let next = npath.toPortablePath(npath.resolve(modulePath));i.e. resolve it natively before converting to a posix path. I can't imagine what's going on in windows (can't test) but can only assume things are acting differently there with the empty string. |
|
I'm like 99% sure this is because
It's faster when contributors bump the versions, this way I often don't have to checkout the branch locally 😄 Note that in this case we don't need to bump aaaall the plugins, just the affected ones + the CLI (so that a new CLI bundle gets generated). Cf my fix. |
|
booyah. Sorry I didn't save you any time on this. But yeah, I was starting to think that the solution should have been at a higher level. Thanks for figuring that out! |
|
Well, the bug was a quite devilish one! Thanks for digging in 🛠 |
The `findNpmPackage` function in `runtime-info.ts` has a subtle and mostly innocuous bug. Its parent, `collectRuntimeInformation`, iterates over the modules in `require.cache` and resolves the library version for each. It uses the `paths` property of each entry to locate the package description for that module. An example: For `/foo/bar/aws-cdk/packages/aws-cdk/lib/index.js`: ```js [ '/foo/aws-cdk/packages/aws-cdk/lib/node_modules', '/foo/aws-cdk/packages/aws-cdk/node_modules', '/foo/aws-cdk/packages/node_modules', '/foo/aws-cdk/node_modules', '/foo/node_modules', '/node_modules' ] ``` `findNpmPackage` maps over these, and if the string ends with `/node_modules` or `\\node_modules` it strips that off, ending with: ```js [ '/foo/aws-cdk/packages/aws-cdk/lib', '/foo/aws-cdk/packages/aws-cdk', '/foo/aws-cdk/packages', '/foo/aws-cdk', '/foo', '' ] ``` ...and passes this array as the `paths` option to `require.resolve`. The problem is that `''` is interpreted as the current working directory, and instead of walking the file tree to the root (as was probably intended) it appends whatever node's current working directory is to the search. It's hard to imagine this ever really having a negative impact on package resolution, but unfortunately this triggered a likewise obscure bug in yarn 2 when configured with PnP. (See yarnpkg/berry#1298). This bug is fixed in yarn 2 master but hasn't been released yet. The effect of these two bugs when triggered is for the CDK CLI to hang. This change seems to meet the intent of the code without having the above problem. It seems canonically correct and uses the `fs` lib more idiomatically. It's also less code :) This passes all existing tests. It's not clear how this would be tested without exporting `findNpmPackage` and adding tests for it. **CAVEAT:** I can't test this on windows, but it's worth noting the following: ```js // Node 12.16.3 path.win32.resolve('C:') // => 'C:\\foo\\aws-cdk' (current working directory) ``` ```js // Node 10.13.0 path.win32.resolve('C:') // => 'C:\\' ``` Both of these results come on OSX. But it appears that the yarn bug would not trigger on either version, while node may or may not have the same wrong behavior for `findNpmVersion` on windows.
The `findNpmPackage` function in `runtime-info.ts` has a subtle and mostly innocuous bug. Its parent, `collectRuntimeInformation`, iterates over the modules in `require.cache` and resolves the library version for each. It uses the `paths` property of each entry to locate the package description for that module. An example: For `/foo/bar/aws-cdk/packages/aws-cdk/lib/index.js`: ```js [ '/foo/aws-cdk/packages/aws-cdk/lib/node_modules', '/foo/aws-cdk/packages/aws-cdk/node_modules', '/foo/aws-cdk/packages/node_modules', '/foo/aws-cdk/node_modules', '/foo/node_modules', '/node_modules' ] ``` `findNpmPackage` maps over these, and if the string ends with `/node_modules` or `\\node_modules` it strips that off, ending with: ```js [ '/foo/aws-cdk/packages/aws-cdk/lib', '/foo/aws-cdk/packages/aws-cdk', '/foo/aws-cdk/packages', '/foo/aws-cdk', '/foo', '' ] ``` ...and passes this array as the `paths` option to `require.resolve`. The problem is that `''` is interpreted as the current working directory, and instead of walking the file tree to the root (as was probably intended) it appends whatever node's current working directory is to the search. It's hard to imagine this ever really having a negative impact on package resolution, but unfortunately this triggered a likewise obscure bug in yarn 2 when configured with PnP. (See yarnpkg/berry#1298). This bug is fixed in yarn 2 master but hasn't been released yet. The effect of these two bugs when triggered is for the CDK CLI to hang. This change seems to meet the intent of the code without having the above problem. It seems canonically correct and uses the `fs` lib more idiomatically. It's also less code :) This passes all existing tests. It's not clear how this would be tested without exporting `findNpmPackage` and adding tests for it. **CAVEAT:** I can't test this on windows, but it's worth noting the following: ```js // Node 12.16.3 path.win32.resolve('C:') // => 'C:\\foo\\aws-cdk' (current working directory) ``` ```js // Node 10.13.0 path.win32.resolve('C:') // => 'C:\\' ``` Both of these results come on OSX. But it appears that the yarn bug would not trigger on either version, while node may or may not have the same wrong behavior for `findNpmVersion` on windows.
What's the problem this PR addresses?
This is definitely a corner case. The error only manifests when
findApiPathForis called with apathsoption that includes the emptystring when the CWD does not contain a
.pnp.js. The way that thedirectory tree is iterated is to use
path.dirname, with the root (/) asthe terminating case.
path.dirname('')returns'.'as doespath.dirname('.'), so the loop never ends.How did you fix it?
The fix is to resolve the path before trying to walk the tree.