Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Limit maxSockets for client connections #130

Closed
wants to merge 1 commit into from

Conversation

misterbyrne
Copy link
Contributor

NPM installs are failing all the time for installs of large shrinkwrapped projects on my home (wifi) network. It tends to work on a wired network. I figured out that this was due to the large number of connections that npm makes to the registry. Inflating a shrinkwrap results in a request being made to the registry for each item in the dependency tree - and because of npm3's flat file structure, almost all of them happen at once.

This PR makes use of request's pool option, along with the agent's maxSockets option to use client connection pooling which drastically reduces the number of outgoing connections, resulting in a quicker and more stable install.

It looks like this should fix some issues people are having with installing lots of dependencies over consumer network hardware / crappy internet connections / environments with limited resources.

@misterbyrne misterbyrne force-pushed the use-connection-pool branch 2 times, most recently from d7b8fd5 to 58dab93 Compare February 21, 2016 22:01
@@ -39,6 +39,7 @@ function RegClient (config) {
if (typeof this.config.retry.factor !== 'number') this.config.retry.factor = 10
if (typeof this.config.retry.minTimeout !== 'number') this.config.retry.minTimeout = 10000
if (typeof this.config.retry.maxTimeout !== 'number') this.config.retry.maxTimeout = 60000
if (typeof this.config.maxSockets !== 'number') this.config.maxSockets = 50
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The number 50 is pretty arbitrary but seemed to perform okay. Maybe the default should be lower or higher though. I've tried a few different values with a big shrinkwrap install and it didn't seem to make much difference past 20.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I expect we'll want the npm side of this patch to pass through a value and to have a default there too, since this is the sort of thing where the best value is likely heavily impacted by network conditions.

@misterbyrne
Copy link
Contributor Author

Update: after more tests - this time with nettop and a small value for maxConnections, I could see that passing the agent to request was, in fact, working okay. I looked at request's source code and can see that while the previous approach was having the desired effect - it was more of a side-effect than this approach.

Also, PR for the npm side is here: npm/npm#11666

@misterbyrne misterbyrne changed the title Use a client connection pool Limit maxSockets for client connections Feb 25, 2016
iarna pushed a commit that referenced this pull request Feb 25, 2016
@iarna
Copy link
Contributor

iarna commented Feb 25, 2016

Looks great! Landed as 3aeca7a and published in v7.1.0

@iarna iarna closed this Feb 25, 2016
@iarna iarna removed the review label Feb 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants