Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: introduce dns caching #441

Closed
3 tasks
nlf opened this issue Mar 14, 2022 · 3 comments · Fixed by npm/make-fetch-happen#132
Closed
3 tasks

feat: introduce dns caching #441

nlf opened this issue Mar 14, 2022 · 3 comments · Fixed by npm/make-fetch-happen#132
Assignees
Labels

Comments

@nlf
Copy link

nlf commented Mar 14, 2022

Is your feature request related to a problem? Please describe.

While we do use keep-alives when making http requests, which reduces the number of new connections and therefore DNS lookups, node itself does not cache DNS results anywhere. Instead it relies on spawning a uv thread and making a synchronous call to getaddrinfo. This blocks the event loop every time a new socket is created, which in some cases can be detrimental to performance. This is especially notable when a proxy is in use, since in that event we no longer have the benefit of keep-alives and as such are potentially creating a significant amount of dns lookups.

Describe the solution you'd like

To work around this, we should add a simple DNS cache. A custom lookup function can be provided to http.request() and https.request() which ultimately power minipass-fetch. We should do the following:

  • make sure minipass-fetch accepts the lookup option and passes it to http(s).request()
  • implement a caching lookup function, this could be a standalone module or built in to make-fetch-happen
  • bonus: round-robin dns entries by default. if a request to a specific host fails, remove that host from the cache so that the retried request will be attempted with a different host. if the cache is empty, allow the call to dns.lookup() through to repopulate it

Describe alternatives you've considered

Another option would be to create our own Agent. This Agent could support keep-alives, idle timeouts, dns caching, and proxies in one place. The node core Agent implementation already allows for keep-alives. Idle timeouts are a very simple extension, as is dns caching. These features could then be tied more closely to how make-fetch-happen and minipass-fetch work (specifically synchronous stream behaviors and error handling). Proxy support is somewhat more difficult. A possible step we could take is dropping agentkeepalive in favor of a new simpler Agent and continuing to use http-proxy-agent, https-proxy-agent and socks-proxy-agent as we do today.

@ljharb
Copy link

ljharb commented Mar 14, 2022

I wonder if there's also a security consideration to the status quo, namely that DNS resolution could change during the middle of an install?

@wraithgar
Copy link
Member

As long as the dns ttl is obeyed it's no different than the existing dns caching systems do.

@simllll
Copy link

simllll commented Mar 21, 2022

As I was curious about this issue, I was playing around with this. I've a very slow npm install step here anyway,

#17 [stage-1 7/10] RUN --mount=type=cache,target=/app/.npm npm set cache /app/.npm && npm install --prefix=. --production --no-audit --no-fund --loglevel=error && rm -f .npmrc
#17 sha256:27187501aa1675590b7925672eb05da4ab9c09322776b94ab127446b0e9cedd4
#17 1326.6
#17 1326.6 added 1764 packages in 22m

Using my verdaccio npm server as a proxy, I was able to use it:
a.) via hostname
b.) directly via IP

as I can say, there is no difference at all in my setup. It always takes around 22minutes (+/-1) (npm 8.5.5 if this is relevant)

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

Successfully merging a pull request may close this issue.

5 participants