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

dns cache #8

Closed
azu opened this issue Aug 23, 2019 · 4 comments
Closed

dns cache #8

azu opened this issue Aug 23, 2019 · 4 comments

Comments

@azu
Copy link
Contributor

azu commented Aug 23, 2019

Though the call to dns.lookup() will be asynchronous from JavaScript's perspective, it is implemented as a synchronous call to getaddrinfo(3) that runs on libuv's threadpool. This can have surprising negative performance implications for some applications, see the UV_THREADPOOL_SIZE documentation for more information.
-- DNS | Node.js v12.9.0 Documentation

Maybe, dns lookup have negative performance implications.
I think that lookup should be cached.

However, dns lookup has not ttl option.

Related

@tinovyatkin
Copy link
Owner

Do you expect a lot of the equal queries? I think in this case the better solutions will be a LRU cache around, but not sure if it should be part of this library of wrap around. What do you think? Cache all requests with LRU cache?

@azu
Copy link
Contributor Author

azu commented Aug 23, 2019

I think in this case the better solutions will be a LRU cache around, but not sure if it should be part of this library of wrap around.

Agree.

I'm ok that caching is out of scope of this library.

📝 I've filed this issue by following reason.
isLocalhost look like primitive function.
So, It is difficult that the library user care about the performance.

@tinovyatkin
Copy link
Owner

That's exactly the reason why I don't want to make it another lodash or request and keep the core thing simple. Depending on a use case you may or may not want to cache the result.

@tinovyatkin
Copy link
Owner

Digging deeper, we definitely can't replace lookup with resolve here, because it will loose hosts lookup and that is the most common use case for this library.

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

No branches or pull requests

2 participants