-
Notifications
You must be signed in to change notification settings - Fork 225
Description
Description
We got reports of memory leaks from users of React InstantSearch with Next.js.
It's easy to reproduce, with an index.mjs file :
import process from 'process';
import { algoliasearch } from 'algoliasearch';
process.stdin.resume();
const client = algoliasearch('latency', '6be0576ff61c053d5f9a3225e2a90f76');
for (let i = 0; i < 1000; i++) {
client.search({ requests: [{ indexName: 'instant_search' }] });
}Then run node --inspect index.mjs, in Chrome open chrome://inspect and select index.mjs.
In the memory tab, the VM instance heap size hovers around 90MB without ever going down.
All responses are retained by the error handler.
I tried patching it by adding req.removeAllListeners() as well as response.removeAllListeners() in this end handler :
algoliasearch-client-javascript/packages/requester-node-http/src/createHttpRequester.ts
Lines 60 to 69 in 36ccbc9
| response.on('end', () => { | |
| clearTimeout(connectTimeout as NodeJS.Timeout); | |
| clearTimeout(responseTimeout as NodeJS.Timeout); | |
| resolve({ | |
| status: response.statusCode || 0, | |
| content: Buffer.concat(contentBuffers).toString(), | |
| isTimedOut: false, | |
| }); | |
| }); |
It's not perfect but it now settles around 20MB. Some unnecessary values are still there but maybe this will properly get GC'd if needed.

I told them to use requester-fetch instead as it's now supported by recent versions of Node.js. Maybe we could make that the default ? From my own benchmarks it seems better than the Node requester currently but with my proposed fix it would make the Node requester more performant than the fetch one 😅 (with undici)
Client
All
Version
5.15.0
Relevant log output
No response