From 73605f4eac355bcbbbf43114d3801948d55fbcdd Mon Sep 17 00:00:00 2001 From: Yijie Ma Date: Wed, 19 Jul 2023 14:23:26 -0700 Subject: [PATCH] [EventEngine] Change `GetDNSResolver` to return `absl::StatusOr>` (#33744) Based on the discussion at: https://github.com/grpc/grpc/pull/32701/files/595a75cc5d56f8d7341a9533a24428ad37810269..e3b402a8fa27fc8787a73775068721bf9f322640#r1244325752 --- include/grpc/event_engine/event_engine.h | 7 +++++-- .../event_engine_client_channel_resolver.cc | 12 ++++++++++-- .../client_channel/resolver/polling_resolver.cc | 9 +++++++-- src/core/lib/event_engine/cf_engine/cf_engine.cc | 3 ++- src/core/lib/event_engine/cf_engine/cf_engine.h | 2 +- .../lib/event_engine/posix_engine/posix_engine.cc | 3 ++- .../lib/event_engine/posix_engine/posix_engine.h | 2 +- .../thready_event_engine/thready_event_engine.cc | 5 +++-- .../thready_event_engine/thready_event_engine.h | 2 +- src/core/lib/event_engine/windows/windows_engine.cc | 3 ++- src/core/lib/event_engine/windows/windows_engine.h | 2 +- .../core/event_engine/default_engine_methods_test.cc | 2 +- .../fuzzing_event_engine/fuzzing_event_engine.cc | 4 ++-- .../fuzzing_event_engine/fuzzing_event_engine.h | 2 +- test/core/event_engine/mock_event_engine.h | 2 +- .../test_suite/posix/oracle_event_engine_posix.h | 2 +- test/core/event_engine/util/aborting_event_engine.h | 2 +- .../resolver_fuzzer.cc | 2 +- 18 files changed, 43 insertions(+), 23 deletions(-) diff --git a/include/grpc/event_engine/event_engine.h b/include/grpc/event_engine/event_engine.h index 4e0df0ef56a..00b57629e73 100644 --- a/include/grpc/event_engine/event_engine.h +++ b/include/grpc/event_engine/event_engine.h @@ -397,8 +397,11 @@ class EventEngine : public std::enable_shared_from_this { virtual bool IsWorkerThread() = 0; /// Creates and returns an instance of a DNSResolver, optionally configured by - /// the \a options struct. - virtual std::unique_ptr GetDNSResolver( + /// the \a options struct. This method may return a non-OK status if an error + /// occurred when creating the DNSResolver. If the caller requests a custom + /// DNS server, and the EventEngine implementation does not support it, this + /// must return an error. + virtual absl::StatusOr> GetDNSResolver( const DNSResolver::ResolverOptions& options) = 0; /// Asynchronously executes a task as soon as possible. diff --git a/src/core/ext/filters/client_channel/resolver/dns/event_engine/event_engine_client_channel_resolver.cc b/src/core/ext/filters/client_channel/resolver/dns/event_engine/event_engine_client_channel_resolver.cc index a7061cfe923..a8596e7c7ca 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/event_engine/event_engine_client_channel_resolver.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/event_engine/event_engine_client_channel_resolver.cc @@ -200,9 +200,17 @@ EventEngineClientChannelDNSResolver::EventEngineClientChannelDNSResolver( event_engine_(channel_args().GetObjectRef()) {} OrphanablePtr EventEngineClientChannelDNSResolver::StartRequest() { + auto dns_resolver = + event_engine_->GetDNSResolver({/*dns_server=*/authority()}); + if (!dns_resolver.ok()) { + Result result; + result.addresses = dns_resolver.status(); + result.service_config = dns_resolver.status(); + OnRequestComplete(std::move(result)); + return nullptr; + } return MakeOrphanable( - Ref(DEBUG_LOCATION, "dns-resolving"), - event_engine_->GetDNSResolver({/*dns_server=*/authority()})); + Ref(DEBUG_LOCATION, "dns-resolving"), std::move(*dns_resolver)); } // ---------------------------------------------------------------------------- diff --git a/src/core/ext/filters/client_channel/resolver/polling_resolver.cc b/src/core/ext/filters/client_channel/resolver/polling_resolver.cc index 46f63e68e7a..ff94e9834de 100644 --- a/src/core/ext/filters/client_channel/resolver/polling_resolver.cc +++ b/src/core/ext/filters/client_channel/resolver/polling_resolver.cc @@ -260,8 +260,13 @@ void PollingResolver::StartResolvingLocked() { request_ = StartRequest(); last_resolution_timestamp_ = Timestamp::Now(); if (GPR_UNLIKELY(tracer_ != nullptr && tracer_->enabled())) { - gpr_log(GPR_INFO, "[polling resolver %p] starting resolution, request_=%p", - this, request_.get()); + if (request_ != nullptr) { + gpr_log(GPR_INFO, + "[polling resolver %p] starting resolution, request_=%p", this, + request_.get()); + } else { + gpr_log(GPR_INFO, "[polling resolver %p] StartRequest failed", this); + } } } diff --git a/src/core/lib/event_engine/cf_engine/cf_engine.cc b/src/core/lib/event_engine/cf_engine/cf_engine.cc index ecd5e4fe218..f835e64a21e 100644 --- a/src/core/lib/event_engine/cf_engine/cf_engine.cc +++ b/src/core/lib/event_engine/cf_engine/cf_engine.cc @@ -155,7 +155,8 @@ bool CFEventEngine::CancelConnectInternal(ConnectionHandle handle, bool CFEventEngine::IsWorkerThread() { grpc_core::Crash("unimplemented"); } -std::unique_ptr CFEventEngine::GetDNSResolver( +absl::StatusOr> +CFEventEngine::GetDNSResolver( const DNSResolver::ResolverOptions& /* options */) { grpc_core::Crash("unimplemented"); } diff --git a/src/core/lib/event_engine/cf_engine/cf_engine.h b/src/core/lib/event_engine/cf_engine/cf_engine.h index 3e55ce85616..e67f9c75331 100644 --- a/src/core/lib/event_engine/cf_engine/cf_engine.h +++ b/src/core/lib/event_engine/cf_engine/cf_engine.h @@ -51,7 +51,7 @@ class CFEventEngine : public EventEngine, Duration timeout) override; bool CancelConnect(ConnectionHandle handle) override; bool IsWorkerThread() override; - std::unique_ptr GetDNSResolver( + absl::StatusOr> GetDNSResolver( const DNSResolver::ResolverOptions& options) override; void Run(Closure* closure) override; void Run(absl::AnyInvocable closure) override; diff --git a/src/core/lib/event_engine/posix_engine/posix_engine.cc b/src/core/lib/event_engine/posix_engine/posix_engine.cc index 0bb74bbab58..a1d217cd3bd 100644 --- a/src/core/lib/event_engine/posix_engine/posix_engine.cc +++ b/src/core/lib/event_engine/posix_engine/posix_engine.cc @@ -487,7 +487,8 @@ EventEngine::TaskHandle PosixEventEngine::RunAfterInternal( return handle; } -std::unique_ptr PosixEventEngine::GetDNSResolver( +absl::StatusOr> +PosixEventEngine::GetDNSResolver( EventEngine::DNSResolver::ResolverOptions const& /*options*/) { grpc_core::Crash("unimplemented"); } diff --git a/src/core/lib/event_engine/posix_engine/posix_engine.h b/src/core/lib/event_engine/posix_engine/posix_engine.h index 815a663d3d0..7fc40a1d33a 100644 --- a/src/core/lib/event_engine/posix_engine/posix_engine.h +++ b/src/core/lib/event_engine/posix_engine/posix_engine.h @@ -188,7 +188,7 @@ class PosixEventEngine final : public PosixEventEngineWithFdSupport, bool CancelConnect(ConnectionHandle handle) override; bool IsWorkerThread() override; - std::unique_ptr GetDNSResolver( + absl::StatusOr> GetDNSResolver( const DNSResolver::ResolverOptions& options) override; void Run(Closure* closure) override; void Run(absl::AnyInvocable closure) override; diff --git a/src/core/lib/event_engine/thready_event_engine/thready_event_engine.cc b/src/core/lib/event_engine/thready_event_engine/thready_event_engine.cc index 81794e02cd0..4beb7b68bcf 100644 --- a/src/core/lib/event_engine/thready_event_engine/thready_event_engine.cc +++ b/src/core/lib/event_engine/thready_event_engine/thready_event_engine.cc @@ -82,9 +82,10 @@ bool ThreadyEventEngine::IsWorkerThread() { grpc_core::Crash("we should remove this"); } -std::unique_ptr ThreadyEventEngine::GetDNSResolver( +absl::StatusOr> +ThreadyEventEngine::GetDNSResolver( const DNSResolver::ResolverOptions& options) { - return std::make_unique(impl_->GetDNSResolver(options)); + return std::make_unique(*impl_->GetDNSResolver(options)); } void ThreadyEventEngine::Run(Closure* closure) { diff --git a/src/core/lib/event_engine/thready_event_engine/thready_event_engine.h b/src/core/lib/event_engine/thready_event_engine/thready_event_engine.h index bcb972a8d65..23b79dc62cf 100644 --- a/src/core/lib/event_engine/thready_event_engine/thready_event_engine.h +++ b/src/core/lib/event_engine/thready_event_engine/thready_event_engine.h @@ -63,7 +63,7 @@ class ThreadyEventEngine final : public EventEngine { bool IsWorkerThread() override; - std::unique_ptr GetDNSResolver( + absl::StatusOr> GetDNSResolver( const DNSResolver::ResolverOptions& options) override; void Run(Closure* closure) override; diff --git a/src/core/lib/event_engine/windows/windows_engine.cc b/src/core/lib/event_engine/windows/windows_engine.cc index 435b9c32197..552488639f1 100644 --- a/src/core/lib/event_engine/windows/windows_engine.cc +++ b/src/core/lib/event_engine/windows/windows_engine.cc @@ -194,7 +194,8 @@ EventEngine::TaskHandle WindowsEventEngine::RunAfterInternal( return handle; } -std::unique_ptr WindowsEventEngine::GetDNSResolver( +absl::StatusOr> +WindowsEventEngine::GetDNSResolver( EventEngine::DNSResolver::ResolverOptions const& /*options*/) { grpc_core::Crash("unimplemented"); } diff --git a/src/core/lib/event_engine/windows/windows_engine.h b/src/core/lib/event_engine/windows/windows_engine.h index 67de3721189..5fed3ae95f0 100644 --- a/src/core/lib/event_engine/windows/windows_engine.h +++ b/src/core/lib/event_engine/windows/windows_engine.h @@ -75,7 +75,7 @@ class WindowsEventEngine : public EventEngine, bool CancelConnect(ConnectionHandle handle) override; bool IsWorkerThread() override; - std::unique_ptr GetDNSResolver( + absl::StatusOr> GetDNSResolver( const DNSResolver::ResolverOptions& options) override; void Run(Closure* closure) override; void Run(absl::AnyInvocable closure) override; diff --git a/test/core/event_engine/default_engine_methods_test.cc b/test/core/event_engine/default_engine_methods_test.cc index 4f98c5736ff..6ec63318409 100644 --- a/test/core/event_engine/default_engine_methods_test.cc +++ b/test/core/event_engine/default_engine_methods_test.cc @@ -66,7 +66,7 @@ class DefaultEngineTest : public testing::Test { return false; }; bool IsWorkerThread() override { return false; }; - std::unique_ptr GetDNSResolver( + absl::StatusOr> GetDNSResolver( const DNSResolver::ResolverOptions& /* options */) override { return nullptr; }; diff --git a/test/core/event_engine/fuzzing_event_engine/fuzzing_event_engine.cc b/test/core/event_engine/fuzzing_event_engine/fuzzing_event_engine.cc index 9779e0f0440..36dcfa5f029 100644 --- a/test/core/event_engine/fuzzing_event_engine/fuzzing_event_engine.cc +++ b/test/core/event_engine/fuzzing_event_engine/fuzzing_event_engine.cc @@ -461,8 +461,8 @@ bool FuzzingEventEngine::CancelConnect(ConnectionHandle connection_handle) { bool FuzzingEventEngine::IsWorkerThread() { abort(); } -std::unique_ptr FuzzingEventEngine::GetDNSResolver( - const DNSResolver::ResolverOptions&) { +absl::StatusOr> +FuzzingEventEngine::GetDNSResolver(const DNSResolver::ResolverOptions&) { abort(); } diff --git a/test/core/event_engine/fuzzing_event_engine/fuzzing_event_engine.h b/test/core/event_engine/fuzzing_event_engine/fuzzing_event_engine.h index 870505bc873..936d926ddfb 100644 --- a/test/core/event_engine/fuzzing_event_engine/fuzzing_event_engine.h +++ b/test/core/event_engine/fuzzing_event_engine/fuzzing_event_engine.h @@ -85,7 +85,7 @@ class FuzzingEventEngine : public EventEngine { bool IsWorkerThread() override; - std::unique_ptr GetDNSResolver( + absl::StatusOr> GetDNSResolver( const DNSResolver::ResolverOptions& options) override; void Run(Closure* closure) ABSL_LOCKS_EXCLUDED(mu_) override; diff --git a/test/core/event_engine/mock_event_engine.h b/test/core/event_engine/mock_event_engine.h index 60a1129c76e..e25f526cc7f 100644 --- a/test/core/event_engine/mock_event_engine.h +++ b/test/core/event_engine/mock_event_engine.h @@ -44,7 +44,7 @@ class MockEventEngine : public EventEngine { Duration timeout)); MOCK_METHOD(bool, CancelConnect, (ConnectionHandle handle)); MOCK_METHOD(bool, IsWorkerThread, ()); - MOCK_METHOD(std::unique_ptr, GetDNSResolver, + MOCK_METHOD(absl::StatusOr>, GetDNSResolver, (const DNSResolver::ResolverOptions& options)); MOCK_METHOD(void, Run, (Closure * closure)); MOCK_METHOD(void, Run, (absl::AnyInvocable closure)); diff --git a/test/core/event_engine/test_suite/posix/oracle_event_engine_posix.h b/test/core/event_engine/test_suite/posix/oracle_event_engine_posix.h index 817480bb4e6..dc6de649e71 100644 --- a/test/core/event_engine/test_suite/posix/oracle_event_engine_posix.h +++ b/test/core/event_engine/test_suite/posix/oracle_event_engine_posix.h @@ -171,7 +171,7 @@ class PosixOracleEventEngine final : public EventEngine { grpc_core::Crash("unimplemented"); } bool IsWorkerThread() override { return false; }; - std::unique_ptr GetDNSResolver( + absl::StatusOr> GetDNSResolver( const DNSResolver::ResolverOptions& /*options*/) override { grpc_core::Crash("unimplemented"); } diff --git a/test/core/event_engine/util/aborting_event_engine.h b/test/core/event_engine/util/aborting_event_engine.h index 016bd899504..3144e015a9e 100644 --- a/test/core/event_engine/util/aborting_event_engine.h +++ b/test/core/event_engine/util/aborting_event_engine.h @@ -49,7 +49,7 @@ class AbortingEventEngine : public EventEngine { abort(); }; bool IsWorkerThread() override { abort(); } - std::unique_ptr GetDNSResolver( + absl::StatusOr> GetDNSResolver( const DNSResolver::ResolverOptions& /* options */) override { abort(); } diff --git a/test/core/ext/filters/event_engine_client_channel_resolver/resolver_fuzzer.cc b/test/core/ext/filters/event_engine_client_channel_resolver/resolver_fuzzer.cc index 3f8412dc416..5c862fa074e 100644 --- a/test/core/ext/filters/event_engine_client_channel_resolver/resolver_fuzzer.cc +++ b/test/core/ext/filters/event_engine_client_channel_resolver/resolver_fuzzer.cc @@ -132,7 +132,7 @@ class FuzzingResolverEventEngine } } - std::unique_ptr GetDNSResolver( + absl::StatusOr> GetDNSResolver( const DNSResolver::ResolverOptions& /* options */) override { return std::make_unique(this); }