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.

Sign up for GitHub

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

Jump to bottom

lib: add option to force handling stopped events #48301

Merged
merged 1 commit into from
Jun 22, 2023
Merged

lib: add option to force handling stopped events #48301

merged 1 commit into from
Jun 22, 2023

Conversation

atlowChemi
Copy link
Member

@atlowChemi atlowChemi commented Jun 2, 2023 •
edited
Loading

@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@atlowChemi atlowChemi requested a review from benjamingr June 2, 2023 13:57
@nodejs-github-bot 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
ljharb reviewed Jun 2, 2023
View reviewed changes
lib/child_process.js Outdated Show resolved Hide resolved
lib/dgram.js Outdated Show resolved Hide resolved
lib/events.js Outdated Show resolved Hide resolved
lib/fs.js Outdated Show resolved Hide resolved
lib/internal/abort_controller.js Outdated Show resolved Hide resolved
lib/readline.js Outdated Show resolved Hide resolved
lib/readline/promises.js Outdated Show resolved Hide resolved
lib/timers/promises.js Outdated Show resolved Hide resolved
lib/timers/promises.js Outdated Show resolved Hide resolved
lib/timers/promises.js Outdated Show resolved Hide resolved
@benjamingr
Copy link
Member

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
atlowChemi commented Jun 3, 2023
View reviewed changes
lib/internal/event_target.js
@@ -609,16 +617,16 @@ class EventTarget {
// not prevent the event target from GC.
Copy link
Member Author

Choose a reason for hiding this comment

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

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

MoLow
MoLow reviewed Jun 3, 2023
View reviewed changes
lib/internal/event_target.js Outdated Show resolved Hide resolved
@MoLow
Copy link
Member

MoLow commented Jun 3, 2023

+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

@atlowChemi 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 atlowChemi marked this pull request as ready for review June 3, 2023 22:42
@atlowChemi 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 4, 2023
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Jun 5, 2023
Open
Open
Open
@atlowChemi atlowChemi force-pushed the avoid-signals-not-working-due-to-propogation-stopped branch from 8218ad4 to d75ff43 Compare June 13, 2023 13:14
@atlowChemi atlowChemi force-pushed the avoid-signals-not-working-due-to-propogation-stopped branch from d75ff43 to fa4dc42 Compare June 20, 2023 15:00
@atlowChemi atlowChemi force-pushed the avoid-signals-not-working-due-to-propogation-stopped branch from fa4dc42 to 061ea54 Compare June 20, 2023 19:04
@benjamingr
Copy link
Member

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

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 20, 2023
@atlowChemi 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 atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 21, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@atlowChemi atlowChemi added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2023
@nodejs-github-bot 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 nodejs-github-bot merged commit bcd35c3 into nodejs:main Jun 22, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in bcd35c3

@atlowChemi atlowChemi deleted the avoid-signals-not-working-due-to-propogation-stopped branch June 22, 2023 14:16
This was referenced Jun 23, 2023
Open
Open
@atlowChemi atlowChemi mentioned this pull request Jun 25, 2023
Closed
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@atlowChemi @RafaelGSS
lib: add option to force handling stopped events
97b00c3 
PR-URL: #48301
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@atlowChemi @Ceres6
lib: add option to force handling stopped events
cadb8bf 
PR-URL: nodejs#48301
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@atlowChemi @Ceres6
lib: add option to force handling stopped events
dd28606 
PR-URL: nodejs#48301
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@ruyadorno
Copy link
Member

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

@ruyadorno ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 10, 2023
@atlowChemi atlowChemi added the backport-open-v18.x Indicate that the PR has an open backport. label Sep 10, 2023
atlowChemi added a commit to atlowChemi/node that referenced this pull request Sep 10, 2023
@atlowChemi
lib: add option to force handling stopped events
832ee65 
PR-URL: nodejs#48301
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@atlowChemi
Copy link
Member Author

@ruyadorno opened a backport at #49587 🙂

ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
@atlowChemi @ruyadorno
lib: add option to force handling stopped events
dbaa2cc 
PR-URL: #48301
Backport-PR-URL: #49587
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@ruyadorno ruyadorno added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. backport-open-v18.x Indicate that the PR has an open backport. labels Sep 11, 2023
@ruyadorno ruyadorno mentioned this pull request Sep 11, 2023
Merged
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@atlowChemi @ruyadorno
lib: add option to force handling stopped events
db355d1 
PR-URL: #48301
Backport-PR-URL: #49587
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@MoLow MoLow MoLow approved these changes

@benjamingr benjamingr benjamingr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v18.x PRs backported to the v18.x-staging branch. eventtarget Issues and PRs related to the EventTarget implementation. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@atlowChemi @nodejs-github-bot @benjamingr @MoLow @ruyadorno @ljharb