fix(current-env): freezing when calling current env #122
Conversation
| } | ||
| return family | ||
| function libc () { | ||
| return familySync() || undefined |
There was a problem hiding this comment.
Could you cache locally the result of familySync?
As it can still be a not so fast call (blocking file read operation or blocking get report) and it's done for each dependency in the tree, so it should be cached because it will never change
There was a problem hiding this comment.
NVM, it seems each subFunction is cached in detect libc, like getReport or getFamily from fs
| function isMusl (file) { | ||
| return file.includes('libc.musl-') || file.includes('ld-musl-') | ||
| } | ||
| const { familySync } = require('detect-libc') |
There was a problem hiding this comment.
This spawns out to the shell to do its detection. I don't know if we want to add that to our security footprint at this time.
There was a problem hiding this comment.
Only in very rare cases, if ever (because it's only on linux as a fallback to reading the ldd file and the getReport)
https://github.com/lovell/detect-libc/blame/2642d9612fadf14f3eeb5cd56eb3a63b254c7693/lib/detect-libc.js#L144-L154
But if it's a problem we can just take the read from ldd file logic which will solve 99% of problems
There was a problem hiding this comment.
I think the ldd file logic is likely the way to go.
There was a problem hiding this comment.
I'll put it in my PR then, could you explain how do you bypass the cache with a separate file and a mock or give an example?
There was a problem hiding this comment.
This PR already has some of it, albeit by using requiremock. We use tap so we'd just want to use tap.mock.
I'd say current-env is already sufficiently isolated to be ready for mocking.
Perhaps mocking isn't even the right option here. The require cache is the problem. cleaning it as a t.afterEach may be sufficient.
There was a problem hiding this comment.
delete require.cache[require.resolve(module)]
|
I'm a little bit confused as to why this is happening at all. We built in a guard so that v7.1.0 fixed this https://github.com/npm/npm-install-checks/blob/main/CHANGELOG.md#710-2024-09-11 ... and was added to npm 10.9.0 https://github.com/npm/cli/blob/latest/CHANGELOG.md#1090-2024-10-03 |
This is where the problem lies, it doesn't happen just in windows, it's only more pronounced there because the DNS implementation seems to be VERY slow (and with WSL it's even worse because DNS queries are not cached) Get report is in itself a debug function and shouldn't really be used for this. Under the hood it does a lot of expensive synchronus and blocking operations, like reporting on each core of the CPU, reverse DNS queries for each open handle (and with node each request is kept alive by default for 5s keeping the handles open) And so depending on the system and what happened before in the process, it could take 2ms like it could take 3 minutes to complete, (and that's just for 1 call) |
|
Great, thanks for the education @Tofandel |
| @@ -1,4 +1,7 @@ | |||
| const t = require('tap') | |||
| const rewiremock = require('rewiremock/node') | |||
There was a problem hiding this comment.
Tap has this built in as t.mock
|
As for the fix in #118 I'll point you to nodejs/node#55576 (comment) where basically I found that the flag So now when nodejs/node#55602 is released, #118 will have an effect, but right now it doesn't |
|
Ok thanks for everyone's patience waiting for us to get to this, we have a lot of things in flight right now for npm 11, and also an impromptu npm 10 deps update. I think at the end of the day we want to do two things here. First, we should cache the ultimate values that WDYT? |
|
@wraithgar That's ok to me, I will rewrite this PR to not include I will try to use the |
|
I think we have two PRs that are trying to solve the same issue now #120 |
|
I will close this in favor of #120 since is basically what I was planning to do. |
|
Sorry for the confusion there. Thanks for all your help. |
Related #121
Related #120
This PR basically avoid calling
getReportthat is well-known for being slow on linux machines with multiple cores (nodejs/node#48831), and also avoid a bug in theexcludeNetworkfixed on nodejs/node#55602.From my tests, the issue with CPU didn't impact the performance, but the
excludeNetwork, fixed in the 55602, was the principal issue that was slowing down thenpm install.Instead of copying the code from
detect-libc, I prefer to just import the library which has been very well tested.