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

lib: remove redundant destroyHook checks by gurgunday · Pull Request #60120 · nodejs/node · GitHub

/ node Public

lib: remove redundant destroyHook checks - #60120

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
gurgunday:remove-redundant-check
Oct 8, 2025
ConversationCommits (3)ChecksFiles changed
Merged

lib: remove redundant destroyHook checks#60120
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
gurgunday:remove-redundant-check

Conversation

gurgunday commented Oct 4, 2025

Copy link
Copy Markdown
Member

// Fewer checks may be possible, but these cover everything.

emitDestroy already checks these now:

node/lib/internal/async_hooks.js

Lines 533 to 538 in 6f941fc

function emitDestroyScript(asyncId) {
// Return early if there are no destroy callbacks, or invalid asyncId.
if (!hasHooks(kDestroy) || asyncId <= 0)
return;
async_wrap.queueDestroyAsyncId(asyncId);
}

We are essentially checking the same condition twice

Also the invalid id check didn't cover undefined. I don't know if it was intentional, but in any case, since #56966, which removed enroll, I'm not sure if we can have an undefined asyncId anyway

nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Oct 4, 2025

codecov Bot commented Oct 4, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.48%. Comparing base (6f941fc) to head (0255b2e).
⚠️ Report is 51 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #60120      +/-   ##
==========================================
+ Coverage   88.45%   88.48%   +0.02%     
==========================================
  Files         703      703              
  Lines      207826   207822       -4     
  Branches    40030    40027       -3     
==========================================
+ Hits       183842   183889      +47     
+ Misses      15973    15940      -33     
+ Partials     8011     7993      -18
Files with missing lines Coverage Δ
lib/internal/async_hooks.js 99.37% <100.00%> (ø)
lib/internal/process/task_queues.js 100.00% <100.00%> (ø)
lib/internal/timers.js 99.72% <100.00%> (+0.13%) ⬆️
lib/timers.js 100.00% <100.00%> (ø)

... and 55 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Flarna added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 6, 2025
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2025

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

nodejs-github-bot commented Oct 7, 2025

Copy link
Copy Markdown
Collaborator

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

gurgunday added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 8, 2025
Flarna added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 8, 2025
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 8, 2025
nodejs-github-bot merged commit 1527825 into nodejs:main Oct 8, 2025
68 of 70 checks passed

nodejs-github-bot commented Oct 8, 2025

Copy link
Copy Markdown
Collaborator

Landed in 1527825

gurgunday deleted the remove-redundant-check branch October 9, 2025 07:10
RafaelGSS mentioned this pull request Oct 13, 2025
Merged
9 tasks
aduh95 pushed a commit that referenced this pull request Oct 31, 2025
lib: remove redundant destroyHook checks
3ef0750 
PR-URL: #60120
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
github-actions Bot mentioned this pull request Nov 7, 2025
Closed
aduh95 pushed a commit that referenced this pull request Nov 7, 2025
lib: remove redundant destroyHook checks
f872586 
PR-URL: #60120
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
github-actions Bot mentioned this pull request Nov 7, 2025
Merged
aduh95 pushed a commit that referenced this pull request Feb 17, 2026
lib: remove redundant destroyHook checks
8d26ccc 
PR-URL: #60120
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
aduh95 mentioned this pull request Feb 22, 2026
Merged
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

anonrig anonrig approved these changes
Flarna Flarna approved these changes

Assignees

No one assigned

Labels

async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no outstanding review comments, 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. process Issues and PRs related to the process subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL