Memory leak each time cachableLookup is imported and constructed.

[dylang]

Hi!

I think I've stumbled into a memory leak in cacheable lookup. This leak is present in got, as well as other libraries that are using got, such as webdriver.io.

const CL = require('cacheable-lookup');
new CL();

I'm working on a standalone example to help reproduce the problem.

[dylang]

Well, we can see this way pretty easily:

const CL = require('cacheable-lookup');
Array.from({ length: 1000 }).forEach(() => new CL())

> (node:60022) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 change listeners added. Use emitter.setMaxListeners() to increase limit
(node:60022) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added. Use emitter.setMaxListeners() to increase limit

<--- Last few GCs --->

[60022:0x10287f000]   115887 ms: Mark-sweep 1397.4 (1425.0) -> 1397.1 (1425.5) MB, 919.5 / 0.2 ms  (average mu = 0.163, current mu = 0.066) allocation failure scavenge might not succeed
[60022:0x10287f000]   116723 ms: Mark-sweep 1397.3 (1425.5) -> 1397.3 (1425.5) MB, 746.4 / 0.3 ms  (average mu = 0.138, current mu = 0.107) allocation failure GC in old space requested


<--- JS stacktrace --->

==== JS stack trace =========================================

    0: ExitFrame [pc: 0x3368bc15be3d]
Security context: 0x15b87979e6c1 <JSObject>
    1: _update [0x15b82418f719] [/Users/dgreene/projects/prs/jest-circus-memory-leak/node_modules/cacheable-lookup/source/hosts-resolver.js:~52] [pc=0x3368bc448710](this=0x15b85406c5d9 <HostsResolver map = 0x15b8dd96e051>)
    2: /* anonymous */ [0x15b85407c221](this=0x15b827a8d461 <JSGlobal Object>,0x15b80f002201 <Very long string[372311]>)
    3: StubFrame...

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
 1: 0x100d77bad node::Abort() (.cold.1) [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
 2: 0x10003ae83 node_module_register [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
 3: 0x10003b044 node::FatalTryCatch::~FatalTryCatch() [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
 4: 0x1001aa117 v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
 5: 0x1001aa0b1 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
 6: 0x1005994c2 v8::internal::Heap::FatalProcessOutOfMemory(char const*) [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
 7: 0x10059bbf3 v8::internal::Heap::CheckIneffectiveMarkCompact(unsigned long, double) [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
 8: 0x100597b6e v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::GCCallbackFlags) [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
 9: 0x1005959ed v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
10: 0x1005a2c3f v8::internal::Heap::AllocateRawWithLigthRetry(int, v8::internal::AllocationSpace, v8::internal::AllocationAlignment) [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
11: 0x1005a2c7f v8::internal::Heap::AllocateRawWithRetryOrFail(int, v8::internal::AllocationSpace, v8::internal::AllocationAlignment) [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
12: 0x1005710c5 v8::internal::Factory::NewFixedArrayWithFiller(v8::internal::Heap::RootListIndex, int, v8::internal::Object*, v8::internal::PretenureFlag) [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
13: 0x1006ea5e8 v8::internal::HashTable<v8::internal::NameDictionary, v8::internal::NameDictionaryShape>::EnsureCapacity(v8::internal::Handle<v8::internal::NameDictionary>, int, v8::internal::PretenureFlag) [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
14: 0x10069f9e5 v8::internal::Dictionary<v8::internal::NameDictionary, v8::internal::NameDictionaryShape>::Add(v8::internal::Handle<v8::internal::NameDictionary>, v8::internal::Handle<v8::internal::Name>, v8::internal::Handle<v8::internal::Object>, v8::internal::PropertyDetails, int*) [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
15: 0x100821cb6 v8::internal::Runtime_AddDictionaryProperty(int, v8::internal::Object**, v8::internal::Isolate*) [/Users/dgreene/.nvm/versions/node/v10.20.1/bin/node]
16: 0x3368bc15be3d
17: 0x3368bc448710
18: 0x3368bc163852
zsh: abort      node

[dylang]

While it's probably not normal to create 1000 cacheable-lookup objects in a production env, it is typical to use got in Jest tests, and Jest will freshly import the dependencies in each test. This means a project with 1000 tests using got will easily run out of memory.

[dylang]

I don't know if this is the only reason for the memory leak, but it seemed like a good place to start.

https://github.com/szmarczak/cacheable-lookup/blob/master/source/hosts-resolver.js#L26-L49

Class constructors must be synchronous. I wish that wasn't the case, but that's just how classes have to work. 😓 In the code above, it looks like we're trying to hack around that, but it's not a good idea. There's a hidden requirement that get(hostname) must be awaited, otherwise the promise is just going to be in limbo forever.

This is clever when we know get(hostname) is about to be called, but we can't assume that in a library used by other packages. What if it's never called?

[dylang]

Separately, it may not be wise to use watchFile to watch the system's host file. We're adding a new file watcher for every instance of this package, maybe for every instance of the class, even if the class is just created but never used.

I don't see any code to stop watching or disable watching, but i could be wrong.

Perhaps watching the hostfile should be opt-in? hostfiles don't change often, especially in a production server, and polling a file that never changes doesn't feel like a great use of the eventloop in a production environment.

[szmarczak]

Thanks for the very detailed report, I will fix this issue ASAP.

As a workaround you can disable the hosts file via setting customHostsPath to false. To fix this in Got, you would need to pass a custom DNS cache instance.

[sindresorhus]

Perhaps watching the hostfile should be opt-in? hostfiles don't change often, especially in a production server, and polling a file that never changes doesn't feel like a great use of the eventloop in a production environment.

Should definitely be opt-in.

[szmarczak]

This is clever when we know get(hostname) is about to be called, but we can't assume that in a library used by other packages. What if it's never called?

The hosts resolver is an internal module. It's not exposed to the public. To disable it you can do new CacheableLookup({customHostsPath: false}). That's what I'm currently doing in tests.

Separately, it may not be wise to use watchFile to watch the system's host file. We're adding a new file watcher for every instance of this package, maybe for every instance of the class, even if the class is just created but never used.

I'd go even further. Only one HostsResolver per hosts file.