Skip to content

src: migrate deprecated c-ares APIs to modern replacements - #62724

Open
omghante wants to merge 1 commit into
nodejs:mainfrom
omghante:fix/52464-cares-deprecation-warnings
Open

src: migrate deprecated c-ares APIs to modern replacements#62724
omghante wants to merge 1 commit into
nodejs:mainfrom
omghante:fix/52464-cares-deprecation-warnings

Conversation

@omghante

@omghante omghante commented Apr 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR migrates the deprecated c-ares reply-parsing APIs in src/cares_wrap.cc to the modern ares_dns_parse / ares_dns_record_t API family, replacing ~10 deprecated call sites (ares_parse_a_reply, ares_parse_aaaa_reply, ares_parse_ns_reply, ares_parse_ptr_reply, ares_parse_mx_reply, ares_parse_caa_reply, ares_parse_txt_reply_ext, ares_parse_srv_reply, ares_parse_naptr_reply, ares_parse_soa_reply) with their modern equivalents while preserving CNAME-TTL clamping behavior.

The server management functions (EnsureServers, GetServers, SetServers) retain the original ares_get_servers_ports / ares_set_servers_ports APIs with targeted pragma suppression, as the modern CSV alternatives (ares_get_servers_csv, ares_set_servers_ports_csv) silently drop IPv6 link-local addresses.

Fixes: #52464

@nodejs-github-bot

nodejs-github-bot commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot 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 Apr 14, 2026
@omghante
omghante force-pushed the fix/52464-cares-deprecation-warnings branch from d0750bf to 140cde3 Compare April 14, 2026 02:28
@codecov

codecov Bot commented Apr 14, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 65.61265% with 87 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.64%. Comparing base (5e61a07) to head (3616cdb).
⚠️ Report is 942 commits behind head on main.

Files with missing lines Patch % Lines
src/cares_wrap.cc 65.61% 49 Missing and 38 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #62724      +/-   ##
==========================================
- Coverage   89.65%   89.64%   -0.01%     
==========================================
  Files         712      712              
  Lines      220498   220595      +97     
  Branches    42278    42305      +27     
==========================================
+ Hits       197683   197758      +75     
- Misses      14663    14675      +12     
- Partials     8152     8162      +10
Files with missing lines Coverage Δ
src/cares_wrap.cc 61.86% <65.61%> (+0.21%) ⬆️

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

@omghante
omghante force-pushed the fix/52464-cares-deprecation-warnings branch from 140cde3 to abc640a Compare April 14, 2026 08:27
@mcollina

mcollina commented Apr 14, 2026

Copy link
Copy Markdown
Member

CI is very red

@omghante
omghante force-pushed the fix/52464-cares-deprecation-warnings branch from abc640a to fedc05d Compare April 14, 2026 17:26
@omghante

omghante commented Apr 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks @mcollina! Fixed, all workflows are finally back to green! Feel free to take a look whenever you have a chance.

@omghante
omghante force-pushed the fix/52464-cares-deprecation-warnings branch 14 times, most recently from a93485a to e5e76a2 Compare April 15, 2026 00:48
@omghante omghante mentioned this pull request Apr 15, 2026
Closed
mcollina
mcollina requested changes Apr 15, 2026
View reviewed changes
Comment thread test/parallel/test-dns.js
Comment thread test/parallel/test-dns.js
@omghante

omghante commented Apr 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@mcollina I've reworked the approach now just suppressing the warnings with compiler pragmas instead of migrating APIs. Test file is fully untouched, all fe80:: cases preserved. Full API migration will be a follow-up PR.

thisalihassan
thisalihassan reviewed Apr 18, 2026
View reviewed changes

@thisalihassan thisalihassan left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Signed-off-by is missing, can you please ammend your commit?

Comment thread src/cares_wrap.cc Outdated
thisalihassan
thisalihassan reviewed Apr 18, 2026
View reviewed changes
Comment thread src/cares_wrap.cc Outdated
Comment thread src/cares_wrap.cc Outdated
@omghante
omghante force-pushed the fix/52464-cares-deprecation-warnings branch from e5e76a2 to 8014a4d Compare April 18, 2026 09:47
@omghante

omghante commented Apr 18, 2026

Copy link
Copy Markdown
Contributor Author

@thisalihassan Great catches, all valid. I've force-pushed a new approach just pragma-based warning suppression (13 lines, 1 file). No API migration, so the IPv6 link-local and CNAME-TTL clamping issues are gone. Signed-off-by added. Full migration can be a separate PR.

@omghante

omghante commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

Hi @mcollina following up on this. I've addressed all feedback from your
earlier review and @thisalihassan's comments:

  • Reworked to a pragma-based warning suppression approach (13 lines, 1 file)
    instead of a full API migration, avoiding the IPv6 link-local and CNAME-TTL
    clamping regressions
  • Signed-off-by added
  • CI is green

The full migration to ares_dns_parse / ares_get_servers_csv /
ares_set_servers_csv will be a separate follow-up PR as discussed.

Could you dismiss the "changes requested" state and re-review when you get
a chance? Thank you!

@targos this addresses the deprecation warnings you reported in #52464.

mcollina
mcollina approved these changes May 1, 2026
View reviewed changes

@mcollina 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.

lgtm

targos
targos requested changes May 2, 2026
View reviewed changes

@targos targos 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.

This change doesn’t help

@omghante
omghante force-pushed the fix/52464-cares-deprecation-warnings branch 2 times, most recently from 372e4bc to 64b6870 Compare May 2, 2026 12:49
@omghante
src: migrate deprecated c-ares APIs
3616cdb 
Signed-off-by: om-ghante <mr.omghante1@gmail.com>
@omghante
omghante force-pushed the fix/52464-cares-deprecation-warnings branch from 64b6870 to 3616cdb Compare May 2, 2026 13:57
@omghante

omghante commented May 2, 2026

Copy link
Copy Markdown
Contributor Author

@targos Thanks for the feedback reworked! The PR now fully migrates all reply-parser APIs to ares_dns_parse; only the 3 server-management calls retain pragmas since the CSV replacements drop IPv6 link-local addresses.

@omghante
omghante requested a review from targos May 3, 2026 19:11
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actions github-actions Bot added the stale label Aug 2, 2026
@omghante

omghante commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Keep this open

@github-actions github-actions Bot removed the stale label Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@thisalihassan thisalihassan Awaiting requested review from thisalihassan
@targos targos Awaiting requested review from targos

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

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.

cares deprecation warnings

5 participants

@omghante @nodejs-github-bot @mcollina @targos @thisalihassan