Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions envoy/network/dns_resolver.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class DnsResolverFactory : public Config::TypedFactory {
const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config) const PURE;

std::string category() const override { return std::string(DnsResolverCategory); }
virtual void cleanup() {}
};

} // namespace Network
Expand Down
2 changes: 1 addition & 1 deletion source/exe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,8 @@ envoy_cc_library(
name = "process_wide_lib",
srcs = ["process_wide.cc"],
hdrs = ["process_wide.h"],
external_deps = ["ares"],
deps = [
"//envoy/network:dns_resolver_interface",
"//source/common/common:assert_lib",
"//source/common/event:libevent_lib",
"//source/common/http/http2:nghttp2_lib",
Expand Down
11 changes: 7 additions & 4 deletions source/exe/process_wide.cc
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
#include "source/exe/process_wide.h"

#include "envoy/network/dns_resolver.h"

#include "source/common/common/assert.h"
#include "source/common/event/libevent.h"
#include "source/common/http/http2/nghttp2.h"
#include "source/server/proto_descriptors.h"

#include "ares.h"

namespace Envoy {
namespace {

Expand All @@ -30,7 +30,6 @@ ProcessWide::ProcessWide() {
if (init_data.count_++ == 0) {
// TODO(mattklein123): Audit the following as not all of these have to be re-initialized in the
// edge case where something does init/destroy/init/destroy.
ares_library_init(ARES_LIB_INIT_ALL);
Event::Libevent::Global::initialize();
Envoy::Server::validateProtoDescriptors();
Http::Http2::initializeNghttp2Logging();
Expand Down Expand Up @@ -58,7 +57,11 @@ ProcessWide::~ProcessWide() {

ASSERT(init_data.count_ > 0);
if (--init_data.count_ == 0) {
ares_library_cleanup();
// Cleanup c-ares library if it is linked in.
Comment thread
yanjunxiang-google marked this conversation as resolved.
Outdated
if (auto* dns_factory = Config::Utility::getAndCheckFactoryByName<Network::DnsResolverFactory>(
std::string(Network::CaresDnsResolver), true)) {
dns_factory->cleanup();
}
}
}

Expand Down
16 changes: 16 additions & 0 deletions source/extensions/network/dns_resolver/cares/dns_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,7 @@ DnsResolverImpl::AddrInfoPendingResolution::availableInterfaces() {
// c-ares DNS resolver factory
class CaresDnsResolverFactory : public DnsResolverFactory {
public:
CaresDnsResolverFactory() : ares_library_initialized_(false) {}
Comment thread
yanjunxiang-google marked this conversation as resolved.
Outdated
std::string name() const override { return std::string(CaresDnsResolver); }

ProtobufTypes::MessagePtr createEmptyConfigProto() override {
Expand All @@ -507,6 +508,11 @@ class CaresDnsResolverFactory : public DnsResolverFactory {
typed_dns_resolver_config) const override {
envoy::extensions::network::dns_resolver::cares::v3::CaresDnsResolverConfig cares;
std::vector<Network::Address::InstanceConstSharedPtr> resolvers;
// Initialize c-ares library in case first time.
if (!ares_library_initialized_) {
ares_library_init(ARES_LIB_INIT_ALL);
ares_library_initialized_ = true;
Comment thread
yanjunxiang-google marked this conversation as resolved.
Outdated
}

ASSERT(dispatcher.isThreadSafe());
// Only c-ares DNS factory will call into this function.
Expand All @@ -521,6 +527,16 @@ class CaresDnsResolverFactory : public DnsResolverFactory {
}
return std::make_shared<Network::DnsResolverImpl>(cares, dispatcher, resolvers);
}

void cleanup() override {
// Cleanup c-ares library if initialized.
if (ares_library_initialized_) {
ares_library_cleanup();

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.

Can we move this cleanup to the ares factory destructor? That would feel a little cleaner.

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.

Can't do it in the destructor since factories are statically initialized and will race with whatever static clean-up happens in c-ares.

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.

Makes sense. Can you comment that the destructor is not called in deterministic order as the object is statically constructed.

}
}

private:
mutable bool ares_library_initialized_;
};

// Register the CaresDnsResolverFactory
Expand Down