-
Notifications
You must be signed in to change notification settings - Fork 26
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. 11 abort listeners added to [EventEmitter] #63
Comments
Hi tomas, we hope it gets your team attention 🙏 |
Heya, I'm no longer actively workin on this project, @JoshMock is the new maintainer. |
Hi Josh, thanks for working on it! |
Just spent a moment digging into this, and it looks like its tied to the EventEmitter in the Undici connection class. I verified that by introducing this fix (9012ddc), the errors all disappear. |
Thanks for taking a look @arciisine. 🖤 I'm just onboarding as the new maintainer of this library so I'll take a look at this soon. Your solution is straightforward, so I'll play with it soon to make sure it has no undesired side effects. |
Hey guys, I removed the warning by adding in the |
As a note while I'm working to prioritize a possible fix for this, former maintainer @delvedor mentioned on the Kibana repo that this is just a warning about a possible memory leak for behavior we know is, in fact, just a large number of concurrent requests in progress. So, it's mostly just noise, which you can configure your project to ignore based on the concurrent request limits you'd expect to see in your app.
|
@JoshMock we are seeing a steady increase in memory until the containers die out of memory, over the course of 8-10 hours, happens literally every day. I don't think it's just noise, our baseline goes from 64MB of memory to 8GB... |
That's good to know. And you've narrowed it down to memory being consumed by Elasticsearch client activity? Would love any traces, usage examples, versions used (Elasticsearch and client) or other relevant details. |
I ran a simple test where I indexed 100 documents every 10 seconds for 8 hours, and then another where I ran 100 searches every 10 seconds for 8 hours, and was not able to reproduce a memory leak. The results show typical Node.js garbage collection behavior. (charts below) I'd still like to see a usage example where a memory leak can be seen and traced back to the client. As has been mentioned before, the |
Running another 8-hour test now that indexes 10k documents every second to push the client a bit harder. Also running another 8-hour test that is identical, but with the EventEmitter fix proposed in #55. Will report back next week. |
A more intense test (index 10k docs/second for 8 hours) yielded more interesting results! See charts below. Unfortunately, the proposed fix in #55 did not have any notable positive impact, other than not getting the |
Ran another long-running process as a test, with an actual memory allocation timeline profiler running and identified a potential leak in Undici that may be fixed in a newer version than 5.5.1, which this library currently uses. Rerunning the test with Undici updated to latest (5.22.1) to see if the results differ. |
The upgrade fixes some bugs that were the cause of a slow memory leak in the Elasticsearch JS client. See #63.
The upgrade fixes some bugs that were the cause of a slow memory leak in the Elasticsearch JS client. See #63.
I just tried this upgrade and there is no change to the warning message. This is with ES client 8.8.1, transport version 8.3.2 and undici version 5.22.1 I've also been unable to find a place where setting the global defaultMaxlisteners as indicated in the node docs has any effect at all. The code doesn't fail, and doesn't do anything Did this update hope to remove the warning or did you focus more in the memory consumption later in the thread?
|
@phil-nelson-bt In my tests, upgrading Undici also had the effect of eliminating the max listeners warning. I'll reopen this and run some more tests soon to verify that. In the meantime, updating defaultMaxListeners will still work on a case by case basis. |
Thanks for your patience, everyone. I was out on leave and am just getting back to work. @breno-alves put together a PR that solves for the warnings being logged. Thanks for that! I'll test and merge soon. However, that PR does not address any underlying memory leak issues that might be related to abort listeners. I'd like to dig into that more before fully closing this. If anyone has any isolated code that can reproduce a leak, I'd love to see it. |
same issue node v18.19.1, @elastic/elasticsearch ^8.12.2. |
Same issue here |
Anyone here who is on Node 18+ and experiencing memory leaks: please try out |
Upgrading to Node: v18.16.1 |
With |
Thanks @daveyarwood. I'll keep investigating soon. |
Any chance to fix it within two years? |
I've continued to address different aspects of this issue over the past year as I've had time. Unfortunately there seem to be a few possible things going on rather than one single root cause. Also, the more clear code examples I can get that consistently reproduce a memory leak that can be traced back to EventEmitter or abort listeners, the easier it will be to track down. I've not received any yet, and the test scenarios I've come up with myself have been solved with fixes that have already been merged. |
when a new release will come up? |
@JoshMock tried every possible solution with version upgradation of elastic/transport and undici. But haven't got the fix. |
@JoshMock Script file:
Output:
|
After 8.5.1 even this workaround doesn't work found this for now https://stackoverflow.com/questions/76579772/node-maxlistenersexceededwarning-when-i-use-same-elasticsearch-instance-in-a-for |
we are getting a lot of request timeoutError /usr/src/app/node_modules/@elastic/transport/lib/Transport.js in SniffingTransport.request at line 540:31 |
@JoshMock Is there any way to suppress these warnings until a solution is proposed.. |
|
A potential fix for #63, largely inspired by a community member's PR that was never merged: #55 According to an Undici core committer in this comment elastic/elasticsearch-js#1716 (comment) the issue that triggers the MaxListenersExceededWarning, and possibly a memory leak in some cases, is caused by attaching an EventEmitter to each request by default when a per-request timeout is set, rather than attaching an AbortSignal. My assumption is that an EventEmitter was used because AbortSignal and AbortController were not added to Node.js until v14.17.0, so we couldn't guarantee v14 users would have it. I'm not certain why using EventEmitters makes a difference memory-wise, but it does get rid of the MaxListenersExceededWarning.
I wasn't able to reproduce a memory leak using the code snippet from @mmcDevops. When observing memory while running it, heap usage does spike pretty high as connections are opened, but everything is properly garbage-collected once However, I rediscovered a PR that fell by the wayside that attempted to solve this problem, addressing @ronag's primary concern. I've opened #96 to reproduce that work, with a few minor tweaks. This does get rid of the In any case, if anyone would like to pull down the repo and test on that PR's code with their use cases, that'd help a ton! I'll try to reproduce a leak a few more times, and look to merge the change closer to the release of Elasticsearch 8.14 in mid-May. |
A potential fix for #63, largely inspired by a community member's PR that was never merged: #55 According to an Undici core committer in this comment elastic/elasticsearch-js#1716 (comment) the issue that triggers the MaxListenersExceededWarning, and possibly a memory leak in some cases, is caused by attaching an EventEmitter to each request by default when a per-request timeout is set, rather than attaching an AbortSignal. My assumption is that an EventEmitter was used because AbortSignal and AbortController were not added to Node.js until v14.17.0, so we couldn't guarantee v14 users would have it. I'm not certain why using EventEmitters makes a difference memory-wise, but it does get rid of the MaxListenersExceededWarning.
#96 has been merged and deployed in v8.5.2. I'll keep this open for a couple weeks, or until someone can upgrade their transport and verify that it resolves the issue. Whichever comes first. 🖤 |
For me, the upgrade resolved the issue. |
I can also confirm that this fixed my issue. Thanks @JoshMock! 🙌 |
We all have to say a big thank you @JoshMock |
Using the connection's emitter for undici's request causes a event emitter memory leak, for each request an event handler is attached to the abort signal and never deleted and because the max listeners by default is 10 for each event, the process shows up warning fro that.
example:
There is already unresolved issues related to the same problem:
elastic/elasticsearch-js#1741
elastic/elasticsearch-js#1733
elastic/elasticsearch-js#1716
Here is a suggested Fix for it but closed unmerged
#55
The text was updated successfully, but these errors were encountered: