Skip to content

lib: add option to force handling stopped events#48301

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
Merged
nodejs-github-bot merged 1 commit into from
Jun 22, 2023
Merged

lib: add option to force handling stopped events #48301

nodejs-github-bot merged 1 commit into from
Jun 22, 2023

Conversation

@atlowChemi
Copy link
Member

@atlowChemiatlowChemi commented Jun 2, 2023•
edited
Loading

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 2, 2023

Review requested:

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

@nodejs-github-botnodejs-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
@benjamingr
Copy link
Member

benjamingr commented Jun 3, 2023

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
return;
}
// TODO(benjamingr) make this weak somehow? ideally the signal would
// not prevent the event target from GC.
Copy link
MemberAuthor

@atlowChemiatlowChemiJun 3, 2023

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

@atlowChemiatlowChemiforce-pushed the avoid-signals-not-working-due-to-propogation-stopped branch 4 times, most recently from 9bbebcc to 8218ad4CompareJune 3, 2023 22:40
@atlowChemiatlowChemi marked this pull request as ready for review June 3, 2023 22:42
@atlowChemiatlowChemi 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-actionsgithub-actionsbot 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

nodejs-github-bot commented Jun 4, 2023

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

This was referenced Jun 5, 2023
Open
Open
Open
@atlowChemiatlowChemiforce-pushed the avoid-signals-not-working-due-to-propogation-stopped branch from 8218ad4 to d75ff43CompareJune 13, 2023 13:14
@atlowChemiatlowChemiforce-pushed the avoid-signals-not-working-due-to-propogation-stopped branch from d75ff43 to fa4dc42CompareJune 20, 2023 15:00
ljharb
ljharb reviewed Jun 20, 2023
View reviewed changes
@atlowChemiatlowChemiforce-pushed the avoid-signals-not-working-due-to-propogation-stopped branch from fa4dc42 to 061ea54CompareJune 20, 2023 19:04
@benjamingr
Copy link
Member

benjamingr commented Jun 20, 2023

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

@benjamingrbenjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 20, 2023
@atlowChemiatlowChemi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 20, 2023
@atlowChemiatlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 21, 2023
@github-actionsgithub-actionsbot 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

nodejs-github-bot commented Jun 21, 2023

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 21, 2023

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 22, 2023

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 22, 2023

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

@atlowChemiatlowChemi added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2023
@nodejs-github-botnodejs-github-bot merged commit bcd35c3 into nodejs:mainJun 22, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 22, 2023

Landed in bcd35c3

@atlowChemiatlowChemi deleted the avoid-signals-not-working-due-to-propogation-stopped branch June 22, 2023 14:16
This was referenced Jun 23, 2023
Open
Open
@atlowChemiatlowChemi 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 <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
@RafaelGSSRafaelGSS 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 <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
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 <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
@ruyadorno
Copy link
Member

ruyadorno commented Sep 10, 2023

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
@atlowChemi
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]>
@atlowChemi
Copy link
MemberAuthor

atlowChemi commented Sep 10, 2023

@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 <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
@ruyadornoruyadorno 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 <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharbljharbljharb left review comments

@benjamingrbenjamingrbenjamingr approved these changes

@MoLowMoLowMoLow approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.eventtargetIssues and PRs related to the EventTarget implementation.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs 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