limit parallel requests to registry during resolution #564
Conversation
During resolution there is no throttling of requests for metadata. A medium size project (react native v61 boilerplate) can have more than 385 simultaneous open connections to the npm registry. While registry.npmjs.org and registry.yarnpkg.com can handle this load, a private registry such as Nexus may not. This limits the concurrent requests to something manageable for private registries.
| @@ -439,6 +439,8 @@ export class Project { | |||
| mustBeResolved.add(workspaceDescriptor.descriptorHash); | |||
| } | |||
|
|
|||
| const limit = pLimit(10); | |||
There was a problem hiding this comment.
I think this would be a good candidate for a .yarnrc.yml configuration. As you said the npm registry can handle this many open connections and it should be the most used registry. Limiting the default use case for a stress case should be avoided IMO.
There was a problem hiding this comment.
I can incorporate it into the .yarnrc.yml configuration and update the PR.
My one concern about not limiting the default is companies do use private npm registries or proxies in front of npm. This took down our internal nexus proxy and made it unusable. Lucky for me, we brought the server back up before it caused a major disruption. If any engineer decides to create a new package and use yarn v2 it will cause the same problem unless they remember to set this value. If a lockfile exists this logic is not hit, so the only performance hit is when packages are updated or the lockfile is deleted. In the case of updating packages the amount of requests to the registry is typically small enough to not see any slow downs.
Does it make sense to still have an upper boundary even for npm? In theory, the current algorithm could make over a 1000 connections simultaneous depending on the packages being resolved.
There was a problem hiding this comment.
Yarn v1 limited concurrent requests to the npm registry at 8 but it is configurable via the .yarnrc file by setting the network-concurrency param.
The rest is a description with code links for how this behaviour was discovered.
From what i can tell the request-manager.js had a queue for both metadata and tarball requests that had a configurable limit which default to 8 via the constant NETWORK_CONCURRENCY. It is configurable via the .yarnrc file with the value network-concurrency code link
There was a problem hiding this comment.
Well then it sounds like we should merge this. I would even argue to use the old default.
There was a problem hiding this comment.
I can change the default to 8, would you like me to expose this as a configuration option as well? If you do want the configuration option, would you like it in this PR or as a follow on?
There was a problem hiding this comment.
A separate PR is fine for me. Wdyt @arcanis ?
There was a problem hiding this comment.
Yep, let's keep this in a separate PR (and we can also make the fetch concurrency configurable) 👍
Let's also keep 10 - the Yarn codebase even used to have 16 but switched to 8 due to some weird bugs in the unpacking logic. Since we don't unpack anymore we don't need to be too cautious there.
|
Thanks @lroling8350! |
What's the problem this PR addresses?
During resolution there is no throttling of requests for metadata. A medium size
project (react native v61 boilerplate) can have more than 385 simultaneous
open connections to the npm registry. While registry.npmjs.org
and registry.yarnpkg.com can handle this load, a private registry such as
Nexus may not.
...
How did you fix it?
This limits the concurrent requests to something manageable
for private registries. It leverages the same tool to limit parallel fetches
of tarballs in the fetchEverything method
...