Cleanup ares_library_init from Envoy ProcessWide::ProcessWide()

[yanjunxiang-google]

Title: Cleanup ares_library_init from Envoy ProcessWide::ProcessWide()

Description: With Envoy DNS as an extension now(#17479), c-ares DNS library may not be needed in some cases. Thus, we should not call ares_library_init(ARES_LIB_INIT_ALL) in ProcessWide::ProcessWide().

Same for call ares_library_cleanup() in ProcessWide::~ProcessWide()

https://source.corp.google.com/piper///depot/google3/third_party/envoy/src/source/exe/process_wide.cc;rcl=404246718;l=20
https://source.corp.google.com/piper///depot/google3/third_party/envoy/src/source/exe/process_wide.cc;rcl=404246718;l=46

[yanjunxiang-google]

Just an initial thought. This ares_library_init() can be moved to the c-ares DNS resolver object creation time, like here:

envoy/source/extensions/network/dns_resolver/cares/dns_impl.cc

Line 26 in f76ae02

DnsResolverImpl::DnsResolverImpl(

And ares_library_cleanup() can be called in the e c-ares DNS resolver object destruction time, like here:

envoy/source/extensions/network/dns_resolver/cares/dns_impl.cc

Line 38 in f76ae02

DnsResolverImpl::~DnsResolverImpl() {

[yanjunxiang-google]

/assign @jmarantz

[yanjunxiang-google]

/assign @yanavlasov

[jmarantz]

RE putting it in the DnsResolverImpl's ctor/dtor makes sense to me. I think if you dive into the impl inside c_ares you will see that it tracks nested instantiations so if you init twice, it won't really cleanup until you cleanup twice. However, it does not use any kind of thread-safe structure to do that (atomic or mutex) so this will race if the DnsResolverImpl objects are created/destroyed across threads.

So we should either do ASSERT(Thread::isMainOrTestThread()) in the ctor/dtor, or we should add a wrapper object that makes a lazy-init mutable singleton with a mutex to de-race the init/cleanup of the c_area code.

Alternatively we could try an upstream change to c_ares to make its init/destruct thread-safe.

[yanjunxiang-google]

HI, Josh,

That's a good point. Yeah, simply adding it in DnsResolverImpl's ctor/dtor is not thread safe since we do support DnsResolver creation/destruction in both main threads (e.g. dns_cache_impl, cluster, ...) and worker threads(e.g. dns_filter). Thus if we want to go this approach, as you mentioned, we need either using mutex to de-race, or refactor code to make it thread safe. The other option I am thinking is to add it the CaresDnsResolverFactory ctor/dtor, which is done during the bootstrapping, and main thread only:

envoy/source/extensions/network/dns_resolver/cares/dns_impl.cc

Line 335 in f76ae02

class CaresDnsResolverFactory : public DnsResolverFactory {

Does this make sense to you?
Thanks,
Yanjun

[yanjunxiang-google]

/assign @htuch

[jmarantz]

CaresDnsResolverFactory seems like a great place to do it, if it's only created when someone configures c_ares. We'd be back in the same boat if that gets created eagerly.

alternatively, you could init c_ares from CaresDnsResolverFactory::createDnsResolver(), but only once per factory, and guarded by a mutex or atomic member var in CaresDnsResolverFactory to avoid races.

[htuch]

+1 factory makes sense here.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[yanjunxiang-google]

Closed by: #21884