node icon indicating copy to clipboard operation
node copied to clipboard

AbortSignal.any() causes memory leak

Open Furzel opened this issue 1 year ago • 3 comments

Version

v22.6.0

Platform

Microsoft Windows NT 10.0.22631.0 x64
Linux ****** 4.4.0-22621-Microsoft #3672-Microsoft Fri Jan 01 08:00:00 PST 2016 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

https://nodejs.org/api/globals.html#class-abortsignal

What steps will reproduce the bug?

Run this and watch memory usage.

const formatMemoryUsage = (data) => `${Math.round(data / 1024 / 1024 * 100) / 100} MB`;

let memoryData = process.memoryUsage();
console.log('Mem before loop', formatMemoryUsage(memoryData.rss));

for (let i = 0; true; i++) {
    const abortController = new AbortController();
    const signal = abortController.signal;
    const composedSignal = AbortSignal.any([signal]);

    if (i === 1000000) {
        break;
    }
}

memoryData = process.memoryUsage();
console.log('Mem after 1 million iteration', formatMemoryUsage(memoryData.rss));

This is what I get on my local machine image

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

Always reproducible as far as I can tell

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

Memory post loop execution should be fairly equivalent to the first log but somehow the const composedSignal = AbortSignal.any([signal]); does not get cleaned up from memory, I would expect this to get cleaned properly or if this is the intended behavior to have a clear warning in the documentation.

What do you see instead?

We see a memory leak that will eventually lead to an out of memory error.

Additional information

This has been tested on Node 22.6 on different machine and both Windows + Unix versions. Happy to provide more details if needed

Furzel avatar Aug 28 '24 17:08 Furzel

This might (I'm not sure) be a duplicate of https://github.com/nodejs/node/issues/48419

kallerosenbaum avatar Aug 28 '24 17:08 kallerosenbaum

$ node repro.js 
Mem before loop 44.25 MB
Mem after 1 million iteration 1375.11 MB

avivkeller avatar Aug 28 '24 17:08 avivkeller

I started taking some snapshot

image image

@RedYetiDev would you have any clue?

geeksilva97 avatar Sep 25 '24 15:09 geeksilva97

I think the issue here is that everything is happening in the same tick. This does not leak for me:

const formatMemoryUsage = (data) => `${Math.round((data / 1024 / 1024) * 100) / 100} MB`;

let memoryData = process.memoryUsage();
console.log('Mem before loop', formatMemoryUsage(memoryData.rss));

let i = 0;
function run() {
    const abortController = new AbortController();
    const signal = abortController.signal;
    const composedSignal = AbortSignal.any([signal]);

    if (i === 1000000) {
        memoryData = process.memoryUsage();
        console.log('Mem after 1 million iteration', formatMemoryUsage(memoryData.rss));
    } else {
        i++;
        setImmediate(run);
    }
}

run();

mika-fischer avatar Oct 10 '24 14:10 mika-fischer

I find that if you combine an abort signal inside the loop with one outside, you still get the problem although not quite as severely.

const formatMemoryUsage = data =>
  `${Math.round((data / 1024 / 1024) * 100) / 100} MB`;

let memoryData = process.memoryUsage();
console.log('Mem before loop', formatMemoryUsage(memoryData.rss));

const otherAbortController = new AbortController();

let i = 0;
function run() {
  const abortController = new AbortController();
  const signal = abortController.signal;
  const composedSignal = AbortSignal.any([signal, otherAbortController.signal]);

  if (i === 1000000) {
    memoryData = process.memoryUsage();
    console.log(
      'Mem after 1 million iteration',
      formatMemoryUsage(memoryData.rss),
    );
  } else {
    i++;
    setImmediate(run);
  }
}

run();
Mem before loop 36.91 MB
Mem after 1 million iteration 797.81 MB

Even if the other abort signal is inside the loop it still ends up using more.

const formatMemoryUsage = data =>
  `${Math.round((data / 1024 / 1024) * 100) / 100} MB`;

let memoryData = process.memoryUsage();
console.log('Mem before loop', formatMemoryUsage(memoryData.rss));

for (let i = 0; true; i++) {
  await new Promise(resolve => {
    const otherAbortController = new AbortController();
    const abortController = new AbortController();
    const signal = abortController.signal;
    const composedSignal = AbortSignal.any([
      signal,
      otherAbortController.signal,
    ]);
    setImmediate(() => {
      resolve();
    });
  });

  if (i === 1000000) {
    break;
  }
}

memoryData = process.memoryUsage();
console.log('Mem after 1 million iteration', formatMemoryUsage(memoryData.rss));
Mem before loop 36.88 MB
Mem after 1 million iteration 137.63 MB
node --version
v22.13.0

Weirdly I find the implementation in https://github.com/graphql-hive/gateway/pull/922 resolves this in practical use, but in this test it doesn't seem to help much.

AlexGilleranGP avatar May 29 '25 03:05 AlexGilleranGP

I've written a test file that demostrates various variations of this bug:

import * as process from 'node:process';
import { strict } from 'assert';

const iterations = 300_000;
describe('Test memory leak in abort controller', () => {
    let before: bigint;
    beforeEach(() => {
        before = BigInt(process.memoryUsage().rss) / 1024n / 1024n;
    });

    afterEach(() => {
        const after = BigInt(process.memoryUsage().rss) / 1024n / 1024n;
        // Memory should not have increased by more than 10MB:
        strict.ok(after < before + 100n, `before: ${before}, after: ${after}`);
    });

    // These don't work
    test('AbortSignal.any with only AbortController', async () => {
        const abortController = new AbortController();
        for (let i = 0; i < iterations; i++) {
            const signal = abortController.signal;
            AbortSignal.any([signal]);
        }
    });
    test('AbortSignal.any with AbortController and timeout', async () => {
        const abortController = new AbortController();
        for (let i = 0; i < iterations; i++) {
            const signal = abortController.signal;
            const timeout = AbortSignal.timeout(1);
            AbortSignal.any([signal, timeout]);
        }
    });
    test('AbortSignal.any with only timeout', async () => {
        for (let i = 0; i < iterations; i++) {
            const timeout = AbortSignal.timeout(1);
            AbortSignal.any([timeout]);
        }
    });
    test('AbortSignal.timeout', async () => {
        for (let i = 0; i < iterations; i++) {
            AbortSignal.timeout(1);
        }
    });

    // These work!
    test('AbortSignal.abort', async () => {
        for (let i = 0; i < iterations; i++) {
            AbortSignal.abort('hello');
        }
    });
    test('AbortSignal.any with nothing', async () => {
        for (let i = 0; i < iterations; i++) {
            AbortSignal.any([]);
        }
    });
});

kallerosenbaum avatar Sep 05 '25 13:09 kallerosenbaum