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

events: remove weak listener for event target #48952

Conversation

rluvaton
Copy link
Member

@rluvaton rluvaton commented Jul 28, 2023

Fixes: #48951

  • need to add a test

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jul 28, 2023
@@ -691,6 +696,30 @@ class EventTarget {
}
}

// TODO - rename this function
removeInternalListener(type, listenerWrapper) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make this a separate, standalone function instead of adding it to the prototype (as a non-standard function)?

Copy link
Member Author

@rluvaton rluvaton Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question, Because it accesses the instance internals and I want to make it close to the other remove listener 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My general impression is that for web standards APIs, node tries to avoid exposing non-standard properties and functions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't plan to expose it as plain function, either extracting or use symbol as a key

Comment on lines 711 to 721
while (handler !== undefined) {
if (handler === listenerWrapper) {
handler.remove();
root.size--;
if (root.size === 0)
this[kEvents].delete(type);
this[kRemoveListener](root.size, type, listener, capture);
break;
}
handler = handler.next;
}
}
Copy link
Member Author

@rluvaton rluvaton Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking maybe just remove the while as we are sure that this exists unless there is some weird thing that can happen here...

@rluvaton rluvaton marked this pull request as ready for review July 28, 2023 15:35

const newMemory = getMemoryAllocatedInMB();

strictEqual(newMemory - currentMemory < 100, true, new Error(`After consuming 10 items the memory increased by ${Math.floor(newMemory - currentMemory)}MB`));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to test 🤔 feels like this could be flakey

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question but to reduce flakiness this test would be to not run in parallel with other tests, will move it to a different file

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this 100 sounds like a magic number which would not be truly reliable... (I don't have a better idea of how to test this though)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the test as it did not work for some reason...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rluvaton I wrote this in another conversation, but I feel it belongs here...
Maybe using a WeakRef[] etc we can ensure the values are gone after globalThis.gc();, instead of checking an increase in memory? (meaning add them wrapped by WeakRef into an array during the createALotOfAbortSignals setup)

Copy link
Member Author

@rluvaton rluvaton Jul 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great idea, the test was added :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the test as it can be flaky and instead checked that the memory leak warning was not emitted...

@debadree25
Copy link
Member

Also cc @nodejs/events

@rluvaton rluvaton marked this pull request as draft July 30, 2023 13:20
@rluvaton rluvaton marked this pull request as ready for review July 30, 2023 16:05
test/parallel/test-abortcontroller.js Outdated Show resolved Hide resolved
test/parallel/test-abortcontroller.js Outdated Show resolved Hide resolved
@@ -231,6 +233,30 @@ const { setTimeout: sleep } = require('timers/promises');
AbortSignal.timeout(1_200_000);
}

{
(async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why this requires the async iife wrapper, or the sleeps

Copy link
Member Author

@rluvaton rluvaton Jul 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without the sleep, the test would fail (as I assume the GC happen in a different thread when having more than 1 core)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rluvaton so is 10ms trust-worthy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used 10ms like the rest

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

increased to 100ms and hope the test now pass

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced the test with another one that check that the memory leak warning not emitted which would be faster and have the same result

@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 31, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 31, 2023
@nodejs-github-bot
Copy link
Collaborator

@atlowChemi
Copy link
Member

@rluvaton the test seems to be failing 😕

not ok 244 parallel/test-abortcontroller
  ---
  duration_ms: 244.52900
  severity: fail
  exitcode: 1
  stack: |-
    node:internal/process/promises:289
                triggerUncaughtException(err, true /* fromPromise */);
                ^
    
    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    + actual - expected
    
    + AbortSignal {
    +   [Symbol(events.maxEventTargetListeners)]: 10,
    +   [Symbol(events.maxEventTargetListenersWarned)]: false,
    +   [Symbol(kAborted)]: false,
    +   [Symbol(kComposite)]: false,
    +   [Symbol(kEvents)]: SafeMap(1) [Map] {
    +     'abort' => <ref *1> {
    +       next: Listener {
    +         callback: SafeWeakRef [WeakRef] {},
    +         flags: 81,
    +         listener: SafeWeakRef [WeakRef] {},
    +         next: undefined,
    +         previous: [Circular *1]
    +       },
    +       resistStopPropagation: true,
    +       size: 1
    +     }
    +   },
    +   [Symbol(kHandlers)]: SafeMap(0) [Map] {},
    +   [Symbol(kReason)]: undefined,
    +   [Symbol(kTimeout)]: true
    + }
    - undefined
        at /home/iojs/build/workspace/node-test-commit-linux/test/parallel/test-abortcontroller.js:256:5 {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: AbortSignal {
        [Symbol(kEvents)]: SafeMap(1) [Map] {
          'abort' => <ref *1> {
            size: 1,
            next: Listener {
              next: undefined,
              previous: [Circular *1],
              listener: SafeWeakRef [WeakRef] {},
              flags: 81,
              callback: SafeWeakRef [WeakRef] {}
            },
            resistStopPropagation: true
          }
        },
        [Symbol(events.maxEventTargetListeners)]: 10,
        [Symbol(events.maxEventTargetListenersWarned)]: false,
        [Symbol(kHandlers)]: SafeMap(0) [Map] {},
        [Symbol(kAborted)]: false,
        [Symbol(kReason)]: undefined,
        [Symbol(kComposite)]: false,
        [Symbol(kTimeout)]: true
      },
      expected: undefined,
      operator: 'strictEqual'
    }
    
    Node.js v21.0.0-pre
  ...

@atlowChemi atlowChemi added the abortcontroller Issues and PRs related to the AbortController API label Jul 31, 2023
@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

[kWeakHandler]: {},
});

await setTimeout(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?

Suggested change
await setTimeout(0);
await Promise.resolve();

Copy link
Member Author

@rluvaton rluvaton Aug 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it won't GC with this

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@atlowChemi atlowChemi added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. events Issues and PRs related to the events subsystem / EventEmitter. and removed abortcontroller Issues and PRs related to the AbortController API labels Aug 12, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2023
@nodejs-github-bot nodejs-github-bot merged commit c39f04c into nodejs:main Aug 12, 2023
36 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c39f04c

@rluvaton rluvaton deleted the fix-memory-leak-in-abort-signal-and-weak-listeners branch August 12, 2023 18:22
Ceres6 pushed a commit to Ceres6/node that referenced this pull request 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 pull request 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 pull request Aug 15, 2023
Fixes: #48951
PR-URL: #48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request 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 pull request 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 pull request Aug 16, 2023
Fixes: #48951
PR-URL: #48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
Fixes: #48951
PR-URL: #48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
@gandazgul
Copy link

Was this included in, or is planned to be included in v18?

@atlowChemi
Copy link
Member

Was this included in, or is planned to be included in v18?

This was released just in v20.6.0, which means it will take some time to reach v18 which is LTS, but hopefully it should happen soon 🙂

targos pushed a commit that referenced this pull request 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 pull request 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 pull request 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
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. events Issues and PRs related to the events subsystem / EventEmitter. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak in AbortSignal.timeout and aborted
8 participants