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

fix(arborist) _findMissingEdges missing dependency due to inconsistent path separators #4261

Merged
merged 3 commits into from
Mar 15, 2022

Conversation

fritzy
Copy link
Contributor

@fritzy fritzy commented Jan 19, 2022

THIS PR HAS BEEN MIGRATED

Originally at npm/arborist#363
Originally by @salvadorj

When installing a package with bundled dependencies with npm it can end up missing some dependencies that were bundled and attempt to fetch from the remote registry (see #2757). This issue only happens on Windows and it's caused by _findMissingEdges missing one of the bundled dependencies. The reason it only happens on Windows is because of this lookup in this[_cache] with a path that contains mixed path separators:
https://github.com/npm/arborist/blob/02f295f2d0caa6de2d3235c6eb1c180a8afa0f6f/lib/arborist/load-actual.js#L433-L435

On macOS we get a result back and avoid creating a new node. On Windows we get no result since the key in the cache is using backslashes but the lookup key has both backslashes and forward slashes. This results in a new node being created with the path of an existing node which in turn removes the existing node and its children from the tree. The children are however not loaded from the file system again because of this check:
https://github.com/npm/arborist/blob/02f295f2d0caa6de2d3235c6eb1c180a8afa0f6f/lib/arborist/load-actual.js#L353-L357

The end result is an unmet dependency and npm attempting to fetch it from the registry instead of using the bundled dependency.

I've changed the cache key by normalizing the path to avoid replacing existing nodes. I've setup a minimal test case where the difference in behavior (with and without normalization) shows up as an error on the outgoing edge of a node.

References

Fixes #2757

Dependencies nested in node_modules triggers an
issue with a file path used as a cache key in the
_findMissingEdges function on Windows due to
paths with mixed path separtors.
Normalize path to avoid mixed path separators on
Windows causing a cache miss for paths that refer
to the same thing. This would otherwise cause a
new node with the path of an existing node to be
created which in turns deletes the existing node
and its children. However, the children will not
be loaded again from the file system due to the
_actualTreeLoaded check in _loadFSTree.
@fritzy fritzy requested a review from a team as a code owner January 19, 2022 00:59
@fritzy fritzy changed the title salvadorj/fix load actual cache key fix(arborist) _findMissingEdges missing dependency due to inconsistent path separators Jan 19, 2022
@fritzy fritzy added the ws:arborist Related to the arborist workspace label Jan 19, 2022
@npm-robot
Copy link
Contributor

npm-robot commented Jan 19, 2022

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 57.123 ±1.79 30.718 ±0.03 24.159 ±7.60 19.606 ±0.92 2.791 ±0.03 2.829 ±0.02 2.343 ±0.10 11.274 ±0.10 2.300 ±0.00 3.278 ±0.04
#4261 52.857 ±2.33 30.157 ±0.01 29.424 ±16.72 19.602 ±0.38 2.942 ±0.05 2.889 ±0.00 2.304 ±0.00 11.341 ±0.05 2.318 ±0.01 3.458 ±0.28
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 37.259 ±0.19 23.010 ±0.37 12.549 ±0.19 13.672 ±0.07 2.576 ±0.01 2.549 ±0.00 2.249 ±0.01 8.196 ±0.02 2.128 ±0.00 2.901 ±0.01
#4261 38.673 ±1.38 23.325 ±0.08 12.761 ±0.11 13.500 ±0.05 2.584 ±0.04 2.614 ±0.04 2.302 ±0.04 8.266 ±0.02 2.069 ±0.05 2.938 ±0.02

@salvadorj
Copy link
Contributor

@fritzy Do I need to do something about the test failures due to ERR_SOCKET_TIMEOUT? It can't reproduce it locally. Could this be an existing test instability?

@wraithgar wraithgar changed the base branch from latest to release-next January 26, 2022 20:43
@salvadorj
Copy link
Contributor

Hey @fritzy, is there something specific blocking this PR? Can I trigger a rerun of the test suite that failed due to what looked like a timeout issue?

@wraithgar wraithgar changed the base branch from release-next to latest March 9, 2022 18:24
@nlf nlf merged commit 0e7511d into latest Mar 15, 2022
@nlf nlf deleted the salvadorj/fix-load-actual-cache-key branch March 15, 2022 18:10
@nlf nlf mentioned this pull request Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ws:arborist Related to the arborist workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] npm install attempts to fetch already bundled indirect dependency from registry
5 participants