search

elastic / elastic-transport-js

Transport classes and utilities shared among Node.js Elastic client libraries
Apache License 2.0
4 stars 24 forks source link

fix: Use an AbortSignal for per-request timeouts #96

Closed JoshMock closed 1 month ago

JoshMock commented 2 months ago

A potential fix for https://github.com/elastic/elastic-transport-js/issues/63, largely inspired by a community member's PR that was never merged: https://github.com/elastic/elastic-transport-js/pull/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.

JoshMock commented 2 months ago

@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. :pray:

JoshMock commented 2 months ago

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 https://github.com/nodejs/undici/issues/165 but the functionality never shipped, so he added similar logic to do the same thing here.

https://github.com/elastic/elastic-transport-js/blob/b006b9a3e9398faec51bcc834350ae978437ba99/src/connection/UndiciConnection.ts#L130-L142

alexey-sh commented 1 month ago

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

JoshMock commented 1 month ago

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.