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: make WeakRef safe in abort_controller #54791

Closed
wants to merge 1 commit into from
Closed

lib: make WeakRef safe in abort_controller #54791

wants to merge 1 commit into from

Conversation

jazelly
Copy link
Contributor

@jazelly jazelly commented Sep 5, 2024

WeakRef can be prototype polluted, which could affect abort_controller
behaviours. Safety is more important than performance in this case, IMHO.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 5, 2024
@codecov Codecov
Copy link

codecov bot commented Sep 5, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.90%. Comparing base (dcc2ed9) to head (11707af).
Report is 53 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54791      +/-   ##
==========================================
+ Coverage   87.62%   87.90%   +0.28%     
==========================================
  Files         650      651       +1     
  Lines      182983   183343     +360     
  Branches    35406    35713     +307     
==========================================
+ Hits       160336   161165     +829     
+ Misses      15917    15461     -456     
+ Partials     6730     6717      -13
Files with missing lines Coverage Δ
lib/internal/abort_controller.js 97.90% <100.00%> (ø)

... and 77 files with indirect coverage changes

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 5, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 5, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jakecastelli jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell
Copy link
Member

jasnell commented Sep 8, 2024

PR is currently blocked from landing by unreliable CI.

@jazelly
Copy link
Contributor Author

jazelly commented Sep 8, 2024 •
edited
Loading

I think rebasing might dodge that, since there are many flaky tests marked. I'll do a rebase later.

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95
Copy link
Contributor

aduh95 commented Sep 8, 2024

I think rebasing might dodge that, since there are many flaky tests marked. I'll do a rebase later.

Nope, as it forces us to re-run the test suite on all platforms. On Jenkins, when we "resume the CI" it always1 tries to rebase the PR on top of main, so it would have picked up the tests marked as flaky. Anyway, it's too late now, it's OK it can wait until the situation improves.

Footnotes

  1. maybe it's not true for all jobs, e.g. Windows CI where we build and run tests on separate jobs? Not sure. ↩

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2024
@jazelly
Copy link
Contributor Author

jazelly commented Sep 8, 2024

@aduh95 Ah right! I keep forgetting about that. Thanks for the heads up 👍 Now I understand.

@jasnell
Copy link
Member

jasnell commented Sep 8, 2024

This PR is currently blocked from landing due to unreliable CI.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 9, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@jakecastelli jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 13, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 13, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 13, 2024 •
edited by jakecastelli
Loading

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

jasnell pushed a commit that referenced this pull request Sep 13, 2024
@jazelly @jasnell
lib: make WeakRef safe in abort_controller
fee02c7 
PR-URL: #54791
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@jasnell
Copy link
Member

jasnell commented Sep 13, 2024

Landed in fee02c7

@jasnell jasnell closed this Sep 13, 2024
RafaelGSS pushed a commit that referenced this pull request Sep 16, 2024
@jazelly @RafaelGSS
lib: make WeakRef safe in abort_controller
c6f05ce 
PR-URL: #54791
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
Merged
RafaelGSS pushed a commit that referenced this pull request Sep 16, 2024
@jazelly @RafaelGSS
lib: make WeakRef safe in abort_controller
9840810 
PR-URL: #54791
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 17, 2024
@jazelly @RafaelGSS
lib: make WeakRef safe in abort_controller
2b9a637 
PR-URL: #54791
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@MoLow MoLow MoLow approved these changes

@aduh95 aduh95 aduh95 approved these changes

@trivikr trivikr trivikr 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. 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.
7 participants
@jazelly @nodejs-github-bot @jasnell @aduh95 @MoLow @trivikr @jakecastelli