Skip to content

dns: restore dns query cache ttl#57640

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
Apr 7, 2025
Merged

dns: restore dns query cache ttl #57640

nodejs-github-bot merged 1 commit into from
Apr 7, 2025

Conversation

@Ethan-Arrowood
Copy link
Contributor

@Ethan-ArrowoodEthan-Arrowood commented Mar 26, 2025•
edited
Loading

Fixes: #57636

When c-ares was updated the default caching behavior changed.

As of c-ares 1.31.0, the query cache is enabled by default with a TTL of 1hr. To disable the query cache, specify this option with a TTL of 0. The query cache is based on the returned TTL in the DNS message.

This PR restores the caching behavior by setting qcache_max_ttl to 0.

This change in c-ares is reasonable; thus, as a follow up to this fix, we should implement a cache management API for DNS queries and then re-enable the new default cache value. Tracking issue for follow up work: #57641

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 26, 2025

Review requested:

  • @nodejs/net

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. needs-ci PRs that need a full CI run. labels Mar 26, 2025
mcollina
mcollina approved these changes Mar 26, 2025
View reviewed changes
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Can you open the follow-up issue to track that work?

@Ethan-ArrowoodEthan-Arrowood mentioned this pull request Mar 26, 2025
Open
@codecov
Copy link

codecovbot commented Mar 26, 2025•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.22%. Comparing base (1123585) to head (61f93c1).
Report is 91 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #57640 +/- ## ========================================== - Coverage 90.22% 90.22% -0.01%  ========================================== Files 630 630 Lines 185055 185056 +1 Branches 36216 36218 +2 ========================================== - Hits 166975 166963 -12 - Misses 11042 11047 +5 - Partials 7038 7046 +8
Files with missing linesCoverage Δ
src/cares_wrap.cc63.05% <100.00%> (+0.02%)⬆️

... and 21 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.

@Ethan-ArrowoodEthan-Arrowood added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 26, 2025
@Ethan-ArrowoodEthan-Arrowood added backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. backport-requested-v23.x backport-requested-v24.x PRs awaiting manual backport to the v24.x-staging branch. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 26, 2025
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 1, 2025

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 2, 2025

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 7, 2025

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

@mcollinamcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 7, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 7, 2025
@nodejs-github-botnodejs-github-bot merged commit b85505d into nodejs:mainApr 7, 2025
79 checks passed
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 7, 2025

Landed in b85505d

@richardlaurichardlau removed backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. labels Apr 8, 2025
@richardlaurichardlau added lts-watch-v20.x PRs that may need to be released in v20.x lts-watch-v22.x PRs that may need to be released in v22.x and removed backport-requested-v23.x backport-requested-v24.x PRs awaiting manual backport to the v24.x-staging branch. labels Apr 8, 2025
@richardlau
Copy link
Member

richardlau commented Apr 8, 2025

I'm assuming this will land cleanly and removed the backport-requested-* labels (and added lts-watch-* labels to make sure this gets lands on Node.js 22 and 20).

JonasBa pushed a commit to JonasBa/node that referenced this pull request Apr 11, 2025
@Ethan-Arrowood@ronag@JonasBa
dns: restore dns query cache ttl
3f55aeb
Fixes: nodejs#57636 Co-authored-by: Robert Nagy <[email protected]> PR-URL: nodejs#57640 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>
@ronagronag mentioned this pull request Apr 14, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 15, 2025
@Ethan-Arrowood@ronag@RafaelGSS
dns: restore dns query cache ttl
f683478
Fixes: #57636 Co-authored-by: Robert Nagy <[email protected]> PR-URL: #57640 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025
@Ethan-Arrowood@ronag@RafaelGSS
dns: restore dns query cache ttl
b2b9eb3
Fixes: #57636 Co-authored-by: Robert Nagy <[email protected]> PR-URL: #57640 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>
@github-actionsgithub-actionsbot mentioned this pull request Apr 16, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025
@Ethan-Arrowood@ronag@RafaelGSS
dns: restore dns query cache ttl
fa10ba0
Fixes: #57636 Co-authored-by: Robert Nagy <[email protected]> PR-URL: #57640 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 17, 2025
@Ethan-Arrowood@ronag@RafaelGSS
dns: restore dns query cache ttl
ef212a4
Fixes: #57636 Co-authored-by: Robert Nagy <[email protected]> PR-URL: #57640 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@Ethan-Arrowood@ronag@RafaelGSS
dns: restore dns query cache ttl
782b189
Fixes: #57636 Co-authored-by: Robert Nagy <[email protected]> PR-URL: #57640 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>
@RafaelGSSRafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@Ethan-Arrowood@ronag@RafaelGSS
dns: restore dns query cache ttl
8b4bacd
Fixes: #57636 Co-authored-by: Robert Nagy <[email protected]> PR-URL: #57640 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>
@aduh95aduh95 removed lts-watch-v20.x PRs that may need to be released in v20.x lts-watch-v22.x PRs that may need to be released in v22.x labels May 6, 2025
augjoh added a commit to augjoh/node that referenced this pull request May 18, 2025
@augjoh
dns: restore dns query cache ttlnodejs#57640
c5ea64c
The original fix in (dns: Breaking Change - DNS Caching nodejs#57636) didn't the the option correctly.
@Ethan-ArrowoodEthan-Arrowood mentioned this pull request May 20, 2025
Merged
nodejs-github-bot pushed a commit that referenced this pull request Jun 2, 2025
@Ethan-Arrowood
dns: fix dns query cache implementation
2be863b
PR-URL: #58404 Refs: #57640 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tim Perry <[email protected]>
targos pushed a commit that referenced this pull request Jun 3, 2025
@Ethan-Arrowood@targos
dns: fix dns query cache implementation
6bddf58
PR-URL: #58404 Refs: #57640 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tim Perry <[email protected]>
aduh95 pushed a commit that referenced this pull request Jun 10, 2025
@Ethan-Arrowood@aduh95
dns: fix dns query cache implementation
a28c336
PR-URL: #58404 Refs: #57640 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tim Perry <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Aug 14, 2025
@Ethan-Arrowood@marco-ippolito
dns: fix dns query cache implementation
182ae67
PR-URL: #58404 Refs: #57640 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tim Perry <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@jasnelljasnelljasnell approved these changes

@lpincalpincalpinca approved these changes

@pimterrypimterrypimterry approved these changes

@ronagronagronag approved these changes

@BridgeARBridgeARBridgeAR 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.c++Issues and PRs that require attention from people who are familiar with C++.caresIssues and PRs related to the c-ares dependency or the cares_wrap binding.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Breaking Change - DNS Caching

10 participants

@Ethan-Arrowood@nodejs-github-bot@richardlau@mcollina@jasnell@lpinca@pimterry@ronag@BridgeAR@aduh95