lib: add option to force handling stopped events #48301

atlowChemi · 2023-06-02T13:57:03Z

Refs: whatwg/dom#1152 (comment)

nodejs-github-bot · 2023-06-02T13:57:07Z

Review requested:

@nodejs/http2
@nodejs/net
@nodejs/streams
@nodejs/test_runner

lib/child_process.js

lib/dgram.js

lib/events.js

lib/fs.js

lib/internal/abort_controller.js

lib/readline.js

lib/readline/promises.js

lib/timers/promises.js

benjamingr · 2023-06-03T08:02:32Z

Good job!

Some comments:

It's forcing the run of a listener. In window.onsomething = foo foo is an event handler. In window.addEventListener("something", bar) bar is an event listener. What about kAlwaysRun?
I would prefer it if we split adding it to EventTarget from changing all the usages.
We need to run the EventTarget benchmark on this to make sure the changes with iterationCondition don't have a negative perf impact.

atlowChemi · 2023-06-03T19:35:26Z

lib/internal/event_target.js

        return;
      }
      // TODO(benjamingr) make this weak somehow? ideally the signal would
      // not prevent the event target from GC.


@benjamingr Is this comment still relevant? seems to me like this was handled already

lib/internal/event_target.js

MoLow · 2023-06-03T19:38:02Z

+1 on the concerns raised by @benjamingr, I have suggested another name.
also, I would expect to see some tests that break before this fix

nodejs-github-bot · 2023-06-04T03:19:55Z

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

lib/internal/event_target.js

benjamingr · 2023-06-20T21:34:15Z

@ronag @jasnell @mcollina this is a notable behavior change there previously stopImmediatePropagation would prevent core APIs passed signal from being cleaned up.

nodejs-github-bot · 2023-06-21T07:50:56Z

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

nodejs-github-bot · 2023-06-21T13:35:34Z

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

nodejs-github-bot · 2023-06-22T07:25:25Z

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

nodejs-github-bot · 2023-06-22T12:37:10Z

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

nodejs-github-bot · 2023-06-22T13:38:04Z

Landed in bcd35c3

PR-URL: #48301 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: nodejs#48301 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

ruyadorno · 2023-09-10T03:08:55Z

This commit does not land cleanly on v18.x-staging and will need manual backport in case we want it in v18.

PR-URL: nodejs#48301 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

atlowChemi · 2023-09-10T07:34:48Z

@ruyadorno opened a backport at #49587 🙂

PR-URL: #48301 Backport-PR-URL: #49587 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

atlowChemi requested a review from benjamingr June 2, 2023 13:57

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jun 2, 2023

ljharb reviewed Jun 2, 2023

View reviewed changes

atlowChemi commented Jun 3, 2023

View reviewed changes

MoLow reviewed Jun 3, 2023

View reviewed changes

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

atlowChemi force-pushed the avoid-signals-not-working-due-to-propogation-stopped branch 4 times, most recently from 9bbebcc to 8218ad4 Compare June 3, 2023 22:40

atlowChemi marked this pull request as ready for review June 3, 2023 22:42

atlowChemi added eventtarget Issues and PRs related to the EventTarget implementation. request-ci Add this label to start a Jenkins CI on a PR. labels Jun 4, 2023

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

This was referenced Jun 5, 2023

CI Reliability 2023-06-05 nodejs/reliability#586

Open

CI Reliability 2023-06-06 nodejs/reliability#587

Open

CI Reliability 2023-06-09 nodejs/reliability#588

Open

atlowChemi force-pushed the avoid-signals-not-working-due-to-propogation-stopped branch from 8218ad4 to d75ff43 Compare June 13, 2023 13:14

atlowChemi force-pushed the avoid-signals-not-working-due-to-propogation-stopped branch from d75ff43 to fa4dc42 Compare June 20, 2023 15:00

ljharb reviewed Jun 20, 2023

View reviewed changes

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

lib: add option to force handling stopped events

061ea54

atlowChemi force-pushed the avoid-signals-not-working-due-to-propogation-stopped branch from fa4dc42 to 061ea54 Compare June 20, 2023 19:04

benjamingr approved these changes Jun 20, 2023

View reviewed changes

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 20, 2023

atlowChemi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 20, 2023

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

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

MoLow approved these changes Jun 21, 2023

View reviewed changes

atlowChemi added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2023

nodejs-github-bot merged commit bcd35c3 into nodejs:main Jun 22, 2023

atlowChemi deleted the avoid-signals-not-working-due-to-propogation-stopped branch June 22, 2023 14:16

This was referenced Jun 23, 2023

CI Reliability 2023-06-23 nodejs/reliability#596

Open

CI Reliability 2023-06-24 nodejs/reliability#597

Open

atlowChemi mentioned this pull request Jun 25, 2023

lib, net, child_process, streams, dgram, readline, http2: Use addAbortListener where applicable #48550

Closed

RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023

lib: add option to force handling stopped events

97b00c3

PR-URL: #48301 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

RafaelGSS mentioned this pull request Jul 3, 2023

v20.4.0 proposal #48643

Merged

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

lib: add option to force handling stopped events

cadb8bf

PR-URL: nodejs#48301 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

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

lib: add option to force handling stopped events

dd28606

PR-URL: nodejs#48301 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

ruyadorno added the backport-requested-v18.x label Sep 10, 2023

atlowChemi added the backport-open-v18.x label Sep 10, 2023

atlowChemi added a commit to atlowChemi/node that referenced this pull request Sep 10, 2023

lib: add option to force handling stopped events

832ee65

PR-URL: nodejs#48301 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

ruyadorno pushed a commit that referenced this pull request Sep 11, 2023

lib: add option to force handling stopped events

dbaa2cc

PR-URL: #48301 Backport-PR-URL: #49587 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

ruyadorno added backported-to-v18.x and removed backport-requested-v18.x labels Sep 11, 2023

ruyadorno mentioned this pull request Sep 11, 2023

v18.18.0 proposal #49220

Merged

ruyadorno pushed a commit that referenced this pull request Sep 13, 2023

lib: add option to force handling stopped events

db355d1

PR-URL: #48301 Backport-PR-URL: #49587 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

Uh oh!

lib: add option to force handling stopped events #48301

lib: add option to force handling stopped events #48301

Uh oh!

Conversation

atlowChemi commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 2, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benjamingr commented Jun 3, 2023

Uh oh!

atlowChemi Jun 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MoLow commented Jun 3, 2023

Uh oh!

nodejs-github-bot commented Jun 4, 2023

Uh oh!

Uh oh!

benjamingr commented Jun 20, 2023

Uh oh!

nodejs-github-bot commented Jun 21, 2023

Uh oh!

nodejs-github-bot commented Jun 21, 2023

Uh oh!

nodejs-github-bot commented Jun 22, 2023

Uh oh!

nodejs-github-bot commented Jun 22, 2023

Uh oh!

nodejs-github-bot commented Jun 22, 2023

Uh oh!

ruyadorno commented Sep 10, 2023

Uh oh!

atlowChemi commented Sep 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

atlowChemi commented Jun 2, 2023 •

edited

Loading