remove the event listener from the { signal } option after the request finishes #51429

vampirefrog · 2024-01-10T19:16:16Z

Version

v18.13.0

Platform

Linux vampi 6.1.0-16-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.67-1 (2023-12-12) x86_64 GNU/Linux

Subsystem

fetch

What steps will reproduce the bug?

When trying to reuse the same AbortSignal and AbortController, as the { signal } option for fetch(), after about 10 requests, a warning is generated

How often does it reproduce? Is there a required condition?

node --trace-warnings -e '(async() => { const c = new AbortController(); for(let i = 0; i < 20; i++) { await fetch("https://google.com", { signal: c.signal }).then(r => r.text()).then(r => console.log(r.substring(0, 100))); }})()'

<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
(node:749969) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit
    at [kNewListener] (node:internal/event_target:514:17)
    at [kNewListener] (node:internal/abort_controller:183:24)
    at EventTarget.addEventListener (node:internal/event_target:623:23)
    at new Request (node:internal/deps/undici/undici:7294:20)
    at fetch2 (node:internal/deps/undici/undici:15371:25)
    at Object.fetch (node:internal/deps/undici/undici:16287:18)
    at fetch (node:internal/process/pre_execution:237:25)
    at [eval]:1:84
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content
<!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="ro"><head><meta content

What is the expected behavior? Why is that the expected behavior?

Expected behavior is that the AbortSignal event listener should be released with .removeEventListener(). This cannot be done outside of fetch() since there's no way to clear all the event listeners for an EventTarget, which AbortSignal inherits, or all the event listeners for a given event. One can only clear the event listener for a given event and callback function.

In fact, if you paste the same code in the browser:

(async() => { const c = new AbortController(); for(let i = 0; i < 20; i++) { await fetch("https://google.com", { signal: c.signal }).then(r => r.text()).then(r => console.log(r.substring(0, 100))); }})()

It works just fine and there is no warning.

What do you see instead?

MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal].

It adds multiple event listeners instead of a single one.

Additional information

I am not sure if this is a fetch() issue, an EventTarget issue (adding the same function with the same event multiple times), and I'm also not sure if this is the right repo to report this issue, so please correct me if I'm wrong.

The text was updated successfully, but these errors were encountered:

KhafraDev · 2024-01-11T03:18:02Z

you can increase the amount of listeners before getting a warning by using https://nodejs.org/api/events.html#eventssetmaxlistenersn-eventtargets

vampirefrog · 2024-01-11T09:35:34Z

you can increase the amount of listeners before getting a warning by using https://nodejs.org/api/events.html#eventssetmaxlistenersn-eventtargets

The point of this bug report is that there should only be one listener, which should be released after fetch() finishes, not multiple ones, so increasing the max listeners is akin to increasing the memory limit when you have a memory leak: it doesn't fix the initial problem. Your comment misses the point.

marco-ippolito · 2024-01-11T10:46:40Z

I can reproduce on 18.13 but not on 18.18 meaning it probably has been fixed in the latest 18 version.
Also your code is leaking memory because the fetch response is not being GC, we can see memory increasing each iteration, that's why its increasing the number of listeners.

(async () => {
    const c = new AbortController();
    for (let i = 0; i < 20; i++) {
        await fetch("https://google.com", { signal: c.signal }).then(r => r.text()).then(r => console.log(r.substring(0, 100)));
        console.log(process.memoryUsage().rss)
    }
})()

vampirefrog · 2024-01-11T12:35:07Z

I can reproduce on 18.13 but not on 18.18 meaning it probably has been fixed in the latest 18 version. Also your code is leaking memory because the fetch response is not being GC, we can see memory increasing each iteration, that's why its increasing the number of listeners.
(async () => {
    const c = new AbortController();
    for (let i = 0; i < 20; i++) {
        await fetch("https://google.com", { signal: c.signal }).then(r => r.text()).then(r => console.log(r.substring(0, 100)));
        console.log(process.memoryUsage().rss)
    }
})()
78053376
78528512
78626816
79380480
79904768
81887232
82083840
82673664
83394560
83509248
83558400
83591168
83623936
83705856
...

