diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 505b1bea7dc1..b07a0780422d 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -84,6 +84,13 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i // We receive ARES_EDESTRUCTION when destructing with pending queries. if (status == ARES_EDESTRUCTION) { ASSERT(owned_); + // This destruction might have been triggered by a peer PendingResolution that received a + // ARES_ECONNREFUSED. If the PendingResolution has not been cancelled that means that the + // callback_ target _should_ still be around. In that case, raise the callback_ so the target + // can be done with this query and initiate a new one. + if (!cancelled_) { + callback_({}); + } delete this; return; } @@ -251,8 +258,9 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, // Enable timer to wake us up if the request times out. updateAresTimer(); - // The PendingResolution will self-delete when the request completes - // (including if cancelled or if ~DnsResolverImpl() happens). + // The PendingResolution will self-delete when the request completes (including if cancelled or + // if ~DnsResolverImpl() happens via ares_destroy() and subsequent handling of ARES_EDESTRUCTION + // in DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback()). pending_resolution->owned_ = true; return pending_resolution.release(); } diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 65bc03c10dc9..68847669876c 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -476,20 +476,41 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, DnsImplTest, TestUtility::ipTestParamsToString); // Validate that when DnsResolverImpl is destructed with outstanding requests, -// that we don't invoke any callbacks. This is a regression test from +// that we don't invoke any callbacks if the query was cancelled. This is a regression test from // development, where segfaults were encountered due to callback invocations on // destruction. TEST_P(DnsImplTest, DestructPending) { - EXPECT_NE(nullptr, resolver_->resolve("", DnsLookupFamily::V4Only, - [&](std::list&& results) -> void { - FAIL(); - UNREFERENCED_PARAMETER(results); - })); + ActiveDnsQuery* query = resolver_->resolve("", DnsLookupFamily::V4Only, + [&](std::list&& results) -> void { + FAIL(); + UNREFERENCED_PARAMETER(results); + }); + ASSERT_NE(nullptr, query); + query->cancel(); // Also validate that pending events are around to exercise the resource // reclamation path. EXPECT_GT(peer_->events().size(), 0U); } +TEST_P(DnsImplTest, DestructCallback) { + server_->addHosts("some.good.domain", {"201.134.56.7"}, RecordType::A); + + ActiveDnsQuery* query = resolver_->resolve("some.domain", DnsLookupFamily::Auto, + [&](std::list &&) -> void { + query = nullptr; + dispatcher_->exit(); + }); + + // This simulates destruction thanks to another query setting the dirty_channel_ bit, thus causing + // a subsequent result to call ares_destroy. + peer_->resetChannelTcpOnly(zero_timeout()); + ares_set_servers_ports_csv(peer_->channel(), socket_->localAddress()->asString().c_str()); + + dispatcher_->run(Event::Dispatcher::RunType::Block); + + ASSERT_EQ(nullptr, query); +} + // Validate basic success/fail lookup behavior. The empty request will connect // to TestDnsServer, but localhost should resolve via the hosts file with no // asynchronous behavior or network events. @@ -946,16 +967,12 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, DnsImplAresFlagsForTcpTest, TEST_P(DnsImplAresFlagsForTcpTest, TcpLookupsEnabled) { server_->addCName("root.cnam.domain", "result.cname.domain"); server_->addHosts("result.cname.domain", {"201.134.56.7"}, RecordType::A); - std::list address_list; ares_options opts{}; int optmask = 0; EXPECT_EQ(ARES_SUCCESS, ares_save_options(peer_->channel(), &opts, &optmask)); EXPECT_TRUE((opts.flags & ARES_FLAG_USEVC) == ARES_FLAG_USEVC); EXPECT_NE(nullptr, resolver_->resolve("root.cnam.domain", DnsLookupFamily::Auto, - [&](std::list&& results) -> void { - address_list = getAddressList(results); - dispatcher_->exit(); - })); + [&](std::list &&) -> void {})); ares_destroy_options(&opts); } @@ -974,16 +991,12 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, DnsImplAresFlagsForUdpTest, TEST_P(DnsImplAresFlagsForUdpTest, UdpLookupsEnabled) { server_->addCName("root.cnam.domain", "result.cname.domain"); server_->addHosts("result.cname.domain", {"201.134.56.7"}, RecordType::A); - std::list address_list; ares_options opts{}; int optmask = 0; EXPECT_EQ(ARES_SUCCESS, ares_save_options(peer_->channel(), &opts, &optmask)); EXPECT_FALSE((opts.flags & ARES_FLAG_USEVC) == ARES_FLAG_USEVC); EXPECT_NE(nullptr, resolver_->resolve("root.cnam.domain", DnsLookupFamily::Auto, - [&](std::list&& results) -> void { - address_list = getAddressList(results); - dispatcher_->exit(); - })); + [&](std::list &&) -> void {})); ares_destroy_options(&opts); }