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

Memory leak in AbortSignal.timeout and aborted #48951

Closed
rluvaton opened this issue Jul 28, 2023 · 0 comments · Fixed by #48952
Closed

Memory leak in AbortSignal.timeout and aborted #48951

rluvaton opened this issue Jul 28, 2023 · 0 comments · Fixed by #48952
Labels
abortcontroller Issues and PRs related to the AbortController API confirmed-bug Issues with confirmed bugs.

Comments

@rluvaton
Copy link
Member

Version

20.5.0,18.16.0

Platform

Darwin razluvaXFX99QJK 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun 8 22:22:20 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T6000 arm64

Subsystem

events

What steps will reproduce the bug?

Run this with the flag --expose-gc

// Flags: --expose-gc

const {setImmediate} = require('timers/promises');
const {aborted} = require('util');

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

function logMemory() {

  const memoryData = process.memoryUsage();

  const memoryUsage = {
    rss: `${formatMemoryUsage(memoryData.rss)} -> Resident Set Size - total memory allocated for the process execution`,
    heapTotal: `${formatMemoryUsage(memoryData.heapTotal)} -> total size of the allocated heap`,
    heapUsed: `${formatMemoryUsage(memoryData.heapUsed)} -> actual memory used during the execution`,
    external: `${formatMemoryUsage(memoryData.external)} -> V8 external memory`,
  };

  console.log(memoryUsage);
}

(async () => {

  while (true) {
    for (let i = 0; i < 10000; i++) {
      function lis() {

      }

      const timeoutSignal = AbortSignal.timeout(1_000_000_000);
      timeoutSignal.addEventListener('abort', lis);
      aborted(timeoutSignal, {});
      timeoutSignal.removeEventListener('abort', lis);
    }

    await setImmediate();
    global.gc();
  }

})().catch(console.error)

setInterval(() => {
  logMemory();
}, 1000);

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

always.

required conditions are to add the aborted after the regular listener and remove the regular listener after the aborted

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

no memory leak

What do you see instead?

memory leak

Additional information

this is happening because:

  1. the regular listener goes to here which add to the map the aborted function add weak listener:
    gcPersistentSignals.add(this);
  2. calling the aborted function add the listener but as weak listener
  3. when the listener is garbage collected we call remove on the listener:
    (listener) => listener.remove(),
  4. the remove does not call the removeEventListener which decreases the size:
  5. because the size is not decreased it will never reach 0 so the abort signal won't get GCed
    if (isTimeoutOrNonEmptyCompositeSignal && type === 'abort' && size === 0) {
    gcPersistentSignals.delete(this);
    }

This is also the reason why calling aborted on the same signal and garbage collecting still emit the max listener warning

rluvaton added a commit to rluvaton/node that referenced this issue Jul 28, 2023
@atlowChemi atlowChemi added confirmed-bug Issues with confirmed bugs. abortcontroller Issues and PRs related to the AbortController API labels Jul 31, 2023
rluvaton added a commit to rluvaton/node that referenced this issue Aug 10, 2023
nodejs-github-bot pushed a commit that referenced this issue Aug 12, 2023
Fixes: #48951
PR-URL: #48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
Fixes: nodejs#48951
PR-URL: nodejs#48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
Fixes: nodejs#48951
PR-URL: nodejs#48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
RafaelGSS pushed a commit that referenced this issue Aug 15, 2023
Fixes: #48951
PR-URL: #48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this issue Aug 15, 2023
Fixes: nodejs#48951
PR-URL: nodejs#48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
rluvaton added a commit to rluvaton/node that referenced this issue Aug 15, 2023
Fixes: nodejs#48951
PR-URL: nodejs#48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
RafaelGSS pushed a commit that referenced this issue Aug 16, 2023
Fixes: #48951
PR-URL: #48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
RafaelGSS pushed a commit that referenced this issue Aug 17, 2023
Fixes: #48951
PR-URL: #48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
targos pushed a commit that referenced this issue Nov 27, 2023
Fixes: #48951
PR-URL: #48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
Fixes: nodejs/node#48951
PR-URL: nodejs/node#48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
Fixes: nodejs/node#48951
PR-URL: nodejs/node#48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
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 confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants