Skip to content
This repository has been archived by the owner on Jul 3, 2019. It is now read-only.

Config maxSockets not passed down in registry request #109

Closed
tgandrews opened this issue Aug 11, 2017 · 8 comments
Closed

Config maxSockets not passed down in registry request #109

tgandrews opened this issue Aug 11, 2017 · 8 comments

Comments

@tgandrews
Copy link
Contributor

tgandrews commented Aug 11, 2017

Currently it seems as if maxSockets is not being passed down to make-fetch-happen when making a registry request

return fetch(uri, {
agent: opts.agent,
algorithms: opts.algorithms,
cache: getCacheMode(opts),
cacheManager: opts.cache,
ca: opts.ca,
cert: opts.cert,
headers: getHeaders(uri, registry, opts),
integrity: opts.integrity,
key: opts.key,
localAddress: opts.localAddress,
memoize: opts.memoize,
noProxy: opts.noProxy,
Promise: BB,
proxy: opts.proxy,
referer: opts.refer,
retry: opts.retry,
strictSSL: !!opts.strictSSL,
timeout: opts.timeout,
uid: opts.uid,
gid: opts.gid
}).then(res => {

So this means it gets defaulted to 15
https://github.com/zkat/make-fetch-happen/blob/800a8e54581e1b84e53e42a274c1eea7f8c652b6/agent.js#L48

Through some console.log debugging it seems to explain why npm config set maxsockets 5 is not being applied.

@zkat
Copy link
Owner

zkat commented Aug 13, 2017

oh huh!

I'll totally take a patch for this.

Another thing that you might wanna do is patch npm to divvy up the maxSockets by the number of subprocesses spawned during extract -- otherwise, your maxsockets setting will end up getting multiplied by the number of workers in the extraction pool.

@tgandrews
Copy link
Contributor Author

I'll have a look this week.

@tareksha
Copy link

tareksha commented Feb 6, 2018

hi @tgandrews , @zkat ,

is this related to the problems below?
npm/npm#16263
npm/npm#18903

npm 5 opens too many connection to a proxy server when it is configured with https-proxy. maxsockets is effectively ignored. the failure trace usually points to pacote and the components below it (make-fetch-happen, https-proxy-agent, ..)

FetchError: request to https://registry.npmjs.org/grunt failed, reason: getaddrinfo EAI_AGAIN proxy.mycompany.corp.:8080
at ClientRequest.req.on.err (.../npm/.../pacote/.../make-fetch-happen/.../node-fetch-npm/src/index.js:68:14)
at emitOne (events.js:116:13)
at ClientRequest.emit (events.js:211:7)
at onerror (.../npm/.../pacote/.../make-fetch-happen/.../https-proxy-agent/.../agent-base/index.js:106:9)
at callbackError (.../npm/../pacote/.../make-fetch-happen/.../https-proxy-agent/../agent-base/index.js:126:5)

I've observed that with one package, installing with a clean cache folder using npm@3 creates ~160 connections to the proxy while exactly the same setup with npm@5 collapses after bombarding over 600 connections.

pacote is version 6.0.2

I'd be glad to help, just need some basic guidance here ..

thank you

@tgandrews
Copy link
Contributor Author

It may be. I was finding a similar issue and hence ended up looking and adding it so the value for maxSockets was passed down and whilst it does help it is a multiple of the number of the workers as mentioned by @zkat

Does setting the value to one in the npm config reduce the number of connections being made?

@tareksha
Copy link

tareksha commented Feb 6, 2018

hi @tgandrews,

reducing the maxsockets npm config does not help. the default is 50 but the issue still reproduces even with a limit of 5 or less.

can it be that the CONNECT requests that are sent to https proxies before the actual TLS/SSL handshakse are initiated prematurely ?

@tgandrews
Copy link
Contributor Author

IIRC I think you could change the way spawning is done in npm to reduce it to just one worked then combining it with a maxSockets of 1 you should get one connection.

@mk-pmb
Copy link

mk-pmb commented Feb 8, 2018

Another thing that you might wanna do is patch npm to divvy up the maxSockets by the number of subprocesses spawned during extract

For low maxSockets numbers, you might even need to limit the number of concurrent extract subprocesses. A user-defined concurrency limit would be nice, too; it might help run/debug npm with restricted memory use.

@mk-pmb
Copy link

mk-pmb commented Feb 8, 2018

change the way spawning is done in npm to reduce it to just one worked

In case of maxSockets=1, that seems to be the only (and obvious) solution. For medium and high values, there should be better performance balances than fully serial.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants