-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MaxListenersExceededWarning: Possible EventEmitter memory leak detected. #1295
Comments
The undici project and we might have a problem where the body isn't consumed what would happend if you try out import https from 'node:https'
import fetch from 'node-fetch'
const agent = new https.Agent({ keepAlive: true })
async function start () {
for (let i = 1; i < 1000; i++) {
const res = await fetch('https://example.com', { agent })
for await (const chunk of res.body) {} // do nothing
}
}
process.on('warning', e => console.warn(e.stack))
start().catch(e => console.error(e)) |
i might have some trick up my sleeve of how to handle unhandled bodies with finalizers |
@jimmywarting thanks for a prompt reply! Indeed body consumption affected the frequency of warnings (I only have 2 now for this example), but they are still there. In production projects response bodies are always consumed by calling |
We just spent more than a week upgrading our code base to ESM and can finally upgrade to node-fetch 3, but was immediately hit by this issue. @jimmywarting: while not consuming the body is probably causing leaks, I don't think this issue is related to that. 51861e9#diff-bfe9874d239014961b1ae4e89875a6155667db834a410aaaa2ebe3cf89820556R310 looks fundamentally suspicious; a |
After some experimentation, it turns out that this solution is not compatible with keep-alive. Because the socket is the only place that can be used to verify that a chunked transfer has properly finished, the only option is to disable |
…keep-alive requests node-fetch#1295
@tamtamchik @kristoffer-zliide I pushed up a change to remove the fix when keep-alive is enabled. Please give it a try when you have a moment: https://github.com/tekwiz/node-fetch/tree/fix/chunked-encoding-keepalive |
@tekwiz made a couple of runs for this code: import https from 'node:https'
import fetch from 'node-fetch'
const agent = new https.Agent({ keepAlive: true })
async function start () {
for (let i = 1; i < 1000; i++) {
const res = await fetch('https://example.com', { agent })
for await (const chunk of res.body) {} // do nothing
}
console.log(`memoryUsage: `, process.memoryUsage());
}
process.on('warning', e => console.warn(e.stack))
start().catch(e => console.error(e)) Results without your patch: $ time node fetch-https.js
(node:5553) ExperimentalWarning: stream/web is an experimental feature. This feature could change at any time
ExperimentalWarning: stream/web is an experimental feature. This feature could change at any time
...
(node:5553) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 data listeners added to [TLSSocket]. Use emitter.setMaxListeners() to increase limit
MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 data listeners added to [TLSSocket]. Use emitter.setMaxListeners() to increase limit
...
memoryUsage: {
rss: 103960576, // 99.1 Mb
heapTotal: 52555776, // 50.1 Mb
heapUsed: 22500456, // 21.4 Mb
external: 17568481,
arrayBuffers: 16409888
}
node fetch-https.js 2.90s user 0.19s system 1% cpu 3:05.48 total Results with your patch (I'm aware of setting up AbortController, but sacrificed it for sake of test simplicity, in fact when I used it it gave identical results because I had no timeouts in my requests): $ time node fetch-https.js
(node:5666) ExperimentalWarning: stream/web is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
ExperimentalWarning: stream/web is an experimental feature. This feature could change at any time
...
memoryUsage: {
rss: 50876416, // 48.5 Mb
heapTotal: 8253440, // 7.9 Mb
heapUsed: 7273968, // 6.9 Mb
external: 1654597,
arrayBuffers: 241914
}
node fetch-https.js 1.81s user 0.20s system 1% cpu 3:05.54 total |
Curios what the memoryUsage was from the start... 🤔 |
@tekwiz: we replaced We already have abort controller-based timeout in those services, but can you elaborate a bit on it is we need to be aware of, in particular, what has changed since 2.6.2 that caused this issue to happen? How is 2.6.2 dealing with these issues? |
@jimmywarting slightly adjusted the script https://gist.github.com/tamtamchik/8ad09447ae9ad8621e268fa9921d5917 without patch (made several runs, results are almost identical):
with patch:
|
Be aware of connection problems. With chunked transfer-encoding, Node.js has trouble figuring out that a chunked response didn't complete successfully if the connection ends prematurely (#1055). Also, and maybe more importantly, there are certain cases where chunked responses never trigger the end of the stream, and programs will hang (#968). All versions of node-fetch before 3.0.0-beta.10 are affected. |
Thank you, @tekwiz. |
What's the hold-up here? Can we please get a release without memory leaks? Thanks. |
Any updates on this issue? |
I am getting these errors and after Googling I believe it is related to using node-fetch (Google Cloud Functions Node.js runtime). In my package-lock.json I believe the version I'm using is |
I've found the problem. In In #1474 I fix that problem by removing both signals on the socket's |
Is it possible to get an update on this issue? We eagerly await a new version without the memory leak. |
So far I have been waiting on @tekwiz response to #1325 (review) - currently it seems as if we have two possible PRs with different approaches |
@jimmywarting The PR has been inactive for 2 months. Can you chose the other fix and get out a release to unblock anyone experiencing the issue? |
In nodejs/undici#430 it was fixed nodejs/undici#431 |
So, what? Both pull requests resolve the problem. #1474 Prevent the leak by removing the previous listener in However. I should say that the #1325 is more correct one. Also note that Both pull request nave no cons. |
Since 8eeeec1 it seems listeners have been leaking on keep-alive sockets. Apparently abort() has been deprecated since v17, using close() the onSocketClose listener is properly removed, and by creating a new onData function I'm able to remove the 'data' listener too.
Could you make a release with this change? |
🎉 This issue has been resolved in version 3.2.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Hi there,
I think there is a regression in version 3.0.
When I'm using Custom Agent (
https
) withkeepAlive: true
option and then just make a bunch of requests. Pretty soon such warnings show up:The same thing with
http
, just listeners added not toTLSSocket
but to justSocket
.Reproduction
Here is a really simple example.
Steps to reproduce the behavior:
node-fetch@3
installed and you will start getting warnings.node-fetch@2
as a dependency, run the same code, and boom, no warnings.Expected behavior
Version 2.6.2 does not produce any warning on the same code.
Version 3.0.0 produces warnings.
Your Environment
I have been running this on node.js 14 (LTS) and 16 (latest) with the same results.
The text was updated successfully, but these errors were encountered: