Skip to content
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

Fix MaxListenersExceededWarnings #55

Closed
wants to merge 1 commit into from
Closed

Fix MaxListenersExceededWarnings #55

wants to merge 1 commit into from

Conversation

skarahoda
Copy link

@skarahoda skarahoda commented Oct 4, 2022

Using the connection's emitter for undici's request causes a memory leak. In this PR, a local abort signal is used to fix the issue.

Related Issues:
elastic/elasticsearch-js#1741
elastic/elasticsearch-js#1733
elastic/elasticsearch-js#1716

@phil-nelson-bt
Copy link

We are getting this error (warning) from EventEmitter shortly after every time a k8s pod starts. Our current client / server version is 7.17 but I tried with the 8.6 client too. With 8.6 the message changes a little in the stack trace, but the result is still another log in our error logs

(node:91202) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 abort listeners added to [EventEmitter]. Use emitter.setMaxListeners() to increase limit
    at _addListener (node:events:587:17)
    at EventEmitter.addListener (node:events:605:10)
    at addSignal (/Users/philnelson/Projects/productivitysuiteservice/node_modules/undici/lib/api/abort-signal.js:35:19)
    at new RequestHandler (/Users/philnelson/Projects/productivitysuiteservice/node_modules/undici/lib/api/api-request.js:63:5)
    at Pool.request (/Users/philnelson/Projects/productivitysuiteservice/node_modules/undici/lib/api/api-request.js:193:25)
    at /Users/philnelson/Projects/productivitysuiteservice/node_modules/undici/lib/api/api-request.js:186:15
    at new Promise (<anonymous>)
    at Pool.request (/Users/philnelson/Projects/productivitysuiteservice/node_modules/undici/lib/api/api-request.js:185:12)
    at Connection.request (/Users/philnelson/Projects/productivitysuiteservice/node_modules/@elastic/transport/lib/connection/UndiciConnection.js:143
    ```

@madispuk
Copy link

madispuk commented Mar 9, 2023

Any updates when this fix will be merged in? We are also running into this issue.

@skarahoda skarahoda closed this by deleting the head repository Mar 24, 2023
@GuillaumeCisco
Copy link

Why closing it? Has it been fixed in another PR?

@ahmedsamir-dev
Copy link

@skarahoda
I just want to inquire has it been fixed ?

@JoshMock
Copy link
Member

JoshMock commented Apr 6, 2023

I'm taking over as the maintainer of this repo and getting familiar with it. Hoping to be able to address this issue soon. I'll track progress in #63.

JoshMock added a commit that referenced this pull request May 1, 2024
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.
JoshMock added a commit that referenced this pull request May 21, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants