events: remove weak listener for event target #48952

rluvaton · 2023-07-28T14:56:00Z

need to add a test

mscdex · 2023-07-28T15:07:36Z

lib/internal/event_target.js

@@ -691,6 +696,30 @@ class EventTarget {
    }
  }

+  // TODO - rename this function
+  removeInternalListener(type, listenerWrapper) {


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

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

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

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

lib/internal/event_target.js

rluvaton · 2023-07-28T15:31:11Z

lib/internal/event_target.js

+    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;
+    }
+  }


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...

debadree25 · 2023-07-29T18:03:52Z

test/parallel/test-abortcontroller.js

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


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

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

#48952 (comment)

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)

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

@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)

great idea, the test was added :)

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

debadree25 · 2023-07-29T18:05:03Z

Also cc @nodejs/events

test/parallel/test-abortsignal-timeout.js

test/parallel/test-abortcontroller.js

atlowChemi · 2023-07-30T18:22:07Z

test/parallel/test-abortcontroller.js

@@ -231,6 +233,30 @@ const { setTimeout: sleep } = require('timers/promises');
  AbortSignal.timeout(1_200_000);
 }

+{
+  (async () => {


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

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

@rluvaton so is 10ms trust-worthy?

I used 10ms like the rest

increased to 100ms and hope the test now pass

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

nodejs-github-bot · 2023-07-31T07:35:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53013/

atlowChemi · 2023-07-31T13:42:11Z

@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
  ...

lib/internal/event_target.js

nodejs-github-bot · 2023-08-01T20:47:47Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53033/

nodejs-github-bot · 2023-08-02T18:41:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53052/

atlowChemi · 2023-08-05T20:06:38Z

test/parallel/test-eventtarget-memoryleakwarning.js

+        [kWeakHandler]: {},
+      });
+
+      await setTimeout(0);


Why not?

Suggested change

await setTimeout(0);

await Promise.resolve();

because it won't GC with this

test/parallel/test-eventtarget-memoryleakwarning.js

nodejs-github-bot · 2023-08-11T10:42:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53183/

nodejs-github-bot · 2023-08-11T13:42:52Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53191/

nodejs-github-bot · 2023-08-11T14:43:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53196/

nodejs-github-bot · 2023-08-12T17:33:51Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53239/

nodejs-github-bot · 2023-08-12T18:20:56Z

Landed in c39f04c

Fixes: nodejs#48951 PR-URL: nodejs#48952 Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Fixes: #48951 PR-URL: #48952 Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Fixes: nodejs#48951 PR-URL: nodejs#48952 Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Fixes: #48951 PR-URL: #48952 Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

gandazgul · 2023-09-18T15:35:45Z

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

atlowChemi · 2023-09-18T17:01:00Z

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 🙂

Fixes: #48951 PR-URL: #48952 Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Fixes: nodejs/node#48951 PR-URL: nodejs/node#48952 Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jul 28, 2023

mscdex reviewed Jul 28, 2023

View reviewed changes

rluvaton commented Jul 28, 2023

View reviewed changes

lib/internal/event_target.js Outdated Show resolved Hide resolved

rluvaton commented Jul 28, 2023

View reviewed changes

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

debadree25 reviewed Jul 29, 2023

View reviewed changes

rluvaton commented Jul 29, 2023

View reviewed changes

test/parallel/test-abortsignal-timeout.js Outdated Show resolved Hide resolved

atlowChemi reviewed Jul 30, 2023

View reviewed changes

test/parallel/test-abortsignal-timeout.js Outdated Show resolved Hide resolved

atlowChemi reviewed Jul 30, 2023

View reviewed changes

test/parallel/test-abortsignal-timeout.js Outdated Show resolved Hide resolved

rluvaton marked this pull request as draft July 30, 2023 13:20

rluvaton marked this pull request as ready for review July 30, 2023 16:05

atlowChemi reviewed Jul 30, 2023

View reviewed changes

atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 31, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 31, 2023

atlowChemi added the abortcontroller Issues and PRs related to the AbortController API label Jul 31, 2023

github-actions bot mentioned this pull request Aug 1, 2023

CI Reliability 2023-08-01 nodejs/reliability#630

Open

21 tasks

atlowChemi reviewed Aug 1, 2023

View reviewed changes

lib/internal/event_target.js Outdated Show resolved Hide resolved

atlowChemi requested a review from benjamingr August 1, 2023 20:42

atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2023

github-actions bot mentioned this pull request Aug 2, 2023

CI Reliability 2023-08-02 nodejs/reliability#631

Open

24 tasks

This was referenced Aug 3, 2023

CI Reliability 2023-08-03 nodejs/reliability#632

Open

CI Reliability 2023-08-04 nodejs/reliability#633

Open

atlowChemi reviewed Aug 5, 2023

View reviewed changes

test/parallel/test-eventtarget-memoryleakwarning.js Outdated Show resolved Hide resolved

atlowChemi approved these changes Aug 12, 2023

View reviewed changes

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 merged commit c39f04c into nodejs:main Aug 12, 2023
64 checks passed

rluvaton deleted the fix-memory-leak-in-abort-signal-and-weak-listeners branch August 12, 2023 18:22

This was referenced Aug 13, 2023

CI Reliability 2023-08-13 nodejs/reliability#635

Open

CI Reliability 2023-08-14 nodejs/reliability#636

Open

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023

events: remove weak listener for event target

4bf139e

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

events: remove weak listener for event target

b1a29cf

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

events: remove weak listener for event target

fd6885b

Fixes: #48951 PR-URL: #48952 Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

UlisesGascon mentioned this pull request Aug 15, 2023

v20.6.0 proposal #49185

Merged

RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023

events: remove weak listener for event target

b822792

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

events: remove weak listener for event target

09aac82

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

events: remove weak listener for event target

eb6e6fc

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

events: remove weak listener for event target

d568600

Fixes: #48951 PR-URL: #48952 Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

targos pushed a commit that referenced this pull request Nov 27, 2023

events: remove weak listener for event target

acd9872

Fixes: #48951 PR-URL: #48952 Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024

events: remove weak listener for event target

513495a

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

events: remove weak listener for event target

fb734ce

Fixes: nodejs/node#48951 PR-URL: nodejs/node#48952 Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

events: remove weak listener for event target #48952

events: remove weak listener for event target #48952

rluvaton commented Jul 28, 2023 •

edited

mscdex Jul 28, 2023

rluvaton Jul 28, 2023 •

edited

mscdex Jul 28, 2023

rluvaton Jul 28, 2023

rluvaton Jul 28, 2023 •

edited

debadree25 Jul 29, 2023

rluvaton Jul 29, 2023

rluvaton Jul 29, 2023

atlowChemi Jul 30, 2023

rluvaton Jul 30, 2023

atlowChemi Jul 30, 2023

rluvaton Jul 30, 2023 •

edited

rluvaton Aug 2, 2023

debadree25 commented Jul 29, 2023

atlowChemi Jul 30, 2023

rluvaton Jul 30, 2023 •

edited

atlowChemi Jul 30, 2023

rluvaton Aug 1, 2023

rluvaton Aug 1, 2023

rluvaton Aug 2, 2023

nodejs-github-bot commented Jul 31, 2023

atlowChemi commented Jul 31, 2023

nodejs-github-bot commented Aug 1, 2023

nodejs-github-bot commented Aug 2, 2023

atlowChemi Aug 5, 2023

rluvaton Aug 5, 2023 •

edited

nodejs-github-bot commented Aug 11, 2023

nodejs-github-bot commented Aug 11, 2023

nodejs-github-bot commented Aug 11, 2023

nodejs-github-bot commented Aug 12, 2023

nodejs-github-bot commented Aug 12, 2023

gandazgul commented Sep 18, 2023

atlowChemi commented Sep 18, 2023


		const newMemory = getMemoryAllocatedInMB();

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

events: remove weak listener for event target #48952

events: remove weak listener for event target #48952

Conversation

rluvaton commented Jul 28, 2023 • edited

Choose a reason for hiding this comment

rluvaton Jul 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rluvaton Jul 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rluvaton Jul 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debadree25 commented Jul 29, 2023

Choose a reason for hiding this comment

rluvaton Jul 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 31, 2023

atlowChemi commented Jul 31, 2023

nodejs-github-bot commented Aug 1, 2023

nodejs-github-bot commented Aug 2, 2023

Choose a reason for hiding this comment

rluvaton Aug 5, 2023 • edited

Choose a reason for hiding this comment

nodejs-github-bot commented Aug 11, 2023

nodejs-github-bot commented Aug 11, 2023

nodejs-github-bot commented Aug 11, 2023

nodejs-github-bot commented Aug 12, 2023

nodejs-github-bot commented Aug 12, 2023

gandazgul commented Sep 18, 2023

atlowChemi commented Sep 18, 2023

rluvaton commented Jul 28, 2023 •

edited

rluvaton Jul 28, 2023 •

edited

rluvaton Jul 28, 2023 •

edited

rluvaton Jul 30, 2023 •

edited

rluvaton Jul 30, 2023 •

edited

rluvaton Aug 5, 2023 •

edited