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

dns: restore dns query cache ttl by Ethan-Arrowood · Pull Request #57640 · nodejs/node · GitHub

/ node Public

dns: restore dns query cache ttl - #57640

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Ethan-Arrowood:fix-dns-resolve-cache
Apr 7, 2025
ConversationCommits (1)ChecksFiles changed
Merged

dns: restore dns query cache ttl#57640
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Ethan-Arrowood:fix-dns-resolve-cache

Conversation

Ethan-Arrowood commented Mar 26, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Mar 26, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net

nodejs-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
dns: restore dns query cache ttl
61f93c1 
Fixes: nodejs#57636

Co-authored-by: Robert Nagy <ronagy@icloud.com>
Ethan-Arrowood force-pushed the fix-dns-resolve-cache branch from da48077 to 61f93c1 Compare March 26, 2025 17:47
mcollina approved these changes Mar 26, 2025
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

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

Ethan-Arrowood mentioned this pull request Mar 26, 2025
Closed

codecov Bot commented Mar 26, 2025 •
edited
Loading

Copy link
Copy Markdown

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 lines Coverage Δ
src/cares_wrap.cc 63.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-Arrowood added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 26, 2025
Ethan-Arrowood added backport-requested-v20.x backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. backport-requested-v24.x PRs awaiting manual backport to the v24.x-staging branch. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Mar 26, 2025
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2025

nodejs-github-bot commented Apr 1, 2025

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 2, 2025

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 7, 2025

Copy link
Copy Markdown
Collaborator

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

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 7, 2025
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 7, 2025
nodejs-github-bot merged commit b85505d into nodejs:main Apr 7, 2025

nodejs-github-bot commented Apr 7, 2025

Copy link
Copy Markdown
Collaborator

Landed in b85505d

richardlau removed backport-requested-v20.x backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. labels Apr 8, 2025
richardlau added lts-watch-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 commented Apr 8, 2025

Copy link
Copy Markdown
Member

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).

ronag mentioned this pull request Apr 14, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 15, 2025
dns: restore dns query cache ttl
f683478 
Fixes: #57636

Co-authored-by: Robert Nagy <ronagy@icloud.com>
PR-URL: #57640
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025
dns: restore dns query cache ttl
b2b9eb3 
Fixes: #57636

Co-authored-by: Robert Nagy <ronagy@icloud.com>
PR-URL: #57640
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
github-actions Bot mentioned this pull request Apr 16, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025
dns: restore dns query cache ttl
fa10ba0 
Fixes: #57636

Co-authored-by: Robert Nagy <ronagy@icloud.com>
PR-URL: #57640
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 17, 2025
dns: restore dns query cache ttl
ef212a4 
Fixes: #57636

Co-authored-by: Robert Nagy <ronagy@icloud.com>
PR-URL: #57640
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
dns: restore dns query cache ttl
782b189 
Fixes: #57636

Co-authored-by: Robert Nagy <ronagy@icloud.com>
PR-URL: #57640
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
dns: restore dns query cache ttl
8b4bacd 
Fixes: #57636

Co-authored-by: Robert Nagy <ronagy@icloud.com>
PR-URL: #57640
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 removed lts-watch-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
dns: restore dns query cache ttl nodejs#57640
c5ea64c 
The original fix in (dns: Breaking Change - DNS Caching nodejs#57636) didn't the the option correctly.
Ethan-Arrowood mentioned this pull request May 20, 2025
Merged
nodejs-github-bot pushed a commit that referenced this pull request Jun 2, 2025
dns: fix dns query cache implementation
2be863b 
PR-URL: #58404
Refs: #57640
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
targos pushed a commit that referenced this pull request Jun 3, 2025
dns: fix dns query cache implementation
6bddf58 
PR-URL: #58404
Refs: #57640
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
aduh95 pushed a commit that referenced this pull request Jun 10, 2025
dns: fix dns query cache implementation
a28c336 
PR-URL: #58404
Refs: #57640
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Aug 14, 2025
dns: fix dns query cache implementation
182ae67 
PR-URL: #58404
Refs: #57640
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
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

mcollina mcollina approved these changes
jasnell jasnell approved these changes
lpinca lpinca approved these changes
pimterry pimterry approved these changes
ronag ronag approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Breaking Change - DNS Caching

10 participants

Back | FazBrowse Home | New Git URL