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

fetch with AbortController signal memory leak (still) #52203

Closed
shigma opened this issue Mar 24, 2024 · 7 comments
Closed

fetch with AbortController signal memory leak (still) #52203

shigma opened this issue Mar 24, 2024 · 7 comments
Labels
abortcontroller Issues and PRs related to the AbortController API fetch Issues and PRs related to the Fetch API

Comments

@shigma
Copy link
Contributor

shigma commented Mar 24, 2024

Version

20.11.0

Platform

Microsoft Windows NT 10.0.22631.0 x64

Subsystem

undici

What steps will reproduce the bug?

There was an issue #48478 with repoduction https://github.com/Robert-Schirmer/nodejs-mem-leak, and it is not reproducible.

However AbortController still leaks in terms of FinalizationRegistry:

const r = new FinalizationRegistry((tag) => console.log(tag));

function alloc() {
  let counter = 0;
  (function allocateMemory() {
    Array.from({ length: 50000 }, () => () => {});
    if (counter > 5000) return;
    counter++;
    setTimeout(allocateMemory);
  })();
}

function test1() {
  let x = ['foo', 'bar']
  r.register(x, 'test1')
  x.length
}

async function test2() {
  const ac = new AbortController()
  r.register(ac, 'test2')
  await fetch('https://nodejs.org', {
    signal: ac.signal,
  })
}

test1() // test1
test2() // no output
alloc()

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

Always (with node 20.11.0 native fetch).

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

Expected output:

test1
test2

What do you see instead?

Actual output:

test1

Additional information

No response

@shigma
Copy link
Contributor Author

shigma commented Mar 24, 2024

Here are some background information. (It has nothing to do with the issue itself, but explains why I need FinalizationRegistry. I would be really glad if someone could provide other solution to my problem.)

I use AbortController to control my fetch process. Different conditions may trigger the abort.

const ac = new AbortController()
const timer = setTimeout(() => ac.abort(), ms)
// add other abort conditions
const resp = await fetch(url, {
  signal: ac.signal,
})

Obviously, the timeout should be cleared when the request is done or aborted:

function request() {
  const ac = new AbortController()
  const timer = setTimeout(() => ac.abort(), ms)
  // add other abort conditions
  ac.signal.addEventListener("abort", () => {
    clearTimeout(timer)
  })
  const resp = await fetch(url, {
    signal: ac.signal,
  })
  ac.abort()
  return resp.body
}

I thought this should be work, but it didn't. ac.abort() not only terminates the fetch process but the body stream as well. Now I no longer can read from the body stream. (Why?)

So there must be something in the stream that uses ac, or rather referencing ac. The only way to safely clear the side effects is to dispose the ac when it is garbage collected.

However, in the above case it never is.

@mertcanaltin mertcanaltin added the fetch Issues and PRs related to the Fetch API label Mar 26, 2024
@aduh95
Copy link
Contributor

aduh95 commented Mar 30, 2024

Consider using AbortSignal.timeout().

ac.abort() not only terminates the fetch process but the body stream as well. Now I no longer can read from the body stream. (Why?)

That's how the spec is written: “If response’s body is non-null and is readable, then error response’s body with error.“ Source

However AbortController still leaks in terms of FinalizationRegistry

I think that's not enough to claim for a memory leak, the spec doesn't require the JS engines to call the cleanup callback. See Notes on cleanup callbacks for more information. As said over there, using FinalizationRegistry for essential program logic is not a good idea.

@shigma
Copy link
Contributor Author

shigma commented Mar 31, 2024

@aduh95 Thanks for your response and information.

Consider using AbortSignal.timeout().

I know, but my actual scenario is more complex and needs to accept another AbortController passed by the user side and mix it with other abort logic.

As said over there, using FinalizationRegistry for essential program logic is not a good idea.

I do not use FinalizationRegistry in the production. It is only used to show that fetch leasks.

My real point is:

  1. AbortController often involves creating / occupying resources (such as registering event hooks).
  2. These resources need to be destoryed at the appropriate time, otherwise memory will leak quickly (because fetch may be a very frequent operation).
  3. We don’t know when an AbortController stops being used:
    1. The fulfilment of a fetch promise does not imply that AbortController can be destoryed;
    2. We will not be notified when the stream is completely consumed;
    3. Even as the last (and bad) resort, AbortController cannot been detected by FinalizationRegistry for GC.
  4. In conclusion: all above resources cannot be destroyed.

@aduh95
Copy link
Contributor

aduh95 commented Mar 31, 2024

Any chance you could produce a code sample that demonstrates the memory leak without using FinalizationRegistry?

@shigma
Copy link
Contributor Author

shigma commented Apr 1, 2024

// `stop` event may be dispatched by user, which will terminate any ongoing requests.
const ee = new EventEmitter()

for (let i = 0; i < 1000; i++) {
  const ac = new AbortController()
  const listener = () => ac.abort()
  ee.on('stop', listener)       // memory leak here
  await fetch(url, {
    signal: ac.signal,
  })
  // ee.off('stop', listener)   // we cannot do this, because it will destory the read stream
}
(node:33352) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 stop listeners added to [EventEmitter]. Use emitter.setMaxListeners() to increase limit    
(Use `node --trace-warnings ...` to show where the warning was created)

In the above case, the leak does not go directly from AbortController.

But as we do not know when an AbortController stops being used, we can not fix it by adding ee.off() anywhere.

@Uzlopak
Copy link
Contributor

Uzlopak commented May 7, 2024

@mcollina

Can this be closed?

@mcollina
Copy link
Member

mcollina commented May 7, 2024

latest undici fixes it, and it landed in Node.js v22. Will be backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abortcontroller Issues and PRs related to the AbortController API fetch Issues and PRs related to the Fetch API
Projects
None yet
Development

No branches or pull requests

6 participants