I don't see how "that's why it's increasing the number of listeners" follows from "we can see the memory increasing each iteration". Sounds to me like a non sequitur. It's not a memory leak that's causing the increase of the number of listeners, but rather, the unlimited listeners generate a memory leak. So I'm not sure what you meant.

But I have, indeed, tested with v19.9.0 and the warning is no longer generated.

On the other hand, your example, if you remove the AbortController code and the signal code, the apparent memory leak is still there, so it seems to have nothing to do with signals or aborting. I've tried on v18.13.0 and v19.9.0.

(async () => {
    for (let i = 0; i < 20; i++) {
        await fetch("https://google.com").then(r => r.text()).then(r => console.log(r.substring(0, 100)));
        console.log(process.memoryUsage().rss)
    }
})()

Goes from 79597568 to 89481216 on v19.9.0. I am not sure how to react to this, is this a bug or is it to be expected? Seems to be a memory leak in fetch(), separate from the AbortSignal issue that I initially presented. I might just file another issue with regard to that.

marco-ippolito · 2024-01-11T13:35:08Z

The behavior of fetch is ~~kinda~~ not expected, please read the link I posted in the previous comment.
https://fetch.spec.whatwg.org/#garbage-collection
If fetch never gets garbage collected you keep adding listeners to the abort controller without removing them, thats why at some point it will warn you that you are exceeding max number.
Every time you use fetch in the loop you are adding an eventlistener to the signal, and since its never garbage collected, the listener is never removed.
The solution to the warning is to updated to a later version where the issue is fixed.

vampirefrog · 2024-01-11T14:43:33Z

I'm sorry for not seeing the link earlier, but I've taken a look and I believe it refers exclusively to an "ongoing fetch", in other words a HTTP request that is still sending or receiving data through the TCP socket:

The user agent may terminate an ongoing fetch if that termination is not observable through script.

Whereas I am executing fetches all the way through, in other words I am awaiting for them to finish before executing the next one, and I don't keep any references to the previous fetch() before moving on to the next one.

Also, I'm not the one who keeps "adding listeners to the abort controller", it's the fetch() call who is adding listeners, which itself finishes and should be unobservable, or unreferenced. I just give it a reference to the signal.

I'm going to close this particular issue because the warning no longer occurs in v19.9.0, and I might open another issue related to what seems to be an undocumented memory leak with fetch. Either that or stop using fetch in my code at all and use something else.

marco-ippolito · 2024-01-11T14:48:22Z

There is some more discussion here #46435 and https://undici.nodejs.org/#/?id=specification-compliance

vampirefrog closed this as completed Jan 11, 2024

vampirefrog mentioned this issue Jan 11, 2024

memory leak in fetch() #51438

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove the event listener from the { signal } option after the request finishes #51429

remove the event listener from the { signal } option after the request finishes #51429

vampirefrog commented Jan 10, 2024

KhafraDev commented Jan 11, 2024

vampirefrog commented Jan 11, 2024

marco-ippolito commented Jan 11, 2024 •

edited

Loading

vampirefrog commented Jan 11, 2024

marco-ippolito commented Jan 11, 2024 •

edited

Loading

vampirefrog commented Jan 11, 2024 •

edited

Loading

marco-ippolito commented Jan 11, 2024 •

edited

Loading

remove the event listener from the { signal } option after the request finishes #51429

remove the event listener from the { signal } option after the request finishes #51429

Comments

vampirefrog commented Jan 10, 2024

Version

Platform

Subsystem

What steps will reproduce the bug?

How often does it reproduce? Is there a required condition?

What is the expected behavior? Why is that the expected behavior?

What do you see instead?

Additional information

KhafraDev commented Jan 11, 2024

vampirefrog commented Jan 11, 2024

marco-ippolito commented Jan 11, 2024 • edited Loading

vampirefrog commented Jan 11, 2024

marco-ippolito commented Jan 11, 2024 • edited Loading

vampirefrog commented Jan 11, 2024 • edited Loading

marco-ippolito commented Jan 11, 2024 • edited Loading

marco-ippolito commented Jan 11, 2024 •

edited

Loading

marco-ippolito commented Jan 11, 2024 •

edited

Loading

vampirefrog commented Jan 11, 2024 •

edited

Loading

marco-ippolito commented Jan 11, 2024 •

edited

Loading