diff --git a/BUILD b/BUILD index 3dceb6d0922..3d703e8cee6 100644 --- a/BUILD +++ b/BUILD @@ -1841,6 +1841,12 @@ grpc_cc_library( srcs = [ "src/core/lib/event_engine/resolved_address.cc", ], + hdrs = [ + "src/core/lib/event_engine/handle_containers.h", + ], + external_deps = [ + "absl/container:flat_hash_set", + ], deps = [ "event_engine_base_hdrs", "gpr_base", @@ -3765,13 +3771,14 @@ grpc_cc_library( "src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h", ], external_deps = [ + "absl/container:flat_hash_set", + "absl/container:inlined_vector", "absl/base:core_headers", "absl/memory", "absl/status", "absl/status:statusor", "absl/strings", "absl/strings:str_format", - "absl/container:inlined_vector", "address_sorting", "cares", ], @@ -3780,6 +3787,7 @@ grpc_cc_library( "config", "debug_location", "error", + "event_engine_common", "gpr_base", "grpc_base", "grpc_client_channel", diff --git a/CMakeLists.txt b/CMakeLists.txt index f00410d2f86..082bb9bc086 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -132,6 +132,7 @@ set(gRPC_ABSL_USED_TARGETS absl_fast_type_id absl_fixed_array absl_flat_hash_map + absl_flat_hash_set absl_function_ref absl_graphcycles_internal absl_hash @@ -2305,6 +2306,7 @@ target_link_libraries(grpc ${_gRPC_UPB_LIBRARIES} ${_gRPC_ALLTARGETS_LIBRARIES} absl::flat_hash_map + absl::flat_hash_set absl::inlined_vector absl::bind_front absl::hash @@ -2850,6 +2852,7 @@ target_link_libraries(grpc_unsecure ${_gRPC_UPB_LIBRARIES} ${_gRPC_ALLTARGETS_LIBRARIES} absl::flat_hash_map + absl::flat_hash_set absl::inlined_vector absl::bind_front absl::hash @@ -8431,7 +8434,6 @@ target_include_directories(cel_authorization_engine_test target_link_libraries(cel_authorization_engine_test ${_gRPC_PROTOBUF_LIBRARIES} ${_gRPC_ALLTARGETS_LIBRARIES} - absl::flat_hash_set grpc_test_util ) @@ -19147,7 +19149,7 @@ generate_pkgconfig( "gRPC" "high performance general RPC framework" "${gRPC_CORE_VERSION}" - "gpr openssl absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" + "gpr openssl absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_flat_hash_set absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" "-lgrpc -laddress_sorting -lre2 -lupb -lcares -lz" "" "grpc.pc") @@ -19157,7 +19159,7 @@ generate_pkgconfig( "gRPC unsecure" "high performance general RPC framework without SSL" "${gRPC_CORE_VERSION}" - "gpr absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" + "gpr absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_flat_hash_set absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" "-lgrpc_unsecure" "" "grpc_unsecure.pc") @@ -19167,7 +19169,7 @@ generate_pkgconfig( "gRPC++" "C++ wrapper for gRPC" "${gRPC_CPP_VERSION}" - "grpc absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" + "grpc absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_flat_hash_set absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" "-lgrpc++" "" "grpc++.pc") @@ -19177,7 +19179,7 @@ generate_pkgconfig( "gRPC++ unsecure" "C++ wrapper for gRPC without SSL" "${gRPC_CPP_VERSION}" - "grpc_unsecure absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" + "grpc_unsecure absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_flat_hash_set absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" "-lgrpc++_unsecure" "" "grpc++_unsecure.pc") diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index cad7c6e2d89..54dd25910c0 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -728,6 +728,7 @@ libs: - src/core/lib/debug/trace.h - src/core/lib/event_engine/channel_args_endpoint_config.h - src/core/lib/event_engine/event_engine_factory.h + - src/core/lib/event_engine/handle_containers.h - src/core/lib/event_engine/sockaddr.h - src/core/lib/gprpp/atomic_utils.h - src/core/lib/gprpp/bitset.h @@ -1643,6 +1644,7 @@ libs: - src/core/tsi/transport_security_grpc.cc deps: - absl/container:flat_hash_map + - absl/container:flat_hash_set - absl/container:inlined_vector - absl/functional:bind_front - absl/hash:hash @@ -1913,6 +1915,7 @@ libs: - src/core/lib/debug/trace.h - src/core/lib/event_engine/channel_args_endpoint_config.h - src/core/lib/event_engine/event_engine_factory.h + - src/core/lib/event_engine/handle_containers.h - src/core/lib/event_engine/sockaddr.h - src/core/lib/gprpp/atomic_utils.h - src/core/lib/gprpp/bitset.h @@ -2417,6 +2420,7 @@ libs: - src/core/tsi/transport_security_grpc.cc deps: - absl/container:flat_hash_map + - absl/container:flat_hash_set - absl/container:inlined_vector - absl/functional:bind_front - absl/hash:hash @@ -4830,7 +4834,6 @@ targets: - src/core/lib/security/authorization/cel_authorization_engine.cc - test/core/security/cel_authorization_engine_test.cc deps: - - absl/container:flat_hash_set - grpc_test_util - name: certificate_provider_registry_test gtest: true diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 05dd79290b9..f8dd34178ef 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -198,6 +198,7 @@ Pod::Spec.new do |s| ss.dependency 'abseil/base/base', abseil_version ss.dependency 'abseil/base/core_headers', abseil_version ss.dependency 'abseil/container/flat_hash_map', abseil_version + ss.dependency 'abseil/container/flat_hash_set', abseil_version ss.dependency 'abseil/container/inlined_vector', abseil_version ss.dependency 'abseil/functional/bind_front', abseil_version ss.dependency 'abseil/hash/hash', abseil_version @@ -669,6 +670,7 @@ Pod::Spec.new do |s| 'src/core/lib/debug/trace.h', 'src/core/lib/event_engine/channel_args_endpoint_config.h', 'src/core/lib/event_engine/event_engine_factory.h', + 'src/core/lib/event_engine/handle_containers.h', 'src/core/lib/event_engine/sockaddr.h', 'src/core/lib/gpr/alloc.h', 'src/core/lib/gpr/env.h', @@ -1484,6 +1486,7 @@ Pod::Spec.new do |s| 'src/core/lib/debug/trace.h', 'src/core/lib/event_engine/channel_args_endpoint_config.h', 'src/core/lib/event_engine/event_engine_factory.h', + 'src/core/lib/event_engine/handle_containers.h', 'src/core/lib/event_engine/sockaddr.h', 'src/core/lib/gpr/alloc.h', 'src/core/lib/gpr/env.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 053b5ea9448..738c1a439f3 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -174,6 +174,7 @@ Pod::Spec.new do |s| ss.dependency 'abseil/base/base', abseil_version ss.dependency 'abseil/base/core_headers', abseil_version ss.dependency 'abseil/container/flat_hash_map', abseil_version + ss.dependency 'abseil/container/flat_hash_set', abseil_version ss.dependency 'abseil/container/inlined_vector', abseil_version ss.dependency 'abseil/functional/bind_front', abseil_version ss.dependency 'abseil/hash/hash', abseil_version @@ -1034,6 +1035,7 @@ Pod::Spec.new do |s| 'src/core/lib/event_engine/default_event_engine_factory.cc', 'src/core/lib/event_engine/event_engine.cc', 'src/core/lib/event_engine/event_engine_factory.h', + 'src/core/lib/event_engine/handle_containers.h', 'src/core/lib/event_engine/memory_allocator.cc', 'src/core/lib/event_engine/resolved_address.cc', 'src/core/lib/event_engine/sockaddr.cc', @@ -2088,6 +2090,7 @@ Pod::Spec.new do |s| 'src/core/lib/debug/trace.h', 'src/core/lib/event_engine/channel_args_endpoint_config.h', 'src/core/lib/event_engine/event_engine_factory.h', + 'src/core/lib/event_engine/handle_containers.h', 'src/core/lib/event_engine/sockaddr.h', 'src/core/lib/gpr/alloc.h', 'src/core/lib/gpr/env.h', diff --git a/grpc.gemspec b/grpc.gemspec index 07d91ce716c..f466c0beed1 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -949,6 +949,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/event_engine/default_event_engine_factory.cc ) s.files += %w( src/core/lib/event_engine/event_engine.cc ) s.files += %w( src/core/lib/event_engine/event_engine_factory.h ) + s.files += %w( src/core/lib/event_engine/handle_containers.h ) s.files += %w( src/core/lib/event_engine/memory_allocator.cc ) s.files += %w( src/core/lib/event_engine/resolved_address.cc ) s.files += %w( src/core/lib/event_engine/sockaddr.cc ) @@ -1562,6 +1563,7 @@ Gem::Specification.new do |s| s.files += %w( third_party/abseil-cpp/absl/base/thread_annotations.h ) s.files += %w( third_party/abseil-cpp/absl/container/fixed_array.h ) s.files += %w( third_party/abseil-cpp/absl/container/flat_hash_map.h ) + s.files += %w( third_party/abseil-cpp/absl/container/flat_hash_set.h ) s.files += %w( third_party/abseil-cpp/absl/container/inlined_vector.h ) s.files += %w( third_party/abseil-cpp/absl/container/internal/common.h ) s.files += %w( third_party/abseil-cpp/absl/container/internal/compressed_tuple.h ) diff --git a/grpc.gyp b/grpc.gyp index de8c620bde0..5706f0d037d 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -361,6 +361,7 @@ 'type': 'static_library', 'dependencies': [ 'absl/container:flat_hash_map', + 'absl/container:flat_hash_set', 'absl/container:inlined_vector', 'absl/functional:bind_front', 'absl/hash:hash', @@ -1113,6 +1114,7 @@ 'type': 'static_library', 'dependencies': [ 'absl/container:flat_hash_map', + 'absl/container:flat_hash_set', 'absl/container:inlined_vector', 'absl/functional:bind_front', 'absl/hash:hash', diff --git a/package.xml b/package.xml index abf2ca44fca..4f17a0a645a 100644 --- a/package.xml +++ b/package.xml @@ -931,6 +931,7 @@ + @@ -1566,6 +1567,7 @@ + diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc index 5347d3ded0d..7d37fa67399 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc @@ -16,6 +16,7 @@ #include +#include #include #include @@ -34,6 +35,7 @@ #include "absl/strings/string_view.h" #include "absl/strings/strip.h" +#include #include #include #include @@ -64,6 +66,7 @@ #include +#include "absl/container/flat_hash_set.h" #include "absl/container/inlined_vector.h" #include "absl/strings/str_cat.h" @@ -73,6 +76,7 @@ #include "src/core/ext/filters/client_channel/resolver/polling_resolver.h" #include "src/core/lib/backoff/backoff.h" #include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/event_engine/handle_containers.h" #include "src/core/lib/gpr/string.h" #include "src/core/lib/iomgr/gethostname.h" #include "src/core/lib/iomgr/resolve_address.h" @@ -358,74 +362,83 @@ class AresClientChannelDNSResolverFactory : public ResolverFactory { class AresDNSResolver : public DNSResolver { public: - class AresRequest : public DNSResolver::Request { + class AresRequest { public: AresRequest( absl::string_view name, absl::string_view default_port, grpc_pollset_set* interested_parties, std::function>)> - on_resolve_address_done) + on_resolve_address_done, + AresDNSResolver* resolver, intptr_t aba_token) : name_(std::string(name)), default_port_(std::string(default_port)), interested_parties_(interested_parties), - on_resolve_address_done_(std::move(on_resolve_address_done)) { + on_resolve_address_done_(std::move(on_resolve_address_done)), + completed_(false), + resolver_(resolver), + aba_token_(aba_token) { GRPC_CARES_TRACE_LOG("AresRequest:%p ctor", this); GRPC_CLOSURE_INIT(&on_dns_lookup_done_, OnDnsLookupDone, this, grpc_schedule_on_exec_ctx); + MutexLock lock(&mu_); + ares_request_ = std::unique_ptr(grpc_dns_lookup_ares( + /*dns_server=*/"", name_.c_str(), default_port_.c_str(), + interested_parties_, &on_dns_lookup_done_, &addresses_, + /*balancer_addresses=*/nullptr, /*service_config_json=*/nullptr, + GRPC_DNS_ARES_DEFAULT_QUERY_TIMEOUT_MS)); + GRPC_CARES_TRACE_LOG("AresRequest:%p Start ares_request_:%p", this, + ares_request_.get()); } - ~AresRequest() override { + ~AresRequest() { GRPC_CARES_TRACE_LOG("AresRequest:%p dtor ares_request_:%p", this, ares_request_.get()); + resolver_->UnregisterRequest(task_handle()); } - void Start() override { + bool Cancel() { MutexLock lock(&mu_); - Ref().release(); // ref held by resolution - ares_request_ = std::unique_ptr(grpc_dns_lookup_ares( - "" /* dns_server */, name_.c_str(), default_port_.c_str(), - interested_parties_, &on_dns_lookup_done_, &addresses_, - nullptr /* balancer_addresses */, nullptr /* service_config_json */, - GRPC_DNS_ARES_DEFAULT_QUERY_TIMEOUT_MS)); - GRPC_CARES_TRACE_LOG("AresRequest:%p Start ares_request_:%p", this, + GRPC_CARES_TRACE_LOG("AresRequest:%p Cancel ares_request_:%p", this, ares_request_.get()); + // Cancelling the same lookup twice is a bug. + if (completed_) return false; + // OnDnsLookupDone will still be run + grpc_cancel_ares_request(ares_request_.get()); + completed_ = true; + return true; } - void Orphan() override { - { - MutexLock lock(&mu_); - GRPC_CARES_TRACE_LOG("AresRequest:%p Orphan ares_request_:%p", this, - ares_request_.get()); - if (ares_request_ != nullptr) { - grpc_cancel_ares_request(ares_request_.get()); - } - } - Unref(); + TaskHandle task_handle() { + return {reinterpret_cast(this), aba_token_}; } private: + // Called by ares when lookup has completed or when cancelled. It is always + // called exactly once. static void OnDnsLookupDone(void* arg, grpc_error_handle error) { - AresRequest* r = static_cast(arg); + AresRequest* request = static_cast(arg); + GRPC_CARES_TRACE_LOG("AresRequest:%p OnDnsLookupDone", request); + // This request is deleted and unregistered upon any exit. + std::unique_ptr deleter(request); std::vector resolved_addresses; { - MutexLock lock(&r->mu_); - GRPC_CARES_TRACE_LOG("AresRequest:%p OnDnsLookupDone error:%s", r, - grpc_error_std_string(error).c_str()); - if (r->addresses_ != nullptr) { - resolved_addresses.reserve(r->addresses_->size()); - for (const auto& server_address : *r->addresses_) { + MutexLock lock(&request->mu_); + if (request->completed_) { + return; + } + request->completed_ = true; + if (request->addresses_ != nullptr) { + resolved_addresses.reserve(request->addresses_->size()); + for (const auto& server_address : *request->addresses_) { resolved_addresses.push_back(server_address.address()); } } } - if (error == GRPC_ERROR_NONE) { - // it's safe to run this inline since we've already been scheduled - // on the ExecCtx - r->on_resolve_address_done_(std::move(resolved_addresses)); - } else { - r->on_resolve_address_done_(grpc_error_to_absl_status(error)); + if (error != GRPC_ERROR_NONE) { + request->on_resolve_address_done_(grpc_error_to_absl_status(error)); + return; } - r->Unref(); + request->on_resolve_address_done_(std::move(resolved_addresses)); } // mutex to synchronize access to this object (but not to the ares_request @@ -449,6 +462,12 @@ class AresDNSResolver : public DNSResolver { grpc_closure on_dns_lookup_done_ ABSL_GUARDED_BY(mu_); // underlying ares_request that the query is performed on std::unique_ptr ares_request_ ABSL_GUARDED_BY(mu_); + bool completed_ ABSL_GUARDED_BY(mu_); + // Parent resolver that created this request + AresDNSResolver* resolver_; + // Unique token to help distinguish this request from others that may later + // be created in the same memory location. + intptr_t aba_token_; }; // gets the singleton instance, possibly creating it first @@ -457,13 +476,17 @@ class AresDNSResolver : public DNSResolver { return instance; } - OrphanablePtr ResolveName( + TaskHandle ResolveName( absl::string_view name, absl::string_view default_port, grpc_pollset_set* interested_parties, std::function>)> on_done) override { - return MakeOrphanable(name, default_port, interested_parties, - std::move(on_done)); + MutexLock lock(&mu_); + auto* request = new AresRequest(name, default_port, interested_parties, + std::move(on_done), this, aba_token_++); + auto handle = request->task_handle(); + open_requests_.insert(handle); + return handle; } absl::StatusOr> ResolveNameBlocking( @@ -473,9 +496,30 @@ class AresDNSResolver : public DNSResolver { return default_resolver_->ResolveNameBlocking(name, default_port); } + bool Cancel(TaskHandle handle) override { + MutexLock lock(&mu_); + if (!open_requests_.contains(handle)) { + // Unknown request, possibly completed already, or an invalid handle. + return false; + } + auto* request = reinterpret_cast(handle.keys[0]); + GRPC_CARES_TRACE_LOG("AresDNSResolver:%p cancel ares_request:%p", this, + request); + return request->Cancel(); + } + private: + // Called exclusively from the AresRequest destructor. + void UnregisterRequest(TaskHandle handle) { + MutexLock lock(&mu_); + open_requests_.erase(handle); + } + // the previous default DNS resolver, used to delegate blocking DNS calls to DNSResolver* default_resolver_ = GetDNSResolver(); + Mutex mu_; + LookupTaskHandleSet open_requests_ ABSL_GUARDED_BY(mu_); + intptr_t aba_token_ ABSL_GUARDED_BY(mu_) = 0; }; bool ShouldUseAres(const char* resolver_env) { diff --git a/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc b/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc index 257fdef1bd8..42cf240616c 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc @@ -108,16 +108,15 @@ NativeClientChannelDNSResolver::~NativeClientChannelDNSResolver() { OrphanablePtr NativeClientChannelDNSResolver::StartRequest() { Ref(DEBUG_LOCATION, "dns_request").release(); - auto dns_request = GetDNSResolver()->ResolveName( + auto dns_request_handle = GetDNSResolver()->ResolveName( name_to_resolve(), kDefaultSecurePort, interested_parties(), absl::bind_front(&NativeClientChannelDNSResolver::OnResolved, this)); if (GRPC_TRACE_FLAG_ENABLED(grpc_trace_dns_resolver)) { gpr_log(GPR_DEBUG, "[dns_resolver=%p] starting request=%p", this, - dns_request.get()); + DNSResolver::HandleToString(dns_request_handle).c_str()); } - dns_request->Start(); - // Explicit type conversion to work around issue with older compilers. - return OrphanablePtr(dns_request.release()); + // Not cancellable. + return nullptr; } void NativeClientChannelDNSResolver::OnResolved( diff --git a/src/core/lib/event_engine/handle_containers.h b/src/core/lib/event_engine/handle_containers.h new file mode 100644 index 00000000000..cdc696716e9 --- /dev/null +++ b/src/core/lib/event_engine/handle_containers.h @@ -0,0 +1,56 @@ +// Copyright 2022 The gRPC Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +#ifndef GRPC_CORE_LIB_EVENT_ENGINE_HANDLE_CONTAINERS_H +#define GRPC_CORE_LIB_EVENT_ENGINE_HANDLE_CONTAINERS_H + +#include + +#include "absl/container/flat_hash_set.h" +#include "absl/hash/hash.h" + +#include + +// Used for heterogenous lookup of TaskHandles in abseil containers. +template +struct TaskHandleComparator { + struct Hash { + using HashType = std::pair; + using is_transparent = void; + size_t operator()(const TaskHandle& handle) const { + return absl::Hash()({handle.keys[0], handle.keys[1]}); + } + }; + struct Eq { + using is_transparent = void; + bool operator()(const TaskHandle& lhs, const TaskHandle& rhs) const { + return lhs.keys[0] == rhs.keys[0] && lhs.keys[1] == rhs.keys[1]; + } + }; +}; + +using TaskHandleSet = absl::flat_hash_set< + grpc_event_engine::experimental::EventEngine::TaskHandle, + TaskHandleComparator< + grpc_event_engine::experimental::EventEngine::TaskHandle>::Hash, + TaskHandleComparator< + grpc_event_engine::experimental::EventEngine::TaskHandle>::Eq>; + +using LookupTaskHandleSet = absl::flat_hash_set< + grpc_event_engine::experimental::EventEngine::DNSResolver::LookupTaskHandle, + TaskHandleComparator::Hash, + TaskHandleComparator::Eq>; + +#endif // GRPC_CORE_LIB_EVENT_ENGINE_HANDLE_CONTAINERS_H diff --git a/src/core/lib/http/httpcli.cc b/src/core/lib/http/httpcli.cc index af08130f936..0194a7a2f1c 100644 --- a/src/core/lib/http/httpcli.cc +++ b/src/core/lib/http/httpcli.cc @@ -179,10 +179,6 @@ HttpRequest::HttpRequest( grpc_schedule_on_exec_ctx); GPR_ASSERT(pollent); grpc_polling_entity_add_to_pollset_set(pollent, pollset_set_); - // Create the DNS resolver. We'll start resolving when Start is called. - dns_request_ = GetDNSResolver()->ResolveName( - uri_.authority(), uri_.scheme(), pollset_set_, - absl::bind_front(&HttpRequest::OnResolved, this)); } HttpRequest::~HttpRequest() { @@ -206,7 +202,9 @@ void HttpRequest::Start() { return; } Ref().release(); // ref held by pending DNS resolution - dns_request_->Start(); + dns_request_handle_ = GetDNSResolver()->ResolveName( + uri_.authority(), uri_.scheme(), pollset_set_, + absl::bind_front(&HttpRequest::OnResolved, this)); } void HttpRequest::Orphan() { @@ -214,14 +212,13 @@ void HttpRequest::Orphan() { MutexLock lock(&mu_); GPR_ASSERT(!cancelled_); cancelled_ = true; - dns_request_.reset(); // cancel potentially pending DNS resolution - if (connecting_) { - // gRPC's TCP connection establishment API doesn't currently have - // a mechanism for cancellation. So invoke the user callback now. The TCP - // connection will eventually complete (at least within its deadline), and - // we'll simply unref ourselves at that point. - // TODO(apolcyn): fix this to cancel the TCP connection attempt when - // an API to do so exists. + // cancel potentially pending DNS resolution. + if (dns_request_handle_.has_value() && + GetDNSResolver()->Cancel(dns_request_handle_.value())) { + Finish(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "cancelled during DNS resolution")); + Unref(); + } else if (connecting_) { Finish(GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( "HTTP request cancelled during TCP connection establishment", &overall_error_, 1)); @@ -410,16 +407,16 @@ void HttpRequest::OnResolved( absl::StatusOr> addresses_or) { RefCountedPtr unreffer(this); MutexLock lock(&mu_); - dns_request_.reset(); - if (!addresses_or.ok()) { - Finish(absl_status_to_grpc_error(addresses_or.status())); - return; - } + dns_request_handle_.reset(); if (cancelled_) { Finish(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "cancelled during DNS resolution")); return; } + if (!addresses_or.ok()) { + Finish(absl_status_to_grpc_error(addresses_or.status())); + return; + } addresses_ = std::move(*addresses_or); next_address_ = 0; NextAddress(GRPC_ERROR_NONE); diff --git a/src/core/lib/http/httpcli.h b/src/core/lib/http/httpcli.h index a3531b796ba..3061c8eab9b 100644 --- a/src/core/lib/http/httpcli.h +++ b/src/core/lib/http/httpcli.h @@ -247,7 +247,8 @@ class HttpRequest : public InternallyRefCounted { grpc_slice_buffer incoming_ ABSL_GUARDED_BY(mu_); grpc_slice_buffer outgoing_ ABSL_GUARDED_BY(mu_); grpc_error_handle overall_error_ ABSL_GUARDED_BY(mu_) = GRPC_ERROR_NONE; - OrphanablePtr dns_request_ ABSL_GUARDED_BY(mu_); + absl::optional dns_request_handle_ + ABSL_GUARDED_BY(mu_) = DNSResolver::kNullHandle; }; } // namespace grpc_core diff --git a/src/core/lib/iomgr/event_engine/resolver.h b/src/core/lib/iomgr/event_engine/resolver.h index 09c98ae7b0d..c3d9ba9c216 100644 --- a/src/core/lib/iomgr/event_engine/resolver.h +++ b/src/core/lib/iomgr/event_engine/resolver.h @@ -35,6 +35,7 @@ namespace grpc_core { namespace experimental { +#ifdef GRPC_USE_EVENT_ENGINE class EventEngineDNSResolver : public DNSResolver { public: // Gets the singleton instance, creating it first if it doesn't exist @@ -49,6 +50,7 @@ class EventEngineDNSResolver : public DNSResolver { absl::StatusOr> ResolveNameBlocking( absl::string_view name, absl::string_view default_port) override; }; +#endif // GRPC_USE_EVENT_ENGINE } // namespace experimental } // namespace grpc_core diff --git a/src/core/lib/iomgr/resolve_address.cc b/src/core/lib/iomgr/resolve_address.cc index 3dfe1bf5482..363146607e3 100644 --- a/src/core/lib/iomgr/resolve_address.cc +++ b/src/core/lib/iomgr/resolve_address.cc @@ -19,6 +19,8 @@ #include "src/core/lib/iomgr/resolve_address.h" +#include "absl/strings/str_cat.h" + #include #include @@ -29,8 +31,14 @@ namespace { DNSResolver* g_dns_resolver; } +constexpr DNSResolver::TaskHandle DNSResolver::kNullHandle; + void SetDNSResolver(DNSResolver* resolver) { g_dns_resolver = resolver; } DNSResolver* GetDNSResolver() { return g_dns_resolver; } +std::string DNSResolver::HandleToString(TaskHandle handle) { + return absl::StrCat("{", handle.keys[0], ",", handle.keys[1], "}"); +} + } // namespace grpc_core diff --git a/src/core/lib/iomgr/resolve_address.h b/src/core/lib/iomgr/resolve_address.h index 35774425b7f..e1ad4d3b788 100644 --- a/src/core/lib/iomgr/resolve_address.h +++ b/src/core/lib/iomgr/resolve_address.h @@ -25,6 +25,8 @@ #include "absl/status/statusor.h" +#include + #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/iomgr/pollset_set.h" #include "src/core/lib/iomgr/port.h" @@ -39,37 +41,40 @@ constexpr int kDefaultSecurePortInt = 443; // A singleton class used for async and blocking DNS resolution class DNSResolver { public: - // Tracks a single asynchronous DNS resolution attempt. The DNS - // resolution should be arranged to be cancelled as soon as possible - // when Orphan is called. - class Request : public InternallyRefCounted { - public: - // Begins async DNS resolution - virtual void Start() = 0; - }; + using TaskHandle = ::grpc_event_engine::experimental::EventEngine:: + DNSResolver::LookupTaskHandle; + static constexpr TaskHandle kNullHandle{0, 0}; virtual ~DNSResolver() {} + static std::string HandleToString(TaskHandle handle); + // Asynchronously resolve name. Use \a default_port if a port isn't designated // in \a name, otherwise use the port in \a name. On completion, \a on_done is // invoked with the result. // // Note for implementations: calls may acquire locks in \a on_done which - // were previously held while calling Request::Start(). Therefore, - // implementations must not invoke \a on_done inline from the call to - // Request::Start(). The DNSCallbackExecCtxScheduler utility may help address - // this. - virtual OrphanablePtr ResolveName( + // were previously held while starting the request. Therefore, + // implementations must not invoke \a on_done inline from the call site that + // starts the request. The DNSCallbackExecCtxScheduler utility may help + // address this. + virtual TaskHandle ResolveName( absl::string_view name, absl::string_view default_port, grpc_pollset_set* interested_parties, std::function>)> - on_done) GRPC_MUST_USE_RESULT = 0; + on_done) = 0; // Resolve name in a blocking fashion. Use \a default_port if a port isn't // designated in \a name, otherwise use the port in \a name. virtual absl::StatusOr> ResolveNameBlocking(absl::string_view name, absl::string_view default_port) = 0; + + // This shares the same semantics with \a EventEngine::Cancel: successfully + // cancelled lookups will not have their callbacks executed, and this + // method returns true. If a TaskHandle is unknown, this method should return + // false. + virtual bool Cancel(TaskHandle handle) = 0; }; // Override the active DNS resolver which should be used for all DNS diff --git a/src/core/lib/iomgr/resolve_address_impl.h b/src/core/lib/iomgr/resolve_address_impl.h index 0f912182758..6c945a56cac 100644 --- a/src/core/lib/iomgr/resolve_address_impl.h +++ b/src/core/lib/iomgr/resolve_address_impl.h @@ -26,9 +26,8 @@ namespace grpc_core { -// A fire and forget class used by DNSResolver::Request implementations to -// schedule DNS resolution callbacks on the ExecCtx, which is frequently -// necessary to avoid lock inversion related problems. +// A fire and forget class to schedule DNS resolution callbacks on the ExecCtx, +// which is frequently necessary to avoid lock inversion related problems. class DNSCallbackExecCtxScheduler { public: DNSCallbackExecCtxScheduler( diff --git a/src/core/lib/iomgr/resolve_address_posix.cc b/src/core/lib/iomgr/resolve_address_posix.cc index d3217362062..7543e06679f 100644 --- a/src/core/lib/iomgr/resolve_address_posix.cc +++ b/src/core/lib/iomgr/resolve_address_posix.cc @@ -45,7 +45,7 @@ namespace grpc_core { namespace { -class NativeDNSRequest : public DNSResolver::Request { +class NativeDNSRequest { public: NativeDNSRequest( absl::string_view name, absl::string_view default_port, @@ -53,18 +53,9 @@ class NativeDNSRequest : public DNSResolver::Request { on_done) : name_(name), default_port_(default_port), on_done_(std::move(on_done)) { GRPC_CLOSURE_INIT(&request_closure_, DoRequestThread, this, nullptr); - } - - // Starts the resolution - void Start() override { - Ref().release(); // ref held by callback Executor::Run(&request_closure_, GRPC_ERROR_NONE, ExecutorType::RESOLVER); } - // This is a no-op for the native resolver. Note - // that no I/O polling is required for the resolution to finish. - void Orphan() override { Unref(); } - private: // Callback to be passed to grpc Executor to asynch-ify // ResolveNameBlocking @@ -74,7 +65,7 @@ class NativeDNSRequest : public DNSResolver::Request { GetDNSResolver()->ResolveNameBlocking(r->name_, r->default_port_); // running inline is safe since we've already been scheduled on the executor r->on_done_(std::move(result)); - r->Unref(); + delete r; } const std::string name_; @@ -91,13 +82,14 @@ NativeDNSResolver* NativeDNSResolver::GetOrCreate() { return instance; } -OrphanablePtr NativeDNSResolver::ResolveName( +DNSResolver::TaskHandle NativeDNSResolver::ResolveName( absl::string_view name, absl::string_view default_port, grpc_pollset_set* /* interested_parties */, std::function>)> on_done) { - return MakeOrphanable(name, default_port, - std::move(on_done)); + // self-deleting class + new NativeDNSRequest(name, default_port, std::move(on_done)); + return kNullHandle; } absl::StatusOr> @@ -181,6 +173,8 @@ done: return error_result; } +bool NativeDNSResolver::Cancel(TaskHandle /*handle*/) { return false; } + } // namespace grpc_core #endif diff --git a/src/core/lib/iomgr/resolve_address_posix.h b/src/core/lib/iomgr/resolve_address_posix.h index 6362d02d43c..1189e6ce201 100644 --- a/src/core/lib/iomgr/resolve_address_posix.h +++ b/src/core/lib/iomgr/resolve_address_posix.h @@ -32,7 +32,7 @@ class NativeDNSResolver : public DNSResolver { // Gets the singleton instance, creating it first if it doesn't exist static NativeDNSResolver* GetOrCreate(); - OrphanablePtr ResolveName( + TaskHandle ResolveName( absl::string_view name, absl::string_view default_port, grpc_pollset_set* interested_parties, std::function>)> @@ -40,6 +40,9 @@ class NativeDNSResolver : public DNSResolver { absl::StatusOr> ResolveNameBlocking( absl::string_view name, absl::string_view default_port) override; + + // NativeDNSResolver does not support cancellation. + bool Cancel(TaskHandle handle) override; }; } // namespace grpc_core diff --git a/src/core/lib/iomgr/resolve_address_windows.cc b/src/core/lib/iomgr/resolve_address_windows.cc index 90592312cfd..406d7af5672 100644 --- a/src/core/lib/iomgr/resolve_address_windows.cc +++ b/src/core/lib/iomgr/resolve_address_windows.cc @@ -48,7 +48,7 @@ namespace grpc_core { namespace { -class NativeDNSRequest : public DNSResolver::Request { +class NativeDNSRequest { public: NativeDNSRequest( absl::string_view name, absl::string_view default_port, @@ -56,18 +56,9 @@ class NativeDNSRequest : public DNSResolver::Request { on_done) : name_(name), default_port_(default_port), on_done_(std::move(on_done)) { GRPC_CLOSURE_INIT(&request_closure_, DoRequestThread, this, nullptr); - } - - // Starts the resolution - void Start() override { - Ref().release(); // ref held by callback Executor::Run(&request_closure_, GRPC_ERROR_NONE, ExecutorType::RESOLVER); } - // This is a no-op for the native resolver. Note - // that no I/O polling is required for the resolution to finish. - void Orphan() override { Unref(); } - private: // Callback to be passed to grpc Executor to asynch-ify // ResolveNameBlocking @@ -77,7 +68,7 @@ class NativeDNSRequest : public DNSResolver::Request { GetDNSResolver()->ResolveNameBlocking(r->name_, r->default_port_); // running inline is safe since we've already been scheduled on the executor r->on_done_(std::move(result)); - r->Unref(); + delete r; } const std::string name_; @@ -94,13 +85,13 @@ NativeDNSResolver* NativeDNSResolver::GetOrCreate() { return instance; } -OrphanablePtr NativeDNSResolver::ResolveName( +DNSResolver::TaskHandle NativeDNSResolver::ResolveName( absl::string_view name, absl::string_view default_port, grpc_pollset_set* /* interested_parties */, std::function>)> on_done) { - return MakeOrphanable(name, default_port, - std::move(on_done)); + new NativeDNSRequest(name, default_port, std::move(on_done)); + return kNullHandle; } absl::StatusOr> @@ -166,6 +157,8 @@ done: return error_result; } +bool NativeDNSResolver::Cancel(TaskHandle /*handle*/) { return false; } + } // namespace grpc_core #endif diff --git a/src/core/lib/iomgr/resolve_address_windows.h b/src/core/lib/iomgr/resolve_address_windows.h index 403c4ff0d13..99cd6b6dfb0 100644 --- a/src/core/lib/iomgr/resolve_address_windows.h +++ b/src/core/lib/iomgr/resolve_address_windows.h @@ -32,7 +32,7 @@ class NativeDNSResolver : public DNSResolver { // Gets the singleton instance, creating it first if it doesn't exist static NativeDNSResolver* GetOrCreate(); - OrphanablePtr ResolveName( + TaskHandle ResolveName( absl::string_view name, absl::string_view default_port, grpc_pollset_set* interested_parties, std::function>)> @@ -40,6 +40,9 @@ class NativeDNSResolver : public DNSResolver { absl::StatusOr> ResolveNameBlocking( absl::string_view name, absl::string_view default_port) override; + + // NativeDNSResolver does not support cancellation. + bool Cancel(TaskHandle handle) override; }; } // namespace grpc_core diff --git a/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc b/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc index 85f7720067b..3e5323fb880 100644 --- a/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc +++ b/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc @@ -66,7 +66,7 @@ class TestDNSResolver : public grpc_core::DNSResolver { public: // Wrapper around default resolve_address in order to count the number of // times we incur in a system-level name resolution. - grpc_core::OrphanablePtr ResolveName( + TaskHandle ResolveName( absl::string_view name, absl::string_view default_port, grpc_pollset_set* interested_parties, std::function>)> @@ -100,6 +100,9 @@ class TestDNSResolver : public grpc_core::DNSResolver { absl::string_view name, absl::string_view default_port) override { return g_default_dns_resolver->ResolveNameBlocking(name, default_port); } + + // Not cancellable + bool Cancel(TaskHandle /*handle*/) override { return false; } }; } // namespace diff --git a/test/core/end2end/fuzzers/api_fuzzer.cc b/test/core/end2end/fuzzers/api_fuzzer.cc index e79a908e54b..682f7e69ab9 100644 --- a/test/core/end2end/fuzzers/api_fuzzer.cc +++ b/test/core/end2end/fuzzers/api_fuzzer.cc @@ -114,25 +114,19 @@ namespace { class FuzzerDNSResolver : public grpc_core::DNSResolver { public: - class FuzzerDNSRequest : public grpc_core::DNSResolver::Request { + class FuzzerDNSRequest { public: FuzzerDNSRequest( absl::string_view name, std::function>)> on_done) - : name_(std::string(name)), on_done_(std::move(on_done)) {} - - void Start() override { - Ref().release(); // ref held by timer callback + : name_(std::string(name)), on_done_(std::move(on_done)) { grpc_timer_init( &timer_, grpc_core::Duration::Seconds(1) + grpc_core::ExecCtx::Get()->Now(), GRPC_CLOSURE_CREATE(FinishResolve, this, grpc_schedule_on_exec_ctx)); } - // cancellation not implemented - void Orphan() override { Unref(); } - private: static void FinishResolve(void* arg, grpc_error_handle error) { FuzzerDNSRequest* self = static_cast(arg); @@ -145,7 +139,7 @@ class FuzzerDNSResolver : public grpc_core::DNSResolver { } else { self->on_done_(absl::UnknownError("Resolution failed")); } - self->Unref(); + delete self; } const std::string name_; @@ -161,13 +155,13 @@ class FuzzerDNSResolver : public grpc_core::DNSResolver { return instance; } - grpc_core::OrphanablePtr ResolveName( + TaskHandle ResolveName( absl::string_view name, absl::string_view /* default_port */, grpc_pollset_set* /* interested_parties */, std::function>)> on_done) override { - return grpc_core::MakeOrphanable(name, - std::move(on_done)); + new FuzzerDNSRequest(name, std::move(on_done)); + return kNullHandle; } absl::StatusOr> ResolveNameBlocking( @@ -175,6 +169,9 @@ class FuzzerDNSResolver : public grpc_core::DNSResolver { absl::string_view /* default_port */) override { GPR_ASSERT(0); } + + // FuzzerDNSResolver does not support cancellation. + bool Cancel(TaskHandle /*handle*/) override { return false; } }; } // namespace diff --git a/test/core/end2end/goaway_server_test.cc b/test/core/end2end/goaway_server_test.cc index f8f8b85852f..3b315a17988 100644 --- a/test/core/end2end/goaway_server_test.cc +++ b/test/core/end2end/goaway_server_test.cc @@ -63,42 +63,7 @@ grpc_core::DNSResolver* g_default_dns_resolver; class TestDNSResolver : public grpc_core::DNSResolver { public: - class TestDNSRequest : public grpc_core::DNSResolver::Request { - public: - explicit TestDNSRequest( - std::function>)> - on_done) - : on_done_(std::move(on_done)) {} - - void Start() override { - gpr_mu_lock(&g_mu); - if (g_resolve_port < 0) { - gpr_mu_unlock(&g_mu); - new grpc_core::DNSCallbackExecCtxScheduler( - std::move(on_done_), absl::UnknownError("Forced Failure")); - } else { - std::vector addrs; - grpc_resolved_address addr; - grpc_sockaddr_in* sa = reinterpret_cast(&addr); - sa->sin_family = GRPC_AF_INET; - sa->sin_addr.s_addr = 0x100007f; - sa->sin_port = grpc_htons(static_cast(g_resolve_port)); - addr.len = static_cast(sizeof(*sa)); - addrs.push_back(addr); - gpr_mu_unlock(&g_mu); - new grpc_core::DNSCallbackExecCtxScheduler(std::move(on_done_), - std::move(addrs)); - } - } - - void Orphan() override { Unref(); } - - private: - std::function>)> - on_done_; - }; - - grpc_core::OrphanablePtr ResolveName( + TaskHandle ResolveName( absl::string_view name, absl::string_view default_port, grpc_pollset_set* interested_parties, std::function>)> @@ -107,13 +72,40 @@ class TestDNSResolver : public grpc_core::DNSResolver { return g_default_dns_resolver->ResolveName( name, default_port, interested_parties, std::move(on_done)); } - return grpc_core::MakeOrphanable(std::move(on_done)); + MakeDNSRequest(std::move(on_done)); + return kNullHandle; } absl::StatusOr> ResolveNameBlocking( absl::string_view name, absl::string_view default_port) override { return g_default_dns_resolver->ResolveNameBlocking(name, default_port); } + + bool Cancel(TaskHandle /*handle*/) override { return false; } + + private: + void MakeDNSRequest( + std::function>)> + on_done) { + gpr_mu_lock(&g_mu); + if (g_resolve_port < 0) { + gpr_mu_unlock(&g_mu); + new grpc_core::DNSCallbackExecCtxScheduler( + std::move(on_done), absl::UnknownError("Forced Failure")); + } else { + std::vector addrs; + grpc_resolved_address addr; + grpc_sockaddr_in* sa = reinterpret_cast(&addr); + sa->sin_family = GRPC_AF_INET; + sa->sin_addr.s_addr = 0x100007f; + sa->sin_port = grpc_htons(static_cast(g_resolve_port)); + addr.len = static_cast(sizeof(*sa)); + addrs.push_back(addr); + gpr_mu_unlock(&g_mu); + new grpc_core::DNSCallbackExecCtxScheduler(std::move(on_done), + std::move(addrs)); + } + } }; } // namespace diff --git a/test/core/iomgr/resolve_address_posix_test.cc b/test/core/iomgr/resolve_address_posix_test.cc index 74baa1402af..f2049250b4d 100644 --- a/test/core/iomgr/resolve_address_posix_test.cc +++ b/test/core/iomgr/resolve_address_posix_test.cc @@ -135,12 +135,11 @@ static void resolve_address_must_succeed(const char* target) { args_struct args; args_init(&args); poll_pollset_until_request_done(&args); - auto r = grpc_core::GetDNSResolver()->ResolveName( + grpc_core::GetDNSResolver()->ResolveName( target, "1" /* port number */, args.pollset_set, [&args](absl::StatusOr> result) { MustSucceed(&args, std::move(result)); }); - r->Start(); grpc_core::ExecCtx::Get()->Flush(); args_finish(&args); } diff --git a/test/core/iomgr/resolve_address_test.cc b/test/core/iomgr/resolve_address_test.cc index 11f0561f88e..1475070e70c 100644 --- a/test/core/iomgr/resolve_address_test.cc +++ b/test/core/iomgr/resolve_address_test.cc @@ -154,10 +154,10 @@ class ResolveAddressTest : public ::testing::Test { Finish(); } - grpc_pollset_set* pollset_set() const { return pollset_set_; } - - private: - static void DoNothing(void* /*arg*/, grpc_error_handle /*error*/) {} + void MustNotBeCalled( + absl::StatusOr> /*result*/) { + FAIL() << "This should never be called"; + } void Finish() { grpc_core::MutexLockForGprMu lock(mu_); @@ -165,6 +165,11 @@ class ResolveAddressTest : public ::testing::Test { GRPC_LOG_IF_ERROR("pollset_kick", grpc_pollset_kick(pollset_, nullptr)); } + grpc_pollset_set* pollset_set() const { return pollset_set_; } + + private: + static void DoNothing(void* /*arg*/, grpc_error_handle /*error*/) {} + gpr_mu* mu_; bool done_ = false; // guarded by mu grpc_pollset* pollset_; // guarded by mu @@ -178,20 +183,18 @@ class ResolveAddressTest : public ::testing::Test { TEST_F(ResolveAddressTest, Localhost) { grpc_core::ExecCtx exec_ctx; - auto r = grpc_core::GetDNSResolver()->ResolveName( + grpc_core::GetDNSResolver()->ResolveName( "localhost:1", "", pollset_set(), absl::bind_front(&ResolveAddressTest::MustSucceed, this)); - r->Start(); grpc_core::ExecCtx::Get()->Flush(); PollPollsetUntilRequestDone(); } TEST_F(ResolveAddressTest, DefaultPort) { grpc_core::ExecCtx exec_ctx; - auto r = grpc_core::GetDNSResolver()->ResolveName( + grpc_core::GetDNSResolver()->ResolveName( "localhost", "1", pollset_set(), absl::bind_front(&ResolveAddressTest::MustSucceed, this)); - r->Start(); grpc_core::ExecCtx::Get()->Flush(); PollPollsetUntilRequestDone(); } @@ -201,10 +204,9 @@ TEST_F(ResolveAddressTest, LocalhostResultHasIPv6First) { GTEST_SKIP() << "this test is only valid with the c-ares resolver"; } grpc_core::ExecCtx exec_ctx; - auto r = grpc_core::GetDNSResolver()->ResolveName( + grpc_core::GetDNSResolver()->ResolveName( "localhost:1", "", pollset_set(), absl::bind_front(&ResolveAddressTest::MustSucceedWithIPv6First, this)); - r->Start(); grpc_core::ExecCtx::Get()->Flush(); PollPollsetUntilRequestDone(); } @@ -249,50 +251,45 @@ TEST_F(ResolveAddressTest, LocalhostResultHasIPv4FirstWhenIPv6IsntAvalailable) { address_sorting_override_source_addr_factory_for_testing(mock); // run the test grpc_core::ExecCtx exec_ctx; - auto r = grpc_core::GetDNSResolver()->ResolveName( + grpc_core::GetDNSResolver()->ResolveName( "localhost:1", "", pollset_set(), absl::bind_front(&ResolveAddressTest::MustSucceedWithIPv4First, this)); - r->Start(); grpc_core::ExecCtx::Get()->Flush(); PollPollsetUntilRequestDone(); } TEST_F(ResolveAddressTest, NonNumericDefaultPort) { grpc_core::ExecCtx exec_ctx; - auto r = grpc_core::GetDNSResolver()->ResolveName( + grpc_core::GetDNSResolver()->ResolveName( "localhost", "http", pollset_set(), absl::bind_front(&ResolveAddressTest::MustSucceed, this)); - r->Start(); grpc_core::ExecCtx::Get()->Flush(); PollPollsetUntilRequestDone(); } TEST_F(ResolveAddressTest, MissingDefaultPort) { grpc_core::ExecCtx exec_ctx; - auto r = grpc_core::GetDNSResolver()->ResolveName( + grpc_core::GetDNSResolver()->ResolveName( "localhost", "", pollset_set(), absl::bind_front(&ResolveAddressTest::MustFail, this)); - r->Start(); grpc_core::ExecCtx::Get()->Flush(); PollPollsetUntilRequestDone(); } TEST_F(ResolveAddressTest, IPv6WithPort) { grpc_core::ExecCtx exec_ctx; - auto r = grpc_core::GetDNSResolver()->ResolveName( + grpc_core::GetDNSResolver()->ResolveName( "[2001:db8::1]:1", "", pollset_set(), absl::bind_front(&ResolveAddressTest::MustSucceed, this)); - r->Start(); grpc_core::ExecCtx::Get()->Flush(); PollPollsetUntilRequestDone(); } void TestIPv6WithoutPort(ResolveAddressTest* test, const char* target) { grpc_core::ExecCtx exec_ctx; - auto r = grpc_core::GetDNSResolver()->ResolveName( + grpc_core::GetDNSResolver()->ResolveName( target, "80", test->pollset_set(), absl::bind_front(&ResolveAddressTest::MustSucceed, test)); - r->Start(); grpc_core::ExecCtx::Get()->Flush(); test->PollPollsetUntilRequestDone(); } @@ -311,10 +308,9 @@ TEST_F(ResolveAddressTest, IPv6WithoutPortV4MappedV6) { void TestInvalidIPAddress(ResolveAddressTest* test, const char* target) { grpc_core::ExecCtx exec_ctx; - auto r = grpc_core::GetDNSResolver()->ResolveName( + grpc_core::GetDNSResolver()->ResolveName( target, "", test->pollset_set(), absl::bind_front(&ResolveAddressTest::MustFail, test)); - r->Start(); grpc_core::ExecCtx::Get()->Flush(); test->PollPollsetUntilRequestDone(); } @@ -329,10 +325,9 @@ TEST_F(ResolveAddressTest, InvalidIPv6Addresses) { void TestUnparseableHostPort(ResolveAddressTest* test, const char* target) { grpc_core::ExecCtx exec_ctx; - auto r = grpc_core::GetDNSResolver()->ResolveName( + grpc_core::GetDNSResolver()->ResolveName( target, "1", test->pollset_set(), absl::bind_front(&ResolveAddressTest::MustFail, test)); - r->Start(); grpc_core::ExecCtx::Get()->Flush(); test->PollPollsetUntilRequestDone(); } @@ -365,11 +360,12 @@ TEST_F(ResolveAddressTest, UnparseableHostPortsBadLocalhostWithPort) { // test doesn't care what the result is, just that we don't crash etc. TEST_F(ResolveAddressTest, ImmediateCancel) { grpc_core::ExecCtx exec_ctx; - auto r = grpc_core::GetDNSResolver()->ResolveName( + auto request_handle = grpc_core::GetDNSResolver()->ResolveName( "localhost:1", "1", pollset_set(), absl::bind_front(&ResolveAddressTest::DontCare, this)); - r->Start(); - r.reset(); // cancel the resolution + if (grpc_core::GetDNSResolver()->Cancel(request_handle)) { + Finish(); + } grpc_core::ExecCtx::Get()->Flush(); PollPollsetUntilRequestDone(); } @@ -410,14 +406,14 @@ TEST_F(ResolveAddressTest, CancelWithNonResponsiveDNSServer) { grpc_ares_test_only_inject_config = InjectNonResponsiveDNSServer; // Run the test grpc_core::ExecCtx exec_ctx; - auto r = grpc_core::GetDNSResolver()->ResolveName( + auto request_handle = grpc_core::GetDNSResolver()->ResolveName( "foo.bar.com:1", "1", pollset_set(), - absl::bind_front(&ResolveAddressTest::MustFailExpectCancelledErrorMessage, - this)); - r->Start(); + absl::bind_front(&ResolveAddressTest::MustNotBeCalled, this)); grpc_core::ExecCtx::Get()->Flush(); // initiate DNS requests - r.reset(); // cancel the resolution - grpc_core::ExecCtx::Get()->Flush(); // let cancellation work finish + ASSERT_TRUE(grpc_core::GetDNSResolver()->Cancel(request_handle)); + Finish(); + // let cancellation work finish to ensure the callback is not called + grpc_core::ExecCtx::Get()->Flush(); PollPollsetUntilRequestDone(); } diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index d890bb30c6d..e320314db8b 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -1931,6 +1931,7 @@ src/core/lib/event_engine/channel_args_endpoint_config.h \ src/core/lib/event_engine/default_event_engine_factory.cc \ src/core/lib/event_engine/event_engine.cc \ src/core/lib/event_engine/event_engine_factory.h \ +src/core/lib/event_engine/handle_containers.h \ src/core/lib/event_engine/memory_allocator.cc \ src/core/lib/event_engine/resolved_address.cc \ src/core/lib/event_engine/sockaddr.cc \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index b17484e7919..49c90a6002e 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -1723,6 +1723,7 @@ src/core/lib/event_engine/channel_args_endpoint_config.h \ src/core/lib/event_engine/default_event_engine_factory.cc \ src/core/lib/event_engine/event_engine.cc \ src/core/lib/event_engine/event_engine_factory.h \ +src/core/lib/event_engine/handle_containers.h \ src/core/lib/event_engine/memory_allocator.cc \ src/core/lib/event_engine/resolved_address.cc \ src/core/lib/event_engine/sockaddr.cc \