Skip to content

Commit

Permalink
Disable options.dnsCache by default
Browse files Browse the repository at this point in the history
  • Loading branch information
@szmarczak
szmarczak committed May 8, 2020
1 parent 05ff878 commit 79507c2
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 5 deletions.
9 changes: 6 additions & 3 deletions readme.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -481,10 +481,13 @@ Default: `false`

###### dnsCache

Type: `object | false`\
Default: `new CacheableLookup()`
Type: `CacheableLookup | false`\
Default: `false`

An instance of [`CacheableLookup`](https://github.com/szmarczak/cacheable-lookup) used for making DNS lookups. Useful when making lots of requests to different *public* hostnames.

An instance of [`CacheableLookup`](https://github.com/szmarczak/cacheable-lookup) used for making DNS lookups.
**Note:** This should stay disabled when making requests to internal hostnames such as `localhost`, `database.local` etc.\
`CacheableLookup` uses `dns.resolver4(..)` and `dns.resolver6(...)` under the hood and fall backs to `dns.lookup(...)` when the first two fail, which may lead to additional delay.

###### request

Expand Down
4 changes: 4 additions & 0 deletions source/core/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,10 @@ export default class Request extends Duplex implements RequestEvents<Request> {
options.cache = undefined;
}

if (options.dnsCache === false) {
options.cache = undefined;
}

// Nice type assertions
assert.any([is.string, is.undefined], options.method);
assert.any([is.object, is.undefined], options.headers);
Expand Down
3 changes: 1 addition & 2 deletions source/index.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import {URL} from 'url';
import CacheableLookup from 'cacheable-lookup';
import {Response, Options} from './as-promise';
import create, {defaultHandler, InstanceDefaults} from './create';

Expand Down Expand Up @@ -54,7 +53,7 @@ const defaults: InstanceDefaults = {
afterResponse: []
},
cache: undefined,
dnsCache: new CacheableLookup(),
dnsCache: undefined,
decompress: true,
throwHttpErrors: true,
followRedirect: true,
Expand Down

2 comments on commit 79507c2

@simenandre
Copy link

Choose a reason for hiding this comment

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

Should this maybe be released as breaking changes? I know the API is still the same,
but there could be users who are dependant on CacheableLookup in their stack.

@szmarczak
Copy link
Collaborator Author

@szmarczak szmarczak commented on 79507c2 May 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@cobraz Unfortunately this had a negative effect for most people. Memory leaks, inconsistencies with the Node.js behavior, problems with parsing the hosts file, OS-specific DNS rules don't apply, the problem is really big, and the fix is not quite easy to do. Releasing Got 12 would be too much, users can easily add it back by const instance = got.extend({dnsCache: new CacheableLookup()}). The CacheableLookup package is not battle-tested. But that doesn't mean that the DNS cache won't be enabled by default in the future. I'm working on the fix. See szmarczak/cacheable-lookup#31

Please sign in to comment.