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: Use an AbortSignal for per-request timeouts #96

Merged
merged 1 commit into from
May 21, 2024
Merged

Conversation

JoshMock
Copy link
Member

@JoshMock JoshMock commented 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 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.

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
Copy link
Member Author

JoshMock commented May 6, 2024

@ronag Since you were helpful in digging into this issue with your knowledge of Undici, I'd love a quick review from you if you have the time. 🙏

Copy link
Contributor

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I'm unsure why you don't just pass null if there is no signal?

@JoshMock
Copy link
Member Author

JoshMock commented May 6, 2024

I'm unsure why you don't just pass null if there is no signal?

Because the code implements per-request timeout functionality, so it needs a signal to manually abort the request when the timeout is reached. @delvedor discussed adding this to the Undici library nodejs/undici#165 but the functionality never shipped, so he added similar logic to do the same thing here.

// undici does not support per-request timeouts,
// to address this issue, we default to the constructor
// timeout (which is handled by undici) and create a local
// setTimeout callback if the request-specific timeout
// is different from the constructor timeout.
let timedout = false
let timeoutId
if (options.timeout != null && options.timeout !== this.timeout) {
timeoutId = setTimeout(() => {
timedout = true
requestParams.signal.dispatchEvent(new Event('abort'))
}, options.timeout)
}

@alexey-sh
Copy link

Does it breaks code which uses emitter(which is removed)?

@JoshMock
Copy link
Member Author

Does it breaks code which uses emitter(which is removed)?

The use of EventEmitter is an internal implementation detail, not something exposed to end users, so removing it does not break the public contract.

@JoshMock JoshMock merged commit 063a4bb into main May 21, 2024
12 checks passed
@JoshMock JoshMock deleted the abort-signal-fix branch May 21, 2024 15:16
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.

3 participants