Skip to content

lib: remove redundant destroyHook checks - #60120

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
gurgunday:remove-redundant-check
Oct 8, 2025
Merged

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

Conversation

@gurgunday

Copy link
Copy Markdown
Member

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

emitDestroy already checks these now:

functionemitDestroyScript(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-botnodejs-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

codecovBot commented Oct 4, 2025

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 linesCoverage Δ
lib/internal/async_hooks.js99.37% <100.00%> (ø)
lib/internal/process/task_queues.js100.00% <100.00%> (ø)
lib/internal/timers.js99.72% <100.00%> (+0.13%)⬆️
lib/timers.js100.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.

@FlarnaFlarna 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-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@gurgundaygurgunday added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 8, 2025
@FlarnaFlarna added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 8, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 8, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 1527825 into nodejs:mainOct 8, 2025
68 of 70 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 1527825

@gurgunday
gurgunday deleted the remove-redundant-check branch October 9, 2025 07:10
@RafaelGSSRafaelGSS mentioned this pull request Oct 13, 2025
9 tasks
aduh95 pushed a commit that referenced this pull request Oct 31, 2025
PR-URL: #60120
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
aduh95 pushed a commit that referenced this pull request Nov 7, 2025
PR-URL: #60120
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
aduh95 pushed a commit that referenced this pull request Feb 17, 2026
PR-URL: #60120
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@aduh95aduh95 mentioned this pull request Feb 22, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

async_hooksIssues and PRs related to the async hooks subsystem.author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.processIssues and PRs related to the process subsystem.timersIssues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@gurgunday@nodejs-github-bot@anonrig@Flarna