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.

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

lib: clean up references to GCed abort signals #55354

Merged
merged 12 commits into from
Oct 14, 2024

Conversation

geeksilva97
Copy link
Contributor

@geeksilva97 geeksilva97 commented Oct 10, 2024

Refs #55351

Using this commit's description as a reference, this PR adds a SafeFinalizationRegistry to remove GCed dependant signals' references since they will no longer abort.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 10, 2024
@geeksilva97 geeksilva97 marked this pull request as ready for review October 10, 2024 20:10
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.41%. Comparing base (10addb0) to head (c749402).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/abort_controller.js 91.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55354      +/-   ##
==========================================
- Coverage   88.41%   88.41%   -0.01%     
==========================================
  Files         652      652              
  Lines      186864   186878      +14     
  Branches    36064    36064              
==========================================
+ Hits       165217   165229      +12     
- Misses      14888    14891       +3     
+ Partials     6759     6758       -1     
Files with missing lines Coverage Δ
lib/internal/abort_controller.js 98.05% <91.30%> (+0.05%) ⬆️

... and 24 files with indirect coverage changes

@geeksilva97 geeksilva97 changed the title [WIP] abort_controller: remove settled dependant signals when they are GCed lib: remove settled dependant signals when they are no longer valid references Oct 10, 2024
@geeksilva97 geeksilva97 changed the title lib: remove settled dependant signals when they are no longer valid references lib: remove dependant signals when they are no longer valid references Oct 10, 2024
@geeksilva97 geeksilva97 changed the title lib: remove dependant signals when they are no longer valid references lib: remove dependant abort signals when they are no longer valid references Oct 10, 2024
Copy link
Member

@RedYetiDev RedYetiDev left a comment

Choose a reason for hiding this comment

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

Does this fix #55351?

test/parallel/test-abortsignal-drop-settled-signals.mjs Outdated Show resolved Hide resolved
lib/internal/abort_controller.js Outdated Show resolved Hide resolved
lib/internal/abort_controller.js Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev added the abortcontroller Issues and PRs related to the AbortController API label Oct 11, 2024
@geeksilva97
Copy link
Contributor Author

Does this fix #55351?

yes. It is intented to fix that. I will add the ref.

@geeksilva97 geeksilva97 force-pushed the abortsignal-mem-leak branch 2 times, most recently from 91c7d36 to 0c94df4 Compare October 11, 2024 03:04
@geeksilva97 geeksilva97 force-pushed the abortsignal-mem-leak branch 2 times, most recently from 7fde7c9 to 1550729 Compare October 11, 2024 13:57
Copy link
Contributor

@mika-fischer mika-fischer left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

lib/internal/abort_controller.js Outdated Show resolved Hide resolved
lib/internal/abort_controller.js Outdated Show resolved Hide resolved
@geeksilva97 geeksilva97 changed the title lib: remove dependant abort signals when they are no longer valid references lib: clean up references to GCed abort signals Oct 13, 2024
@nodejs-github-bot
Copy link
Collaborator

