Skip to content

Conversation

@fallenbagel
Copy link
Contributor

@fallenbagel fallenbagel commented Sep 17, 2025

Description

Forgot to pass in the forceIPv4 option in the this.lookup with the fix in #4. Also fixed an issue with the test where we were testing the IPv6 scenario without clearing previous cache.

In addition, also added process.nextTick around all dns.lookup callbacks to prevent user-callback exceptions from propagating into our internal Promise chain to ensure that .catch handles only real dns errors and not accidental cache updates thrown by a caller code (as further hardening).

Copy link
Member

@gauthier-th gauthier-th left a comment

Choose a reason for hiding this comment

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

  • Why process.nextTick and not setImmediate? IIRC setImmediate is a more standard way of running something for the next event loop, while process.nextTick is only used to do some edge-case optimizations.

  • Both process.nextTick and setImmediate will introduce a small delay because it will end for the current event loop to finish. It shouldn't be that much but it can be a few milliseconds.

  • to prevent user-callback exceptions from propagating into our internal Promise chain

    I get the point, but in this case shouldn't we just invert the order of our .catch and .then so our .catch doesn't catch the errors coming from the call of the callback in our .then?

@fallenbagel
Copy link
Contributor Author

  • Why process.nextTick and not setImmediate? IIRC setImmediate is a more standard way of running something for the next event loop, while process.nextTick is only used to do some edge-case optimizations.

  • Both process.nextTick and setImmediate will introduce a small delay because it will end for the current event loop to finish. It shouldn't be that much but it can be a few milliseconds.

  • to prevent user-callback exceptions from propagating into our internal Promise chain

    I get the point, but in this case shouldn't we just invert the order of our .catch and .then so our .catch doesn't catch the errors coming from the call of the callback in our .then?

So should I remove it entirely?

@fallenbagel fallenbagel force-pushed the fix-dns-cache-force-ipv4-lookup branch from 7cb5fcb to 142e07e Compare September 18, 2025 11:12
Copy link
Member

@gauthier-th gauthier-th left a comment

Choose a reason for hiding this comment

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

LGTM

@gauthier-th gauthier-th merged commit 19301cb into main Sep 18, 2025
1 check passed
@gauthier-th gauthier-th deleted the fix-dns-cache-force-ipv4-lookup branch September 18, 2025 11:19
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

Successfully merging this pull request may close these issues.

3 participants