Memory leak when using cache

[privatenumber]

Describe the bug

• Node.js version: v12.16.1
• OS & version: macOS 10.15.3

Actual behavior

Getting the following error message when making multiple requests with got:

(node:60669) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added to [KeyvMemcache]. Use emitter.setMaxListeners() to increase limit

Expected behavior

For got/keyv to:

• not keep adding the event listener for each call, leading to the memory leak
• clean up the event listener (can add a teardown feature req in Keyv)

I expect this behavior especially when .extending:

const extendedGot = got.extend({ cache });

Code to reproduce

• I traced it back to keyv:L43
• The cache storage is passed into got, and got passes that into Keyv, which adds the event listener
• In the code below, I shim .on, but in practice I'm using keyv-memcache, which extends the EventEmitter which is what makes the warning about the memory leak

const got = require('got');

const cache = new Map();
cache.on = function(eventName) {
	console.trace(eventName);
};

// Individual calls
for (let i = 0; i < 10; i++) {
	got('http://google.com', { cache });
}

// Extended calls
const extendedGot = got.extend({ cache });
for (let i = 0; i < 10; i++) {
	extendedGot('http://google.com');
}

Checklist

• I have read the documentation.
• I have tried my code with the latest version of Node.js and Got.

For what it's worth, I also read #792, jaredwray/cacheable#30, and #921 (the "click for solution" link is broken for me) but I believe the problem is different.

[szmarczak]

You're creating a new Keyv instance every time (using the cacheable-request package unknowingly).

[szmarczak]

@sindresorhus I think we should disallow passing raw storage instances and force users to pass a Keyv instance.

[sindresorhus]

I think we should disallow passing raw storage instances and force users to pass a Keyv instance.

That's inconvenient for users just wanting a simple memory cache. Can we not just handle it somehow?

[szmarczak]

Can we not just handle it somehow?

It can be solved on the Keyv side via a WeakMap.

[privatenumber]

A destructor/destroy method can also be added in Keyv that takes care of tear-downs.

[szmarczak]

Actually I can solve this in Got, then this issue would be fixed as well as #1098

[szmarczak]

Fixed in 2abacff

[szmarczak]

The script now gives (I replaced console.trace with console.log and ran node --trace-warnings bug.js):

error
(node:24387) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added to [Keyv]. Use emitter.setMaxListeners() to increase limit
    at _addListener (events.js:390:17)
    at Keyv.addListener (events.js:406:10)
    at Keyv.once (events.js:437:8)
    at /home/szm/Desktop/got/node_modules/cacheable-request/src/index.js:190:16
    at /home/szm/Desktop/got/node_modules/cacheable-request/src/index.js:202:6
    at /home/szm/Desktop/got/dist/source/core/index.js:62:59
    at new Promise (<anonymous>)
    at cacheFn (/home/szm/Desktop/got/dist/source/core/index.js:56:41)
    at PromisableRequest._makeRequest (/home/szm/Desktop/got/dist/source/core/index.js:881:39)
    at /home/szm/Desktop/got/dist/source/core/index.js:291:28

It just attaches keyv.once('error', ...) but it's removed when the cache has been read.

[szmarczak]

Related: jaredwray/keyv#101

[szmarczak]

Reopening as we need to test this with newest cacheable-request

[Rumatoid]

memory leak

const map = new Map();

let response = await got('https://www.youtube.com', { cache: map });

for (let i = 0; i < 1000; i++)
    response = await got('https://www.youtube.com', { cache: map });

[GalRab]

It seems to be fix in cacheable-request version 10.2.6
Could you please update cacheable-request to version 10.2.6?