Skip to content
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.

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

asyn-thread: use c-ares to resolve HTTPS RR #16054

Closed
wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Jan 20, 2025

Allow building with c-ares and yet use threaded resolver for the main host A/AAAA resolving.

--with-ares provide the c-ares install path and defaults to use c-ares for name resolving

--with-threaded-resolver still uses c-ares in the build (for HTTPS) but uses the threaded resolver for "normal resolves".

@bagder bagder added the name lookup DNS and related tech label Jan 20, 2025
@github-actions github-actions bot added the tests label Jan 20, 2025
@bagder bagder force-pushed the bagder/cares-threaded branch from 57e6156 to faacaa6 Compare January 20, 2025 15:57
@dfandrich
Copy link
Contributor

dfandrich commented Jan 20, 2025 via email

@bagder
Copy link
Member Author

bagder commented Jan 20, 2025

Yeah, I'm also working on #16052

@bagder bagder force-pushed the bagder/cares-threaded branch from 21614cd to 8f73088 Compare January 21, 2025 10:42
@bagder bagder added the feature-window A merge of this requires an open feature window label Jan 22, 2025
@bagder bagder force-pushed the bagder/cares-threaded branch from 3f87202 to c844334 Compare January 22, 2025 08:23
@github-actions github-actions bot added libcurl API CI Continuous Integration labels Jan 22, 2025
@bagder bagder marked this pull request as ready for review January 22, 2025 08:46
@bagder

This comment was marked as outdated.

@vszakats

This comment was marked as resolved.

@bagder
Copy link
Member Author

bagder commented Jan 23, 2025

Thanks @vszakats I'll try that!

@bagder bagder force-pushed the bagder/cares-threaded branch from d125c4f to da34e9a Compare January 23, 2025 21:48
@bagder

This comment was marked as resolved.

@bagder
Copy link
Member Author

bagder commented Jan 24, 2025

@vszakats if you get a few minutes over and want to help out, I could use an extra set of eyes on this to help me nail down this issue.

@bagder bagder removed the feature-window A merge of this requires an open feature window label Jan 24, 2025
@bagder
Copy link
Member Author

bagder commented Jan 24, 2025

Ah I can repro now, it is unity-build related.

Allow building with c-ares and yet use threaded resolver for the main
host A/AAAA resolving:

  `--with-ares` provides the c-ares install path and defaults to use
  c-ares for name resolving

  `--with-threaded-resolver` still uses c-ares in the build (for HTTPS)
  but uses the threaded resolver for "normal" resolves.

It works similarly for cmake: ENABLE_ARES enables ares, and if
ENABLE_THREADED_RESOLVER also is set, c-ares is used for HTTPS RR and
the threaded resolver for "normal" resolves.

HTTPSRR and c-ares-rr are new features return by curl_version_info() and
thus shown by curl -V

The c-ares-rr feature bit is there to make it possible to distinguish
between builds using c-ares for all name resolves and builds that use
the threaded resolves for the regular name resolves and c-ares for
HTTPSRR only. "c-ares-rr" means it does not use c-ares for "plain" name
resolves.

Closes #16054
@bagder bagder force-pushed the bagder/cares-threaded branch from dd977cf to 93b7c64 Compare January 24, 2025 15:17
@vszakats
Copy link
Member

Sorry, got here late and it seems resolved now. If there's something still, I can take a look.

@bagder
Copy link
Member Author

bagder commented Jan 24, 2025

Thanks. You know, sometimes you just need to vent a little and then suddenly the solution appears! 😁

@bagder
Copy link
Member Author

bagder commented Jan 24, 2025

I will merge this in a day or two.

@bagder bagder closed this in 0d4fdbf Jan 25, 2025
@bagder bagder deleted the bagder/cares-threaded branch January 25, 2025 22:46
@vszakats
Copy link
Member

vszakats commented Jan 26, 2025

I'm trying to make a build like this (also for curl-for-win), and c-ares-rr appears in the configure output (both with CMake and autotools), but missing from curl -V.

It would need both CURLRES_ARES and CURLRES_THREADED defined, but curl_setup.h
seems to enable either one or the other:

curl/lib/curl_setup.h

Lines 705 to 710 in 3e552ef

#if defined(USE_THREADS_POSIX) || defined(USE_THREADS_WIN32)
# define CURLRES_ASYNCH
# define CURLRES_THREADED
#elif defined(USE_ARES)
# define CURLRES_ASYNCH
# define CURLRES_ARES

Is it too early to try or am I doing some mistake?

@bagder
Copy link
Member Author

bagder commented Jan 26, 2025

Oh right, I clearly broke that when I reversed it. Let me give it a poke...

vszakats added a commit that referenced this pull request Feb 3, 2025
- replace deprecated `ares_init()` call with `ares_init_options()`.
  Follow-up to 0d4fdbf #16054

- dedupe `CARES_STATICLIB` initalizations into `curl_setup.h`, to
  ensure it's defined before the first (and every) `ares.h` include and
  avoid a potential confusion.

- move `CARES_NO_DEPRECATED` from build level to `curl_setup.h`.
  To work regardless of build system.
  It is necessary because curl calls `ares_getsock()` from two places,
  of which one feeds a chain of wrappers: `Curl_ares_getsock()`,
  `Curl_resolver_getsock()`, `Curl_resolv_getsock()`.

Closes #16131
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration libcurl API name lookup DNS and related tech tests
Development

Successfully merging this pull request may close these issues.

3 participants