@atlowChemi atlowChemi added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 14, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 14, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/55354
✔  Done loading data for nodejs/node/pull/55354
----------------------------------- PR info ------------------------------------
Title      lib: clean up references to GCed abort signals (#55354)
Author     Edigleysson Silva (Edy) <edigleyssonsilva@gmail.com> (@geeksilva97)
Branch     geeksilva97:abortsignal-mem-leak -> nodejs:main
Labels     author ready, needs-ci, abortcontroller
Commits    12
 - lib: remove settled dependant signals when they are GCed
 - test: test settled signals dropping
 - fixup: typo
 - fix: avoid holding strong reference on finalizers
 - test: improve tests
 - lib: optmize finalizers logic
 - test: ensure short-lived signals are GCed
 - refactor: rename variables for improving readability
 - test: improve signal dropping test
 - test: test settled signals when AbortSignal.any receives a composite …
 - refactor: remove unneeded describe
 - chore: address pr's comments
Committers 1
 - Edy Silva <edigleyssonsilva@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 10 Oct 2024 19:39:41 GMT
   ✔  Approvals: 2
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/55354#pullrequestreview-2365009577
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/55354#pullrequestreview-2367476798
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-10-14T15:55:25Z: https://ci.nodejs.org/job/node-test-pull-request/63096/
- Querying data for job/node-test-pull-request/63096/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 55354
From https://github.com/nodejs/node
 * branch                  refs/pull/55354/merge -> FETCH_HEAD
✔  Fetched commits as 10addb0a208c..c74940249bd5
--------------------------------------------------------------------------------
[main 4745da97a9] lib: remove settled dependant signals when they are GCed
 Author: Edy Silva <edigleyssonsilva@gmail.com>
 Date: Thu Oct 10 17:21:35 2024 -0300
 1 file changed, 11 insertions(+)
[main 2bdfd0d35a] test: test settled signals dropping
 Author: Edy Silva <edigleyssonsilva@gmail.com>
 Date: Thu Oct 10 18:37:47 2024 -0300
 1 file changed, 64 insertions(+)
 create mode 100644 test/parallel/test-abortsignal-drop-settled-signals.mjs
[main 6fb8c73417] fixup: typo
 Author: Edy Silva <edigleyssonsilva@gmail.com>
 Date: Thu Oct 10 21:59:08 2024 -0300
 2 files changed, 15 insertions(+), 22 deletions(-)
[main 2cfd50727a] fix: avoid holding strong reference on finalizers
 Author: Edy Silva <edigleyssonsilva@gmail.com>
 Date: Fri Oct 11 00:04:54 2024 -0300
 1 file changed, 8 insertions(+), 3 deletions(-)
[main 65f4b7a559] test: improve tests
 Author: Edy Silva <edigleyssonsilva@gmail.com>
 Date: Fri Oct 11 01:12:53 2024 -0300
 1 file changed, 22 insertions(+), 20 deletions(-)
[main 11b379fef8] lib: optmize finalizers logic
 Author: Edy Silva <edigleyssonsilva@gmail.com>
 Date: Fri Oct 11 10:32:19 2024 -0300
 2 files changed, 6 insertions(+), 5 deletions(-)
[main 15bafd6818] test: ensure short-lived signals are GCed
 Author: Edy Silva <edigleyssonsilva@gmail.com>
 Date: Fri Oct 11 10:57:37 2024 -0300
 1 file changed, 33 insertions(+)
[main 92d13d06cf] refactor: rename variables for improving readability
 Author: Edy Silva <edigleyssonsilva@gmail.com>
 Date: Fri Oct 11 11:03:10 2024 -0300
 1 file changed, 9 insertions(+), 10 deletions(-)
[main e2ada51234] test: improve signal dropping test
 Author: Edy Silva <edigleyssonsilva@gmail.com>
 Date: Fri Oct 11 12:12:36 2024 -0300
 1 file changed, 44 insertions(+), 5 deletions(-)
[main 1c899c72ef] test: test settled signals when AbortSignal.any receives a composite signal
 Author: Edy Silva <edigleyssonsilva@gmail.com>
 Date: Fri Oct 11 14:21:22 2024 -0300
 1 file changed, 20 insertions(+), 21 deletions(-)
[main 7c9801c22a] refactor: remove unneeded describe
 Author: Edy Silva <edigleyssonsilva@gmail.com>
 Date: Fri Oct 11 14:25:54 2024 -0300
 1 file changed, 6 insertions(+), 8 deletions(-)
[main d673215df0] chore: address pr's comments
 Author: Edy Silva <edigleyssonsilva@gmail.com>
 Date: Mon Oct 14 11:05:12 2024 -0300
 2 files changed, 20 insertions(+), 28 deletions(-)
   ✔  Patches applied
There are 12 commits in the PR. Attempting autorebase.
Rebasing (2/24)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
lib: remove settled dependant signals when they are GCed

PR-URL: #55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

[detached HEAD 65983b7d45] lib: remove settled dependant signals when they are GCed
Author: Edy Silva <edigleyssonsilva@gmail.com>
Date: Thu Oct 10 17:21:35 2024 -0300
1 file changed, 11 insertions(+)
Rebasing (3/24)
Rebasing (4/24)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: test settled signals dropping

PR-URL: #55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

[detached HEAD 2fe912a966] test: test settled signals dropping
Author: Edy Silva <edigleyssonsilva@gmail.com>
Date: Thu Oct 10 18:37:47 2024 -0300
1 file changed, 64 insertions(+)
create mode 100644 test/parallel/test-abortsignal-drop-settled-signals.mjs
Rebasing (5/24)
Rebasing (6/24)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup: typo

PR-URL: #55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

[detached HEAD cab6e3c761] fixup: typo
Author: Edy Silva <edigleyssonsilva@gmail.com>
Date: Thu Oct 10 21:59:08 2024 -0300
2 files changed, 15 insertions(+), 22 deletions(-)
Rebasing (7/24)
Rebasing (8/24)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fix: avoid holding strong reference on finalizers

PR-URL: #55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

[detached HEAD f9e900be48] fix: avoid holding strong reference on finalizers
Author: Edy Silva <edigleyssonsilva@gmail.com>
Date: Fri Oct 11 00:04:54 2024 -0300
1 file changed, 8 insertions(+), 3 deletions(-)
Rebasing (9/24)
Rebasing (10/24)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: improve tests

PR-URL: #55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

[detached HEAD 98584c3ae1] test: improve tests
Author: Edy Silva <edigleyssonsilva@gmail.com>
Date: Fri Oct 11 01:12:53 2024 -0300
1 file changed, 22 insertions(+), 20 deletions(-)
Rebasing (11/24)
Rebasing (12/24)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
lib: optmize finalizers logic

PR-URL: #55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

[detached HEAD 08a07c801e] lib: optmize finalizers logic
Author: Edy Silva <edigleyssonsilva@gmail.com>
Date: Fri Oct 11 10:32:19 2024 -0300
2 files changed, 6 insertions(+), 5 deletions(-)
Rebasing (13/24)
Rebasing (14/24)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: ensure short-lived signals are GCed

PR-URL: #55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

[detached HEAD 6d9e3f7faf] test: ensure short-lived signals are GCed
Author: Edy Silva <edigleyssonsilva@gmail.com>
Date: Fri Oct 11 10:57:37 2024 -0300
1 file changed, 33 insertions(+)
Rebasing (15/24)
Rebasing (16/24)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
refactor: rename variables for improving readability

PR-URL: #55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

[detached HEAD d0c2a1894c] refactor: rename variables for improving readability
Author: Edy Silva <edigleyssonsilva@gmail.com>
Date: Fri Oct 11 11:03:10 2024 -0300
1 file changed, 9 insertions(+), 10 deletions(-)
Rebasing (17/24)
Rebasing (18/24)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: improve signal dropping test

PR-URL: #55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

[detached HEAD 90ec8edab0] test: improve signal dropping test
Author: Edy Silva <edigleyssonsilva@gmail.com>
Date: Fri Oct 11 12:12:36 2024 -0300
1 file changed, 44 insertions(+), 5 deletions(-)
Rebasing (19/24)
Rebasing (20/24)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: test settled signals when AbortSignal.any receives a composite signal

PR-URL: #55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

[detached HEAD 2a666c9d05] test: test settled signals when AbortSignal.any receives a composite signal
Author: Edy Silva <edigleyssonsilva@gmail.com>
Date: Fri Oct 11 14:21:22 2024 -0300
1 file changed, 20 insertions(+), 21 deletions(-)
Rebasing (21/24)
Rebasing (22/24)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
refactor: remove unneeded describe

PR-URL: #55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

[detached HEAD 94e0682924] refactor: remove unneeded describe
Author: Edy Silva <edigleyssonsilva@gmail.com>
Date: Fri Oct 11 14:25:54 2024 -0300
1 file changed, 6 insertions(+), 8 deletions(-)
Rebasing (23/24)
Rebasing (24/24)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
chore: address pr's comments

PR-URL: #55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

[detached HEAD f922ab0ec4] chore: address pr's comments
Author: Edy Silva <edigleyssonsilva@gmail.com>
Date: Mon Oct 14 11:05:12 2024 -0300
2 files changed, 20 insertions(+), 28 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/11334066922

@atlowChemi atlowChemi added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 14, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 14, 2024
@nodejs-github-bot nodejs-github-bot merged commit 7a7c2b3 into nodejs:main Oct 14, 2024
71 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 7a7c2b3

@geeksilva97
Copy link
Contributor Author

geeksilva97 commented Oct 15, 2024

Backport PRs

#55388
#55389

geeksilva97 added a commit to geeksilva97/node that referenced this pull request Oct 15, 2024
PR-URL: nodejs#55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
@RedYetiDev
Copy link
Member

See #55389 (comment)
See #55388 (comment)

Why are you backporting this? It lands cleanly, meaning that a manual backport isn't needed

@geeksilva97
Copy link
Contributor Author

See #55389 (comment) See #55388 (comment)

Why are you backporting this? It lands cleanly, meaning that a manual backport isn't needed

Both are closed.

aduh95 pushed a commit that referenced this pull request Oct 19, 2024
PR-URL: #55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
@aduh95 aduh95 mentioned this pull request Oct 24, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
PR-URL: nodejs#55354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abortcontroller Issues and PRs related to the AbortController API author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants