connections opened but never closed

tareksha commented 6 years ago

Hi,

I've researched the behavior of pacote for the infamous issue of opening too many connections to https proxies during npm@5 installations. After thorough analysis and interceptions, I found that the tool does regulate opening the connections correctly but but the real issue is that connections are never closed!

I've made several tests, the results are the same. Here i present my numbers of installing grunt several times with an empty cache folder. npm 5.5.1 and pacote 6.0.2 were used.

full npm execution took ~26 seconds.
a total of 180 connections were opened to the proxy.
each connection was opened right before it became active, 160 ms on average.
each connection stayed active for 170 ms on average.
all connections were closed ~25.5 seconds after the first connection was created, right when npm terminated.
no more than 50 connections were active at any point in time ( matching maxsockets config ).

All connections are killed by client shutdown (no graceful close), which combined with the times strongly suggests that the connections are unplugged violently only after tool finishes all of its work and terminates.

I think the correct solution is to simply close each connection safely right after the tool finishes using it, Otherwise, as observed by many users, the number of open connections grows uncontrollably high. For many large project and corporate users this renders npm@5 unusable.

thank you

tareksha commented 6 years ago

hi @zkat , @tgandrews , this is related to my comments in https://github.com/zkat/pacote/issues/109

zkat commented 6 years ago

Yes, I'm aware -- I just don't know why those connections are staying alive, is the issue. If you manage to track -that- down, that'd be super useful. Last I looked, it was literally only happening when using the proxy libraries.

zkat commented 6 years ago

I still think this probably has to do with https-proxy-agent, specially since it's a thing that happens when you proxy. There's relatively little I can do on the pacote and make-fetch-happen end of things if that library is... messing up closure. Can you look into it over on that end? If anyone else is willing to do the necessary sleuthing here, that would be -lovely- (/cc @tootallnate)

tareksha commented 6 years ago

Found it !!

The custom agent in https://github.com/TooTallNate/node-agent-base does receive the automatic 'free' event from the underlying socket after the various fetching libraries finish using it but does nothing with it. Technically, this should be used for pooling and reusing TCP connections.

Since the agent does not pool stuff, we should simply close the sockets when they are free by calling socket.end() explicitly. My fix is to close them but I think we should reuse connections in a future improvement.

https://github.com/TooTallNate/node-agent-base/pull/18

zkat commented 6 years ago

omg good job! So excited. I hope this finally resolves this nonsense. 🎉

silverwind commented 6 years ago

Just wondering why this fix hasn't propagated to npm yet. Is it because npm/cli depends on pacote@8?

jjimenez commented 5 years ago

If I'm reading this right, once make-fetch-happen references agent-base 4.2.1 (pull request: https://github.com/zkat/make-fetch-happen/pull/66), pacote can be updated to reference the right version of make-fetch-happen. Once pacote has the right version, npm can be updated since, as of v6.8.0-next.2, it references the latest pacote.

shartte commented 5 years ago

NPM 6.9.0 has fixed this issue for me.

camueller commented 5 years ago

Same here - using NPM 6.9.0 "npm i" runs fine using HTTP proxy.

zkat / pacote

connections opened but never closed #138