Memory leak when using cache feature.

[clement-buchart]

Hello,

We are facing a memory leak in our production environment, using got and it's cache feature (with a KeyvRedis storage backend).

A heap dump revealed that most memory is retained by multiple error event listener on the single KeyvRedis instance.
Further analysis showed that for each request, a new CacheableRequest is instantiated :

if (options.cache) {
	const cacheableRequest = new CacheableRequest(fn.request, options.cache);
	const cacheRequest = cacheableRequest(options, handleResponse);

	cacheRequest.once('error', error => {
		if (error instanceof CacheableRequest.RequestError) {
			emitError(new RequestError(error, options));
		} else {
			emitError(new CacheError(error, options));
		}
	});

	cacheRequest.once('request', handleRequest);
}

Then, for each CacheableRequest, a new Keyv is instantiated :

this.cache = new Keyv({
	uri: typeof cacheAdapter === 'string' && cacheAdapter,
	store: typeof cacheAdapter !== 'string' && cacheAdapter,
	namespace: 'cacheable-request'
});

KeyvRedis being an EventEmitter, a eventListener is registered.

if (typeof this.opts.store.on === 'function') {
	this.opts.store.on('error', err => this.emit('error', err));
}

The full context of the request being retained by this event listener.

Is there something wrong with the way we use got ? Is there a way to prevent CacheableRequest to be instantiated for each request ?

We tried using got.create, but that didn't work.

Thanks.

[szmarczak]

We already are aware of this, but this is not so simple to solve. It's possible to remove the initialization of CacheableRequest instance. If to take the request function out of the constructor and move it to the options instead, the only thing left to fix would be the Keyv initialization. There are two solutions here:

1. Force users to make keyv instances on their own.
2. Generate the instance on got.extend or got.create (the better one).

[szmarczak]

This is how it's done for cacheable-lookup:

got/source/normalize-arguments.ts

Lines 96 to 100 in c8b96aa

	if (options.dnsCache) {
	const cacheableLookup = new CacheableLookup({cacheAdapter: options.dnsCache});
	options.lookup = cacheableLookup.lookup;
	delete options.dnsCache;
	}

[szmarczak]

We also need to note in the README that dnsCache is used only when normalizing and it will translate to the lookup option.

[sindresorhus]

Seems like this can be closed now that jaredwray/cacheable#30 is closed?

[szmarczak]

Yeah... I think so. We could still improve this by pregenerating the instance on got.create()/got.extend() and regenerate if someone provides custom request or cache option. What do you think?

[AaronFriel]

@sindresorhus Could you release a 9.6.1 that uses cacheable-request 6.1.0?

[jburghardt]

A 9.6.1 release would be appreciated