FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: add option to force handling stopped events by atlowChemi · Pull Request #48301 · nodejs/node · GitHub

/ node Public

lib: add option to force handling stopped events - #48301

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
atlowChemi:avoid-signals-not-working-due-to-propogation-stopped
Jun 22, 2023
ConversationCommits (1)ChecksFiles changed
Merged

lib: add option to force handling stopped events#48301
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
atlowChemi:avoid-signals-not-working-due-to-propogation-stopped

Conversation

atlowChemi commented Jun 2, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

nodejs-github-bot commented Jun 2, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

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

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
Comment thread lib/child_process.js Outdated
Comment thread lib/dgram.js Outdated
Comment thread lib/events.js Outdated
Comment thread lib/fs.js Outdated
Comment thread lib/internal/abort_controller.js Outdated
Comment thread lib/readline.js Outdated
Comment thread lib/readline/promises.js Outdated
Comment thread lib/timers/promises.js Outdated
Comment thread lib/timers/promises.js Outdated
Comment thread lib/timers/promises.js Outdated

benjamingr commented Jun 3, 2023

Copy link
Copy Markdown
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 commented Jun 3, 2023
View reviewed changes
Comment thread lib/internal/event_target.js Outdated

atlowChemi Jun 3, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

MoLow reviewed Jun 3, 2023
View reviewed changes
Comment thread lib/internal/event_target.js Outdated

MoLow commented Jun 3, 2023

Copy link
Copy Markdown
Member

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

nodejs-github-bot commented Jun 4, 2023

Copy link
Copy Markdown
Collaborator

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

This was referenced Jun 5, 2023
Open
Open
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
Comment thread lib/internal/event_target.js Outdated
atlowChemi force-pushed the avoid-signals-not-working-due-to-propogation-stopped branch from fa4dc42 to 061ea54 Compare June 20, 2023 19:04

benjamingr commented Jun 20, 2023

Copy link
Copy Markdown
Member

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

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 outstanding review comments, 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

nodejs-github-bot commented Jun 21, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jun 21, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jun 22, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jun 22, 2023

Copy link
Copy Markdown
Collaborator

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

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

nodejs-github-bot commented Jun 22, 2023

Copy link
Copy Markdown
Collaborator

Landed in bcd35c3

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 mentioned this pull request Jun 25, 2023
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 <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
RafaelGSS mentioned this pull request Jul 3, 2023
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 <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
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 commented Sep 10, 2023

Copy link
Copy Markdown
Member

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

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 <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

atlowChemi commented Sep 10, 2023

Copy link
Copy Markdown
Member Author

@ruyadorno opened a backport at #49587 🙂

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 <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
ruyadorno mentioned this pull request Sep 11, 2023
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 <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ljharb ljharb left review comments
benjamingr benjamingr approved these changes
MoLow MoLow approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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

Back | FazBrowse Home | New Git URL