diff --git a/BUILD b/BUILD index 7b2cca3c1c3c6..084e7cafeb3f0 100644 --- a/BUILD +++ b/BUILD @@ -2116,6 +2116,7 @@ grpc_cc_library( "//src/core:time", "//src/core:transport_fwd", "//src/core:try_seq", + "//src/core:unique_type_name", "//src/core:useful", ], ) @@ -2284,6 +2285,7 @@ grpc_cc_library( external_deps = [ "absl/base:core_headers", "absl/container:inlined_vector", + "absl/functional:any_invocable", "absl/log:check", "absl/log:log", "absl/status", @@ -2308,6 +2310,7 @@ grpc_cc_library( "grpc_trace", "handshaker", "iomgr", + "orphanable", "promise", "ref_counted_ptr", "resource_quota_api", @@ -3191,9 +3194,11 @@ grpc_cc_library( external_deps = [ "absl/base:core_headers", "absl/container:inlined_vector", + "absl/functional:any_invocable", "absl/log:check", "absl/log:log", "absl/status", + "absl/status:statusor", "absl/strings:str_format", ], language = "c++", @@ -3210,6 +3215,7 @@ grpc_cc_library( "grpc_public_hdrs", "grpc_trace", "iomgr", + "orphanable", "ref_counted_ptr", "//src/core:channel_args", "//src/core:closure", diff --git a/examples/cpp/route_guide/route_guide_callback_client.cc b/examples/cpp/route_guide/route_guide_callback_client.cc index 102061269a8f7..d44e9fae2d3e3 100644 --- a/examples/cpp/route_guide/route_guide_callback_client.cc +++ b/examples/cpp/route_guide/route_guide_callback_client.cc @@ -243,7 +243,11 @@ class RouteGuideClient { StartRead(&server_note_); StartCall(); } - void OnWriteDone(bool /*ok*/) override { NextWrite(); } + void OnWriteDone(bool ok) override { + if (ok) { + NextWrite(); + } + } void OnReadDone(bool ok) override { if (ok) { std::cout << "Got message " << server_note_.message() << " at " diff --git a/src/core/BUILD b/src/core/BUILD index e8fbbeeeebf8b..d21115411d749 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -1299,6 +1299,7 @@ grpc_cc_library( ], external_deps = [ "absl/base:core_headers", + "absl/functional:any_invocable", "absl/log:check", "absl/status", "absl/status:statusor", @@ -1341,6 +1342,7 @@ grpc_cc_library( "handshaker/endpoint_info/endpoint_info_handshaker.h", ], external_deps = [ + "absl/functional:any_invocable", "absl/status", ], language = "c++", @@ -3073,6 +3075,7 @@ grpc_cc_library( "channel_fwd", "channel_stack_type", "interception_chain", + "unique_type_name", "//:channel_stack_builder", "//:debug_location", "//:gpr", diff --git a/src/core/client_channel/client_channel_filter.cc b/src/core/client_channel/client_channel_filter.cc index 7db7613f136bd..0ad970b549229 100644 --- a/src/core/client_channel/client_channel_filter.cc +++ b/src/core/client_channel/client_channel_filter.cc @@ -316,7 +316,7 @@ const grpc_channel_filter ClientChannelFilter::kFilter = { grpc_channel_stack_no_post_init, ClientChannelFilter::Destroy, ClientChannelFilter::GetChannelInfo, - "client-channel", + GRPC_UNIQUE_TYPE_NAME_HERE("client-channel"), }; // @@ -443,7 +443,7 @@ const grpc_channel_filter DynamicTerminationFilter::kFilterVtable = { grpc_channel_stack_no_post_init, DynamicTerminationFilter::Destroy, DynamicTerminationFilter::GetChannelInfo, - "dynamic_filter_termination", + GRPC_UNIQUE_TYPE_NAME_HERE("dynamic_filter_termination"), }; } // namespace diff --git a/src/core/client_channel/dynamic_filters.cc b/src/core/client_channel/dynamic_filters.cc index f901cf8b9bc81..1e33da2105c3c 100644 --- a/src/core/client_channel/dynamic_filters.cc +++ b/src/core/client_channel/dynamic_filters.cc @@ -78,7 +78,9 @@ void DynamicFilters::Call::StartTransportStreamOpBatch( grpc_transport_stream_op_batch* batch) { grpc_call_stack* call_stack = CALL_TO_CALL_STACK(this); grpc_call_element* top_elem = grpc_call_stack_element(call_stack, 0); - GRPC_CALL_LOG_OP(GPR_INFO, top_elem, batch); + GRPC_TRACE_LOG(channel, INFO) + << "OP[" << top_elem->filter->name << ":" << top_elem + << "]: " << grpc_transport_stream_op_batch_string(batch, false); top_elem->filter->start_transport_stream_op_batch(top_elem, batch); } diff --git a/src/core/client_channel/retry_filter.cc b/src/core/client_channel/retry_filter.cc index f09d06d0e28b8..754a543d7eb49 100644 --- a/src/core/client_channel/retry_filter.cc +++ b/src/core/client_channel/retry_filter.cc @@ -147,7 +147,7 @@ const grpc_channel_filter RetryFilter::kVtable = { grpc_channel_stack_no_post_init, RetryFilter::Destroy, RetryFilter::GetChannelInfo, - "retry_filter", + GRPC_UNIQUE_TYPE_NAME_HERE("retry_filter"), }; } // namespace grpc_core diff --git a/src/core/client_channel/subchannel.cc b/src/core/client_channel/subchannel.cc index b71df3cd9d6b6..9a76c22ae3020 100644 --- a/src/core/client_channel/subchannel.cc +++ b/src/core/client_channel/subchannel.cc @@ -270,7 +270,9 @@ void SubchannelCall::StartTransportStreamOpBatch( MaybeInterceptRecvTrailingMetadata(batch); grpc_call_stack* call_stack = SUBCHANNEL_CALL_TO_CALL_STACK(this); grpc_call_element* top_elem = grpc_call_stack_element(call_stack, 0); - GRPC_CALL_LOG_OP(GPR_INFO, top_elem, batch); + GRPC_TRACE_LOG(channel, INFO) + << "OP[" << top_elem->filter->name << ":" << top_elem + << "]: " << grpc_transport_stream_op_batch_string(batch, false); top_elem->filter->start_transport_stream_op_batch(top_elem, batch); } diff --git a/src/core/ext/filters/backend_metrics/backend_metric_filter.cc b/src/core/ext/filters/backend_metrics/backend_metric_filter.cc index 082efb9716e84..ebe189f9bea8c 100644 --- a/src/core/ext/filters/backend_metrics/backend_metric_filter.cc +++ b/src/core/ext/filters/backend_metrics/backend_metric_filter.cc @@ -117,8 +117,7 @@ absl::optional MaybeSerializeBackendMetrics( } // namespace const grpc_channel_filter BackendMetricFilter::kFilter = - MakePromiseBasedFilter( - "backend_metric"); + MakePromiseBasedFilter(); absl::StatusOr> BackendMetricFilter::Create(const ChannelArgs&, ChannelFilter::Args) { diff --git a/src/core/ext/filters/backend_metrics/backend_metric_filter.h b/src/core/ext/filters/backend_metrics/backend_metric_filter.h index 114fc3cc7bca1..02ee8611dce9e 100644 --- a/src/core/ext/filters/backend_metrics/backend_metric_filter.h +++ b/src/core/ext/filters/backend_metrics/backend_metric_filter.h @@ -35,6 +35,8 @@ class BackendMetricFilter : public ImplementChannelFilter { public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "backend_metric"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args); diff --git a/src/core/ext/filters/channel_idle/legacy_channel_idle_filter.cc b/src/core/ext/filters/channel_idle/legacy_channel_idle_filter.cc index c2e82de449074..54e0c2a395fc6 100644 --- a/src/core/ext/filters/channel_idle/legacy_channel_idle_filter.cc +++ b/src/core/ext/filters/channel_idle/legacy_channel_idle_filter.cc @@ -289,11 +289,9 @@ void LegacyChannelIdleFilter::CloseChannel() { } const grpc_channel_filter LegacyClientIdleFilter::kFilter = - MakePromiseBasedFilter( - "client_idle"); + MakePromiseBasedFilter(); const grpc_channel_filter LegacyMaxAgeFilter::kFilter = - MakePromiseBasedFilter( - "max_age"); + MakePromiseBasedFilter(); void RegisterLegacyChannelIdleFilters(CoreConfiguration::Builder* builder) { builder->channel_init() diff --git a/src/core/ext/filters/channel_idle/legacy_channel_idle_filter.h b/src/core/ext/filters/channel_idle/legacy_channel_idle_filter.h index 001282276f1df..9ee7981b2f59e 100644 --- a/src/core/ext/filters/channel_idle/legacy_channel_idle_filter.h +++ b/src/core/ext/filters/channel_idle/legacy_channel_idle_filter.h @@ -96,6 +96,8 @@ class LegacyClientIdleFilter final : public LegacyChannelIdleFilter { public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "client_idle"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args filter_args); @@ -107,6 +109,8 @@ class LegacyMaxAgeFilter final : public LegacyChannelIdleFilter { static const grpc_channel_filter kFilter; struct Config; + static absl::string_view TypeName() { return "max_age"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args filter_args); diff --git a/src/core/ext/filters/fault_injection/fault_injection_filter.cc b/src/core/ext/filters/fault_injection/fault_injection_filter.cc index 6190ad400849e..033f8ee99c381 100644 --- a/src/core/ext/filters/fault_injection/fault_injection_filter.cc +++ b/src/core/ext/filters/fault_injection/fault_injection_filter.cc @@ -270,8 +270,7 @@ std::string FaultInjectionFilter::InjectionDecision::ToString() const { } const grpc_channel_filter FaultInjectionFilter::kFilter = - MakePromiseBasedFilter( - "fault_injection_filter"); + MakePromiseBasedFilter(); void FaultInjectionFilterRegister(CoreConfiguration::Builder* builder) { FaultInjectionServiceConfigParser::Register(builder); diff --git a/src/core/ext/filters/fault_injection/fault_injection_filter.h b/src/core/ext/filters/fault_injection/fault_injection_filter.h index 515df16a85373..7bcd22270602e 100644 --- a/src/core/ext/filters/fault_injection/fault_injection_filter.h +++ b/src/core/ext/filters/fault_injection/fault_injection_filter.h @@ -45,6 +45,8 @@ class FaultInjectionFilter public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "fault_injection_filter"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args filter_args); diff --git a/src/core/ext/filters/http/client/http_client_filter.cc b/src/core/ext/filters/http/client/http_client_filter.cc index 6af2b959be3e3..7fd04a371e4a1 100644 --- a/src/core/ext/filters/http/client/http_client_filter.cc +++ b/src/core/ext/filters/http/client/http_client_filter.cc @@ -59,7 +59,7 @@ const NoInterceptor HttpClientFilter::Call::OnFinalize; const grpc_channel_filter HttpClientFilter::kFilter = MakePromiseBasedFilter("http-client"); + kFilterExaminesServerInitialMetadata>(); namespace { absl::Status CheckServerMetadata(ServerMetadata* b) { diff --git a/src/core/ext/filters/http/client/http_client_filter.h b/src/core/ext/filters/http/client/http_client_filter.h index f985337f2cac5..0b319cec303d2 100644 --- a/src/core/ext/filters/http/client/http_client_filter.h +++ b/src/core/ext/filters/http/client/http_client_filter.h @@ -35,6 +35,8 @@ class HttpClientFilter : public ImplementChannelFilter { public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "http-client"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args filter_args); diff --git a/src/core/ext/filters/http/client_authority_filter.cc b/src/core/ext/filters/http/client_authority_filter.cc index b6970d9ecb5fd..6dd4423415190 100644 --- a/src/core/ext/filters/http/client_authority_filter.cc +++ b/src/core/ext/filters/http/client_authority_filter.cc @@ -66,8 +66,7 @@ void ClientAuthorityFilter::Call::OnClientInitialMetadata( } const grpc_channel_filter ClientAuthorityFilter::kFilter = - MakePromiseBasedFilter( - "authority"); + MakePromiseBasedFilter(); namespace { bool NeedsClientAuthorityFilter(const ChannelArgs& args) { diff --git a/src/core/ext/filters/http/client_authority_filter.h b/src/core/ext/filters/http/client_authority_filter.h index da154fbac5de7..c3b817527c157 100644 --- a/src/core/ext/filters/http/client_authority_filter.h +++ b/src/core/ext/filters/http/client_authority_filter.h @@ -39,6 +39,8 @@ class ClientAuthorityFilter final public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "authority"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args); diff --git a/src/core/ext/filters/http/message_compress/compression_filter.cc b/src/core/ext/filters/http/message_compress/compression_filter.cc index e3d0a61cd012f..00839eebe5b78 100644 --- a/src/core/ext/filters/http/message_compress/compression_filter.cc +++ b/src/core/ext/filters/http/message_compress/compression_filter.cc @@ -66,12 +66,12 @@ const grpc_channel_filter ClientCompressionFilter::kFilter = MakePromiseBasedFilter("compression"); + kFilterExaminesOutboundMessages>(); const grpc_channel_filter ServerCompressionFilter::kFilter = MakePromiseBasedFilter("compression"); + kFilterExaminesOutboundMessages>(); absl::StatusOr> ClientCompressionFilter::Create(const ChannelArgs& args, ChannelFilter::Args) { diff --git a/src/core/ext/filters/http/message_compress/compression_filter.h b/src/core/ext/filters/http/message_compress/compression_filter.h index 5d82846d01d24..ae3a9ee317e8d 100644 --- a/src/core/ext/filters/http/message_compress/compression_filter.h +++ b/src/core/ext/filters/http/message_compress/compression_filter.h @@ -25,6 +25,7 @@ #include #include "absl/status/statusor.h" +#include "absl/strings/string_view.h" #include "absl/types/optional.h" #include @@ -110,6 +111,8 @@ class ClientCompressionFilter final public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "compression"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args filter_args); @@ -147,6 +150,8 @@ class ServerCompressionFilter final public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "compression"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args filter_args); diff --git a/src/core/ext/filters/http/server/http_server_filter.cc b/src/core/ext/filters/http/server/http_server_filter.cc index 8d4b71d5108a0..0e86363890151 100644 --- a/src/core/ext/filters/http/server/http_server_filter.cc +++ b/src/core/ext/filters/http/server/http_server_filter.cc @@ -55,7 +55,7 @@ const NoInterceptor HttpServerFilter::Call::OnFinalize; const grpc_channel_filter HttpServerFilter::kFilter = MakePromiseBasedFilter("http-server"); + kFilterExaminesServerInitialMetadata>(); namespace { void FilterOutgoingMetadata(ServerMetadata* md) { diff --git a/src/core/ext/filters/http/server/http_server_filter.h b/src/core/ext/filters/http/server/http_server_filter.h index a1f330e58bbbd..8f933865d7da0 100644 --- a/src/core/ext/filters/http/server/http_server_filter.h +++ b/src/core/ext/filters/http/server/http_server_filter.h @@ -36,6 +36,8 @@ class HttpServerFilter : public ImplementChannelFilter { public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "http-server"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args filter_args); diff --git a/src/core/ext/filters/load_reporting/server_load_reporting_filter.cc b/src/core/ext/filters/load_reporting/server_load_reporting_filter.cc index 100584f5ba182..6b4f77bdae5da 100644 --- a/src/core/ext/filters/load_reporting/server_load_reporting_filter.cc +++ b/src/core/ext/filters/load_reporting/server_load_reporting_filter.cc @@ -247,8 +247,8 @@ void ServerLoadReportingFilter::Call::OnFinalize( } const grpc_channel_filter ServerLoadReportingFilter::kFilter = - MakePromiseBasedFilter( - "server_load_reporting"); + MakePromiseBasedFilter(); // TODO(juanlishen): We should register the filter during grpc initialization // time once OpenCensus is compatible with our build system. For now, we force diff --git a/src/core/ext/filters/load_reporting/server_load_reporting_filter.h b/src/core/ext/filters/load_reporting/server_load_reporting_filter.h index 76684093a0bab..41f66ec948d98 100644 --- a/src/core/ext/filters/load_reporting/server_load_reporting_filter.h +++ b/src/core/ext/filters/load_reporting/server_load_reporting_filter.h @@ -39,6 +39,8 @@ class ServerLoadReportingFilter public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "server_load_reporting"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args); diff --git a/src/core/ext/filters/logging/logging_filter.cc b/src/core/ext/filters/logging/logging_filter.cc index 3d95d914585e5..c0a749d315e74 100644 --- a/src/core/ext/filters/logging/logging_filter.cc +++ b/src/core/ext/filters/logging/logging_filter.cc @@ -417,7 +417,7 @@ const grpc_channel_filter ClientLoggingFilter::kFilter = MakePromiseBasedFilter("logging"); + kFilterExaminesOutboundMessages>(); absl::StatusOr> ServerLoggingFilter::Create(const ChannelArgs& /*args*/, @@ -483,7 +483,7 @@ const grpc_channel_filter ServerLoggingFilter::kFilter = MakePromiseBasedFilter("logging"); + kFilterExaminesOutboundMessages>(); void RegisterLoggingFilter(LoggingSink* sink) { g_logging_sink = sink; diff --git a/src/core/ext/filters/logging/logging_filter.h b/src/core/ext/filters/logging/logging_filter.h index 30c28a5b57aaf..5d8b77752cd0d 100644 --- a/src/core/ext/filters/logging/logging_filter.h +++ b/src/core/ext/filters/logging/logging_filter.h @@ -78,6 +78,8 @@ class ClientLoggingFilter final public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "logging"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args /*filter_args*/); @@ -108,6 +110,8 @@ class ServerLoggingFilter final public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "logging"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args /*filter_args*/); diff --git a/src/core/ext/filters/message_size/message_size_filter.cc b/src/core/ext/filters/message_size/message_size_filter.cc index 3a8a3659a432d..580770ede7bbf 100644 --- a/src/core/ext/filters/message_size/message_size_filter.cc +++ b/src/core/ext/filters/message_size/message_size_filter.cc @@ -137,12 +137,12 @@ size_t MessageSizeParser::ParserIndex() { const grpc_channel_filter ClientMessageSizeFilter::kFilter = MakePromiseBasedFilter("message_size"); + kFilterExaminesInboundMessages>(); const grpc_channel_filter ServerMessageSizeFilter::kFilter = MakePromiseBasedFilter("message_size"); + kFilterExaminesInboundMessages>(); absl::StatusOr> ClientMessageSizeFilter::Create(const ChannelArgs& args, ChannelFilter::Args) { diff --git a/src/core/ext/filters/message_size/message_size_filter.h b/src/core/ext/filters/message_size/message_size_filter.h index 970604fe35a55..3ed3b2b735b73 100644 --- a/src/core/ext/filters/message_size/message_size_filter.h +++ b/src/core/ext/filters/message_size/message_size_filter.h @@ -89,6 +89,8 @@ class ServerMessageSizeFilter final public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "message_size"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args filter_args); @@ -117,6 +119,8 @@ class ClientMessageSizeFilter final public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "message_size"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args filter_args); diff --git a/src/core/ext/filters/rbac/rbac_filter.cc b/src/core/ext/filters/rbac/rbac_filter.cc index a86636b10a8ca..583042786f062 100644 --- a/src/core/ext/filters/rbac/rbac_filter.cc +++ b/src/core/ext/filters/rbac/rbac_filter.cc @@ -71,7 +71,7 @@ absl::Status RbacFilter::Call::OnClientInitialMetadata(ClientMetadata& md, } const grpc_channel_filter RbacFilter::kFilterVtable = - MakePromiseBasedFilter("rbac_filter"); + MakePromiseBasedFilter(); RbacFilter::RbacFilter(size_t index, EvaluateArgs::PerChannelArgs per_channel_evaluate_args) diff --git a/src/core/ext/filters/rbac/rbac_filter.h b/src/core/ext/filters/rbac/rbac_filter.h index d033b799d5f63..f41e8490ccf65 100644 --- a/src/core/ext/filters/rbac/rbac_filter.h +++ b/src/core/ext/filters/rbac/rbac_filter.h @@ -42,6 +42,8 @@ class RbacFilter : public ImplementChannelFilter { // and enforces the RBAC policy. static const grpc_channel_filter kFilterVtable; + static absl::string_view TypeName() { return "rbac_filter"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args filter_args); diff --git a/src/core/ext/filters/stateful_session/stateful_session_filter.cc b/src/core/ext/filters/stateful_session/stateful_session_filter.cc index 246605a490b85..c677bd2cc1d43 100644 --- a/src/core/ext/filters/stateful_session/stateful_session_filter.cc +++ b/src/core/ext/filters/stateful_session/stateful_session_filter.cc @@ -69,8 +69,7 @@ UniqueTypeName XdsOverrideHostAttribute::TypeName() { const grpc_channel_filter StatefulSessionFilter::kFilter = MakePromiseBasedFilter( - "stateful_session_filter"); + kFilterExaminesServerInitialMetadata>(); absl::StatusOr> StatefulSessionFilter::Create(const ChannelArgs&, diff --git a/src/core/ext/filters/stateful_session/stateful_session_filter.h b/src/core/ext/filters/stateful_session/stateful_session_filter.h index 64c488bce3365..3bffbaa854753 100644 --- a/src/core/ext/filters/stateful_session/stateful_session_filter.h +++ b/src/core/ext/filters/stateful_session/stateful_session_filter.h @@ -74,6 +74,8 @@ class StatefulSessionFilter public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "stateful_session_filter"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args filter_args); diff --git a/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc b/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc index 0f71d509ba406..cecb5fa53af13 100644 --- a/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc +++ b/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc @@ -256,20 +256,23 @@ void ChaoticGoodConnector::Connect(const Args& args, Result* result, error); return; } - auto* p = self.release(); auto* chaotic_good_ext = grpc_event_engine::experimental::QueryExtension< grpc_event_engine::experimental::ChaoticGoodExtension>( - endpoint.value().get()); + endpoint->get()); if (chaotic_good_ext != nullptr) { chaotic_good_ext->EnableStatsCollection(/*is_control_channel=*/true); chaotic_good_ext->UseMemoryQuota( ResourceQuota::Default()->memory_quota()); } + auto* p = self.get(); p->handshake_mgr_->DoHandshake( - grpc_event_engine_endpoint_create(std::move(endpoint.value())), + OrphanablePtr( + grpc_event_engine_endpoint_create(std::move(*endpoint))), p->args_.channel_args, p->args_.deadline, nullptr /* acceptor */, - OnHandshakeDone, p); + [self = std::move(self)](absl::StatusOr result) { + self->OnHandshakeDone(std::move(result)); + }); }; event_engine_->Connect( std::move(on_connect), *resolved_addr_, @@ -280,45 +283,37 @@ void ChaoticGoodConnector::Connect(const Args& args, Result* result, std::chrono::seconds(kTimeoutSecs)); } -void ChaoticGoodConnector::OnHandshakeDone(void* arg, grpc_error_handle error) { - auto* args = static_cast(arg); - RefCountedPtr self( - static_cast(args->user_data)); - grpc_slice_buffer_destroy(args->read_buffer); - gpr_free(args->read_buffer); +void ChaoticGoodConnector::OnHandshakeDone( + absl::StatusOr result) { // Start receiving setting frames; { - MutexLock lock(&self->mu_); - if (!error.ok() || self->is_shutdown_) { - if (error.ok()) { + MutexLock lock(&mu_); + if (!result.ok() || is_shutdown_) { + absl::Status error = result.status(); + if (result.ok()) { error = GRPC_ERROR_CREATE("connector shutdown"); - // We were shut down after handshaking completed successfully, so - // destroy the endpoint here. - if (args->endpoint != nullptr) { - grpc_endpoint_destroy(args->endpoint); - } } - self->result_->Reset(); - ExecCtx::Run(DEBUG_LOCATION, std::exchange(self->notify_, nullptr), - error); + result_->Reset(); + ExecCtx::Run(DEBUG_LOCATION, std::exchange(notify_, nullptr), error); return; } } - if (args->endpoint != nullptr) { + if ((*result)->endpoint != nullptr) { CHECK(grpc_event_engine::experimental::grpc_is_event_engine_endpoint( - args->endpoint)); - self->control_endpoint_ = PromiseEndpoint( - grpc_event_engine::experimental:: - grpc_take_wrapped_event_engine_endpoint(args->endpoint), - SliceBuffer()); + (*result)->endpoint.get())); + control_endpoint_ = + PromiseEndpoint(grpc_event_engine::experimental:: + grpc_take_wrapped_event_engine_endpoint( + (*result)->endpoint.release()), + SliceBuffer()); auto activity = MakeActivity( - [self] { + [self = RefAsSubclass()] { return TrySeq(ControlEndpointWriteSettingsFrame(self), ControlEndpointReadSettingsFrame(self), []() { return absl::OkStatus(); }); }, - EventEngineWakeupScheduler(self->event_engine_), - [self](absl::Status status) { + EventEngineWakeupScheduler(event_engine_), + [self = RefAsSubclass()](absl::Status status) { if (GRPC_TRACE_FLAG_ENABLED(chaotic_good)) { gpr_log(GPR_INFO, "ChaoticGoodConnector::OnHandshakeDone: %s", status.ToString().c_str()); @@ -338,17 +333,19 @@ void ChaoticGoodConnector::OnHandshakeDone(void* arg, grpc_error_handle error) { status); } }, - self->arena_, self->event_engine_.get()); - MutexLock lock(&self->mu_); - if (!self->is_shutdown_) { - self->connect_activity_ = std::move(activity); + arena_, event_engine_.get()); + MutexLock lock(&mu_); + if (!is_shutdown_) { + connect_activity_ = std::move(activity); } } else { // Handshaking succeeded but there is no endpoint. - MutexLock lock(&self->mu_); - self->result_->Reset(); + MutexLock lock(&mu_); + result_->Reset(); auto error = GRPC_ERROR_CREATE("handshake complete with empty endpoint."); - ExecCtx::Run(DEBUG_LOCATION, std::exchange(self->notify_, nullptr), error); + ExecCtx::Run( + DEBUG_LOCATION, std::exchange(notify_, nullptr), + absl::InternalError("handshake complete with empty endpoint.")); } } diff --git a/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.h b/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.h index b8db7a525029a..caf2c564a146e 100644 --- a/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.h +++ b/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.h @@ -77,7 +77,7 @@ class ChaoticGoodConnector : public SubchannelConnector { RefCountedPtr self); static auto WaitForDataEndpointSetup( RefCountedPtr self); - static void OnHandshakeDone(void* arg, grpc_error_handle error); + void OnHandshakeDone(absl::StatusOr result); RefCountedPtr arena_ = SimpleArenaAllocator()->MakeArena(); Mutex mu_; diff --git a/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc b/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc index 6964d4d422cfb..b38e1e4a9df64 100644 --- a/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc +++ b/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc @@ -211,9 +211,12 @@ ChaoticGoodServerListener::ActiveConnection::HandshakingState::HandshakingState( void ChaoticGoodServerListener::ActiveConnection::HandshakingState::Start( std::unique_ptr endpoint) { handshake_mgr_->DoHandshake( - grpc_event_engine_endpoint_create(std::move(endpoint)), - connection_->args(), GetConnectionDeadline(), nullptr, OnHandshakeDone, - Ref().release()); + OrphanablePtr( + grpc_event_engine_endpoint_create(std::move(endpoint))), + connection_->args(), GetConnectionDeadline(), nullptr, + [self = Ref()](absl::StatusOr result) { + self->OnHandshakeDone(std::move(result)); + }); } auto ChaoticGoodServerListener::ActiveConnection::HandshakingState:: @@ -384,33 +387,28 @@ auto ChaoticGoodServerListener::ActiveConnection::HandshakingState:: } void ChaoticGoodServerListener::ActiveConnection::HandshakingState:: - OnHandshakeDone(void* arg, grpc_error_handle error) { - auto* args = static_cast(arg); - CHECK_NE(args, nullptr); - RefCountedPtr self( - static_cast(args->user_data)); - grpc_slice_buffer_destroy(args->read_buffer); - gpr_free(args->read_buffer); - if (!error.ok()) { - self->connection_->Done( - absl::StrCat("Handshake failed: ", StatusToString(error))); + OnHandshakeDone(absl::StatusOr result) { + if (!result.ok()) { + connection_->Done( + absl::StrCat("Handshake failed: ", result.status().ToString())); return; } - if (args->endpoint == nullptr) { - self->connection_->Done("Server handshake done but has empty endpoint."); + CHECK_NE(*result, nullptr); + if ((*result)->endpoint == nullptr) { + connection_->Done("Server handshake done but has empty endpoint."); return; } CHECK(grpc_event_engine::experimental::grpc_is_event_engine_endpoint( - args->endpoint)); + (*result)->endpoint.get())); auto ee_endpoint = grpc_event_engine::experimental::grpc_take_wrapped_event_engine_endpoint( - args->endpoint); + (*result)->endpoint.release()); auto* chaotic_good_ext = grpc_event_engine::experimental::QueryExtension< grpc_event_engine::experimental::ChaoticGoodExtension>(ee_endpoint.get()); - self->connection_->endpoint_ = + connection_->endpoint_ = PromiseEndpoint(std::move(ee_endpoint), SliceBuffer()); auto activity = MakeActivity( - [self, chaotic_good_ext]() { + [self = Ref(), chaotic_good_ext]() { return TrySeq( Race(EndpointReadSettingsFrame(self), TrySeq(Sleep(Timestamp::Now() + kConnectionDeadline), @@ -430,8 +428,8 @@ void ChaoticGoodServerListener::ActiveConnection::HandshakingState:: return EndpointWriteSettingsFrame(self, is_control_endpoint); }); }, - EventEngineWakeupScheduler(self->connection_->listener_->event_engine_), - [self](absl::Status status) { + EventEngineWakeupScheduler(connection_->listener_->event_engine_), + [self = Ref()](absl::Status status) { if (!status.ok()) { self->connection_->Done( absl::StrCat("Server setting frame handling failed: ", @@ -440,11 +438,10 @@ void ChaoticGoodServerListener::ActiveConnection::HandshakingState:: self->connection_->Done(); } }, - self->connection_->arena_.get(), - self->connection_->listener_->event_engine_.get()); - MutexLock lock(&self->connection_->mu_); - if (self->connection_->orphaned_) return; - self->connection_->receive_settings_activity_ = std::move(activity); + connection_->arena_.get(), connection_->listener_->event_engine_.get()); + MutexLock lock(&connection_->mu_); + if (connection_->orphaned_) return; + connection_->receive_settings_activity_ = std::move(activity); } Timestamp ChaoticGoodServerListener::ActiveConnection::HandshakingState:: diff --git a/src/core/ext/transport/chaotic_good/server/chaotic_good_server.h b/src/core/ext/transport/chaotic_good/server/chaotic_good_server.h index 8511790c03bb4..8da7bc7513ec6 100644 --- a/src/core/ext/transport/chaotic_good/server/chaotic_good_server.h +++ b/src/core/ext/transport/chaotic_good/server/chaotic_good_server.h @@ -104,7 +104,7 @@ class ChaoticGoodServerListener final : public Server::ListenerInterface { static auto DataEndpointWriteSettingsFrame( RefCountedPtr self); - static void OnHandshakeDone(void* arg, grpc_error_handle error); + void OnHandshakeDone(absl::StatusOr result); Timestamp GetConnectionDeadline(); const RefCountedPtr connection_; const RefCountedPtr handshake_mgr_; diff --git a/src/core/ext/transport/chttp2/client/chttp2_connector.cc b/src/core/ext/transport/chttp2/client/chttp2_connector.cc index 8711eece3bfb0..6fb92f0d6f819 100644 --- a/src/core/ext/transport/chttp2/client/chttp2_connector.cc +++ b/src/core/ext/transport/chttp2/client/chttp2_connector.cc @@ -120,10 +120,12 @@ void Chttp2Connector::Connect(const Args& args, Result* result, CoreConfiguration::Get().handshaker_registry().AddHandshakers( HANDSHAKER_CLIENT, channel_args, args_.interested_parties, handshake_mgr_.get()); - Ref().release(); // Ref held by OnHandshakeDone(). - handshake_mgr_->DoHandshake(nullptr /* endpoint */, channel_args, - args.deadline, nullptr /* acceptor */, - OnHandshakeDone, this); + handshake_mgr_->DoHandshake( + /*endpoint=*/nullptr, channel_args, args.deadline, /*acceptor=*/nullptr, + [self = RefAsSubclass()]( + absl::StatusOr result) { + self->OnHandshakeDone(std::move(result)); + }); } void Chttp2Connector::Shutdown(grpc_error_handle error) { @@ -135,54 +137,42 @@ void Chttp2Connector::Shutdown(grpc_error_handle error) { } } -void Chttp2Connector::OnHandshakeDone(void* arg, grpc_error_handle error) { - auto* args = static_cast(arg); - Chttp2Connector* self = static_cast(args->user_data); - { - MutexLock lock(&self->mu_); - if (!error.ok() || self->shutdown_) { - if (error.ok()) { - error = GRPC_ERROR_CREATE("connector shutdown"); - // We were shut down after handshaking completed successfully, so - // destroy the endpoint here. - if (args->endpoint != nullptr) { - grpc_endpoint_destroy(args->endpoint); - grpc_slice_buffer_destroy(args->read_buffer); - gpr_free(args->read_buffer); - } - } - self->result_->Reset(); - NullThenSchedClosure(DEBUG_LOCATION, &self->notify_, error); - } else if (args->endpoint != nullptr) { - self->result_->transport = - grpc_create_chttp2_transport(args->args, args->endpoint, true); - CHECK_NE(self->result_->transport, nullptr); - self->result_->socket_node = - grpc_chttp2_transport_get_socket_node(self->result_->transport); - self->result_->channel_args = args->args; - self->Ref().release(); // Ref held by OnReceiveSettings() - GRPC_CLOSURE_INIT(&self->on_receive_settings_, OnReceiveSettings, self, - grpc_schedule_on_exec_ctx); - grpc_chttp2_transport_start_reading( - self->result_->transport, args->read_buffer, - &self->on_receive_settings_, self->args_.interested_parties, nullptr); - self->timer_handle_ = self->event_engine_->RunAfter( - self->args_.deadline - Timestamp::Now(), - [self = self->RefAsSubclass()] { - ApplicationCallbackExecCtx callback_exec_ctx; - ExecCtx exec_ctx; - self->OnTimeout(); - }); - } else { - // If the handshaking succeeded but there is no endpoint, then the - // handshaker may have handed off the connection to some external - // code. Just verify that exit_early flag is set. - DCHECK(args->exit_early); - NullThenSchedClosure(DEBUG_LOCATION, &self->notify_, error); +void Chttp2Connector::OnHandshakeDone(absl::StatusOr result) { + MutexLock lock(&mu_); + if (!result.ok() || shutdown_) { + if (result.ok()) { + result = GRPC_ERROR_CREATE("connector shutdown"); } - self->handshake_mgr_.reset(); + result_->Reset(); + NullThenSchedClosure(DEBUG_LOCATION, ¬ify_, result.status()); + } else if ((*result)->endpoint != nullptr) { + result_->transport = grpc_create_chttp2_transport( + (*result)->args, std::move((*result)->endpoint), true); + CHECK_NE(result_->transport, nullptr); + result_->socket_node = + grpc_chttp2_transport_get_socket_node(result_->transport); + result_->channel_args = std::move((*result)->args); + Ref().release(); // Ref held by OnReceiveSettings() + GRPC_CLOSURE_INIT(&on_receive_settings_, OnReceiveSettings, this, + grpc_schedule_on_exec_ctx); + grpc_chttp2_transport_start_reading( + result_->transport, (*result)->read_buffer.c_slice_buffer(), + &on_receive_settings_, args_.interested_parties, nullptr); + timer_handle_ = + event_engine_->RunAfter(args_.deadline - Timestamp::Now(), + [self = RefAsSubclass()] { + ApplicationCallbackExecCtx callback_exec_ctx; + ExecCtx exec_ctx; + self->OnTimeout(); + }); + } else { + // If the handshaking succeeded but there is no endpoint, then the + // handshaker may have handed off the connection to some external + // code. Just verify that exit_early flag is set. + DCHECK((*result)->exit_early); + NullThenSchedClosure(DEBUG_LOCATION, ¬ify_, result.status()); } - self->Unref(); + handshake_mgr_.reset(); } void Chttp2Connector::OnReceiveSettings(void* arg, grpc_error_handle error) { @@ -380,12 +370,12 @@ grpc_channel* grpc_channel_create_from_fd(const char* target, int fd, int flags = fcntl(fd, F_GETFL, 0); CHECK_EQ(fcntl(fd, F_SETFL, flags | O_NONBLOCK), 0); - grpc_endpoint* client = grpc_tcp_create_from_fd( + grpc_core::OrphanablePtr client(grpc_tcp_create_from_fd( grpc_fd_create(fd, "client", true), grpc_event_engine::experimental::ChannelArgsEndpointConfig(final_args), - "fd-client"); + "fd-client")); grpc_core::Transport* transport = - grpc_create_chttp2_transport(final_args, client, true); + grpc_create_chttp2_transport(final_args, std::move(client), true); CHECK(transport); auto channel = grpc_core::ChannelCreate( target, final_args, GRPC_CLIENT_DIRECT_CHANNEL, transport); diff --git a/src/core/ext/transport/chttp2/client/chttp2_connector.h b/src/core/ext/transport/chttp2/client/chttp2_connector.h index 679c7db6ce99d..0cb08474ca62f 100644 --- a/src/core/ext/transport/chttp2/client/chttp2_connector.h +++ b/src/core/ext/transport/chttp2/client/chttp2_connector.h @@ -41,7 +41,7 @@ class Chttp2Connector : public SubchannelConnector { void Shutdown(grpc_error_handle error) override; private: - static void OnHandshakeDone(void* arg, grpc_error_handle error); + void OnHandshakeDone(absl::StatusOr result); static void OnReceiveSettings(void* arg, grpc_error_handle error); void OnTimeout() ABSL_LOCKS_EXCLUDED(mu_); diff --git a/src/core/ext/transport/chttp2/server/chttp2_server.cc b/src/core/ext/transport/chttp2/server/chttp2_server.cc index b20d2a5554863..37196c21b608e 100644 --- a/src/core/ext/transport/chttp2/server/chttp2_server.cc +++ b/src/core/ext/transport/chttp2/server/chttp2_server.cc @@ -107,6 +107,13 @@ const char kUnixUriPrefix[] = "unix:"; const char kUnixAbstractUriPrefix[] = "unix-abstract:"; const char kVSockUriPrefix[] = "vsock:"; +struct AcceptorDeleter { + void operator()(grpc_tcp_server_acceptor* acceptor) const { + gpr_free(acceptor); + } +}; +using AcceptorPtr = std::unique_ptr; + class Chttp2ServerListener : public Server::ListenerInterface { public: static grpc_error_handle Create(Server* server, grpc_resolved_address* addr, @@ -167,15 +174,15 @@ class Chttp2ServerListener : public Server::ListenerInterface { class HandshakingState : public InternallyRefCounted { public: HandshakingState(RefCountedPtr connection_ref, - grpc_pollset* accepting_pollset, - grpc_tcp_server_acceptor* acceptor, + grpc_pollset* accepting_pollset, AcceptorPtr acceptor, const ChannelArgs& args); ~HandshakingState() override; void Orphan() override; - void Start(grpc_endpoint* endpoint, const ChannelArgs& args); + void Start(OrphanablePtr endpoint, + const ChannelArgs& args); // Needed to be able to grab an external ref in // ActiveConnection::Start() @@ -184,10 +191,10 @@ class Chttp2ServerListener : public Server::ListenerInterface { private: void OnTimeout() ABSL_LOCKS_EXCLUDED(&connection_->mu_); static void OnReceiveSettings(void* arg, grpc_error_handle /* error */); - static void OnHandshakeDone(void* arg, grpc_error_handle error); + void OnHandshakeDone(absl::StatusOr result); RefCountedPtr const connection_; grpc_pollset* const accepting_pollset_; - grpc_tcp_server_acceptor* acceptor_; + AcceptorPtr acceptor_; RefCountedPtr handshake_mgr_ ABSL_GUARDED_BY(&connection_->mu_); // State for enforcing handshake timeout on receiving HTTP/2 settings. @@ -198,8 +205,7 @@ class Chttp2ServerListener : public Server::ListenerInterface { grpc_pollset_set* const interested_parties_; }; - ActiveConnection(grpc_pollset* accepting_pollset, - grpc_tcp_server_acceptor* acceptor, + ActiveConnection(grpc_pollset* accepting_pollset, AcceptorPtr acceptor, EventEngine* event_engine, const ChannelArgs& args, MemoryOwner memory_owner); ~ActiveConnection() override; @@ -209,7 +215,7 @@ class Chttp2ServerListener : public Server::ListenerInterface { void SendGoAway(); void Start(RefCountedPtr listener, - grpc_endpoint* endpoint, const ChannelArgs& args); + OrphanablePtr endpoint, const ChannelArgs& args); // Needed to be able to grab an external ref in // Chttp2ServerListener::OnAccept() @@ -367,11 +373,11 @@ Timestamp GetConnectionDeadline(const ChannelArgs& args) { Chttp2ServerListener::ActiveConnection::HandshakingState::HandshakingState( RefCountedPtr connection_ref, - grpc_pollset* accepting_pollset, grpc_tcp_server_acceptor* acceptor, + grpc_pollset* accepting_pollset, AcceptorPtr acceptor, const ChannelArgs& args) : connection_(std::move(connection_ref)), accepting_pollset_(accepting_pollset), - acceptor_(acceptor), + acceptor_(std::move(acceptor)), handshake_mgr_(MakeRefCounted()), deadline_(GetConnectionDeadline(args)), interested_parties_(grpc_pollset_set_create()) { @@ -387,7 +393,6 @@ Chttp2ServerListener::ActiveConnection::HandshakingState::~HandshakingState() { grpc_pollset_set_del_pollset(interested_parties_, accepting_pollset_); } grpc_pollset_set_destroy(interested_parties_); - gpr_free(acceptor_); } void Chttp2ServerListener::ActiveConnection::HandshakingState::Orphan() { @@ -401,16 +406,18 @@ void Chttp2ServerListener::ActiveConnection::HandshakingState::Orphan() { } void Chttp2ServerListener::ActiveConnection::HandshakingState::Start( - grpc_endpoint* endpoint, const ChannelArgs& channel_args) { - Ref().release(); // Held by OnHandshakeDone + OrphanablePtr endpoint, const ChannelArgs& channel_args) { RefCountedPtr handshake_mgr; { MutexLock lock(&connection_->mu_); if (handshake_mgr_ == nullptr) return; handshake_mgr = handshake_mgr_; } - handshake_mgr->DoHandshake(endpoint, channel_args, deadline_, acceptor_, - OnHandshakeDone, this); + handshake_mgr->DoHandshake( + std::move(endpoint), channel_args, deadline_, acceptor_.get(), + [self = Ref()](absl::StatusOr result) { + self->OnHandshakeDone(std::move(result)); + }); } void Chttp2ServerListener::ActiveConnection::HandshakingState::OnTimeout() { @@ -444,61 +451,50 @@ void Chttp2ServerListener::ActiveConnection::HandshakingState:: } void Chttp2ServerListener::ActiveConnection::HandshakingState::OnHandshakeDone( - void* arg, grpc_error_handle error) { - auto* args = static_cast(arg); - HandshakingState* self = static_cast(args->user_data); + absl::StatusOr result) { OrphanablePtr handshaking_state_ref; RefCountedPtr handshake_mgr; bool cleanup_connection = false; bool release_connection = false; { - MutexLock connection_lock(&self->connection_->mu_); - if (!error.ok() || self->connection_->shutdown_) { - std::string error_str = StatusToString(error); + MutexLock connection_lock(&connection_->mu_); + if (!result.ok() || connection_->shutdown_) { cleanup_connection = true; release_connection = true; - if (error.ok() && args->endpoint != nullptr) { - // We were shut down or stopped serving after handshaking completed - // successfully, so destroy the endpoint here. - grpc_endpoint_destroy(args->endpoint); - grpc_slice_buffer_destroy(args->read_buffer); - gpr_free(args->read_buffer); - } } else { // If the handshaking succeeded but there is no endpoint, then the // handshaker may have handed off the connection to some external // code, so we can just clean up here without creating a transport. - if (args->endpoint != nullptr) { + if ((*result)->endpoint != nullptr) { RefCountedPtr transport = - grpc_create_chttp2_transport(args->args, args->endpoint, false) + grpc_create_chttp2_transport((*result)->args, + std::move((*result)->endpoint), false) ->Ref(); grpc_error_handle channel_init_err = - self->connection_->listener_->server_->SetupTransport( - transport.get(), self->accepting_pollset_, args->args, + connection_->listener_->server_->SetupTransport( + transport.get(), accepting_pollset_, (*result)->args, grpc_chttp2_transport_get_socket_node(transport.get())); if (channel_init_err.ok()) { // Use notify_on_receive_settings callback to enforce the // handshake deadline. - self->connection_->transport_ = + connection_->transport_ = DownCast(transport.get())->Ref(); - self->Ref().release(); // Held by OnReceiveSettings(). - GRPC_CLOSURE_INIT(&self->on_receive_settings_, OnReceiveSettings, - self, grpc_schedule_on_exec_ctx); + Ref().release(); // Held by OnReceiveSettings(). + GRPC_CLOSURE_INIT(&on_receive_settings_, OnReceiveSettings, this, + grpc_schedule_on_exec_ctx); // If the listener has been configured with a config fetcher, we // need to watch on the transport being closed so that we can an // updated list of active connections. grpc_closure* on_close = nullptr; - if (self->connection_->listener_->config_fetcher_watcher_ != - nullptr) { + if (connection_->listener_->config_fetcher_watcher_ != nullptr) { // Refs helds by OnClose() - self->connection_->Ref().release(); - on_close = &self->connection_->on_close_; + connection_->Ref().release(); + on_close = &connection_->on_close_; } else { // Remove the connection from the connections_ map since OnClose() // will not be invoked when a config fetcher is set. auto connection_quota = - self->connection_->listener_->connection_quota_->Ref() - .release(); + connection_->listener_->connection_quota_->Ref().release(); auto on_close_transport = [](void* arg, grpc_error_handle /*handle*/) { ConnectionQuota* connection_quota = @@ -511,11 +507,10 @@ void Chttp2ServerListener::ActiveConnection::HandshakingState::OnHandshakeDone( cleanup_connection = true; } grpc_chttp2_transport_start_reading( - transport.get(), args->read_buffer, &self->on_receive_settings_, - nullptr, on_close); - self->timer_handle_ = self->connection_->event_engine_->RunAfter( - self->deadline_ - Timestamp::Now(), - [self = self->Ref()]() mutable { + transport.get(), (*result)->read_buffer.c_slice_buffer(), + &on_receive_settings_, nullptr, on_close); + timer_handle_ = connection_->event_engine_->RunAfter( + deadline_ - Timestamp::Now(), [self = Ref()]() mutable { ApplicationCallbackExecCtx callback_exec_ctx; ExecCtx exec_ctx; self->OnTimeout(); @@ -527,8 +522,6 @@ void Chttp2ServerListener::ActiveConnection::HandshakingState::OnHandshakeDone( LOG(ERROR) << "Failed to create channel: " << StatusToString(channel_init_err); transport->Orphan(); - grpc_slice_buffer_destroy(args->read_buffer); - gpr_free(args->read_buffer); cleanup_connection = true; release_connection = true; } @@ -541,25 +534,21 @@ void Chttp2ServerListener::ActiveConnection::HandshakingState::OnHandshakeDone( // shutdown the handshake when the listener needs to stop serving. // Avoid calling the destructor of HandshakeManager and HandshakingState // from within the critical region. - handshake_mgr = std::move(self->handshake_mgr_); - handshaking_state_ref = std::move(self->connection_->handshaking_state_); + handshake_mgr = std::move(handshake_mgr_); + handshaking_state_ref = std::move(connection_->handshaking_state_); } - gpr_free(self->acceptor_); - self->acceptor_ = nullptr; OrphanablePtr connection; if (cleanup_connection) { - MutexLock listener_lock(&self->connection_->listener_->mu_); + MutexLock listener_lock(&connection_->listener_->mu_); if (release_connection) { - self->connection_->listener_->connection_quota_->ReleaseConnections(1); + connection_->listener_->connection_quota_->ReleaseConnections(1); } - auto it = self->connection_->listener_->connections_.find( - self->connection_.get()); - if (it != self->connection_->listener_->connections_.end()) { + auto it = connection_->listener_->connections_.find(connection_.get()); + if (it != connection_->listener_->connections_.end()) { connection = std::move(it->second); - self->connection_->listener_->connections_.erase(it); + connection_->listener_->connections_.erase(it); } } - self->Unref(); } // @@ -567,11 +556,11 @@ void Chttp2ServerListener::ActiveConnection::HandshakingState::OnHandshakeDone( // Chttp2ServerListener::ActiveConnection::ActiveConnection( - grpc_pollset* accepting_pollset, grpc_tcp_server_acceptor* acceptor, + grpc_pollset* accepting_pollset, AcceptorPtr acceptor, EventEngine* event_engine, const ChannelArgs& args, MemoryOwner memory_owner) : handshaking_state_(memory_owner.MakeOrphanable( - Ref(), accepting_pollset, acceptor, args)), + Ref(), accepting_pollset, std::move(acceptor), args)), event_engine_(event_engine) { GRPC_CLOSURE_INIT(&on_close_, ActiveConnection::OnClose, this, grpc_schedule_on_exec_ctx); @@ -625,29 +614,24 @@ void Chttp2ServerListener::ActiveConnection::SendGoAway() { } void Chttp2ServerListener::ActiveConnection::Start( - RefCountedPtr listener, grpc_endpoint* endpoint, - const ChannelArgs& args) { - RefCountedPtr handshaking_state_ref; + RefCountedPtr listener, + OrphanablePtr endpoint, const ChannelArgs& args) { listener_ = std::move(listener); if (listener_->tcp_server_ != nullptr) { grpc_tcp_server_ref(listener_->tcp_server_); } + RefCountedPtr handshaking_state_ref; { - ReleasableMutexLock lock(&mu_); - if (shutdown_) { - lock.Release(); - // If the Connection is already shutdown at this point, it implies the - // owning Chttp2ServerListener and all associated ActiveConnections have - // been orphaned. The generated endpoints need to be shutdown here to - // ensure the tcp connections are closed appropriately. - grpc_endpoint_destroy(endpoint); - return; - } + MutexLock lock(&mu_); + // If the Connection is already shutdown at this point, it implies the + // owning Chttp2ServerListener and all associated ActiveConnections have + // been orphaned. + if (shutdown_) return; // Hold a ref to HandshakingState to allow starting the handshake outside // the critical region. handshaking_state_ref = handshaking_state_->Ref(); } - handshaking_state_ref->Start(endpoint, args); + handshaking_state_ref->Start(std::move(endpoint), args); } void Chttp2ServerListener::ActiveConnection::OnClose( @@ -841,48 +825,41 @@ void Chttp2ServerListener::AcceptConnectedEndpoint( void Chttp2ServerListener::OnAccept(void* arg, grpc_endpoint* tcp, grpc_pollset* accepting_pollset, - grpc_tcp_server_acceptor* acceptor) { + grpc_tcp_server_acceptor* server_acceptor) { Chttp2ServerListener* self = static_cast(arg); ChannelArgs args = self->args_; + OrphanablePtr endpoint(tcp); + AcceptorPtr acceptor(server_acceptor); RefCountedPtr connection_manager; { MutexLock lock(&self->mu_); connection_manager = self->connection_manager_; } - auto endpoint_cleanup = [&]() { - grpc_endpoint_destroy(tcp); - gpr_free(acceptor); - }; if (!self->connection_quota_->AllowIncomingConnection( - self->memory_quota_, grpc_endpoint_get_peer(tcp))) { - endpoint_cleanup(); + self->memory_quota_, grpc_endpoint_get_peer(endpoint.get()))) { return; } if (self->config_fetcher_ != nullptr) { if (connection_manager == nullptr) { - endpoint_cleanup(); return; } absl::StatusOr args_result = connection_manager->UpdateChannelArgsForConnection(args, tcp); if (!args_result.ok()) { - endpoint_cleanup(); return; } grpc_error_handle error; args = self->args_modifier_(*args_result, &error); if (!error.ok()) { - endpoint_cleanup(); return; } } auto memory_owner = self->memory_quota_->CreateMemoryOwner(); EventEngine* const event_engine = self->args_.GetObject(); auto connection = memory_owner.MakeOrphanable( - accepting_pollset, acceptor, event_engine, args, std::move(memory_owner)); - // We no longer own acceptor - acceptor = nullptr; + accepting_pollset, std::move(acceptor), event_engine, args, + std::move(memory_owner)); // Hold a ref to connection to allow starting handshake outside the // critical region RefCountedPtr connection_ref = connection->Ref(); @@ -902,10 +879,8 @@ void Chttp2ServerListener::OnAccept(void* arg, grpc_endpoint* tcp, self->connections_.emplace(connection.get(), std::move(connection)); } } - if (connection != nullptr) { - endpoint_cleanup(); - } else { - connection_ref->Start(std::move(listener_ref), tcp, args); + if (connection == nullptr) { + connection_ref->Start(std::move(listener_ref), std::move(endpoint), args); } } @@ -1161,15 +1136,17 @@ void grpc_server_add_channel_from_fd(grpc_server* server, int fd, std::string name = absl::StrCat("fd:", fd); auto memory_quota = server_args.GetObject()->memory_quota(); - grpc_endpoint* server_endpoint = grpc_tcp_create_from_fd( - grpc_fd_create(fd, name.c_str(), true), - grpc_event_engine::experimental::ChannelArgsEndpointConfig(server_args), - name); + grpc_core::OrphanablePtr server_endpoint( + grpc_tcp_create_from_fd( + grpc_fd_create(fd, name.c_str(), true), + grpc_event_engine::experimental::ChannelArgsEndpointConfig( + server_args), + name)); for (grpc_pollset* pollset : core_server->pollsets()) { - grpc_endpoint_add_to_pollset(server_endpoint, pollset); + grpc_endpoint_add_to_pollset(server_endpoint.get(), pollset); } grpc_core::Transport* transport = grpc_create_chttp2_transport( - server_args, server_endpoint, false // is_client + server_args, std::move(server_endpoint), false // is_client ); grpc_error_handle error = core_server->SetupTransport(transport, nullptr, server_args, nullptr); diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index fe00b2e4bdc46..349e6ceb575f9 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -84,6 +84,7 @@ #include "src/core/lib/gprpp/status_helper.h" #include "src/core/lib/gprpp/time.h" #include "src/core/lib/iomgr/combiner.h" +#include "src/core/lib/iomgr/endpoint.h" #include "src/core/lib/iomgr/error.h" #include "src/core/lib/iomgr/ev_posix.h" #include "src/core/lib/iomgr/event_engine_shims/endpoint.h" @@ -378,8 +379,6 @@ grpc_chttp2_transport::~grpc_chttp2_transport() { channelz_socket.reset(); } - if (ep != nullptr) grpc_endpoint_destroy(ep); - grpc_slice_buffer_destroy(&qbuf); grpc_error_handle error = GRPC_ERROR_CREATE("Transport destroyed"); @@ -495,7 +494,7 @@ static void read_channel_args(grpc_chttp2_transport* t, .value_or(GRPC_ENABLE_CHANNELZ_DEFAULT)) { t->channelz_socket = grpc_core::MakeRefCounted( - std::string(grpc_endpoint_get_local_address(t->ep)), + std::string(grpc_endpoint_get_local_address(t->ep.get())), std::string(t->peer_string.as_string_view()), absl::StrCat(t->GetTransportName(), " ", t->peer_string.as_string_view()), @@ -589,11 +588,11 @@ using grpc_event_engine::experimental::QueryExtension; using grpc_event_engine::experimental::TcpTraceExtension; grpc_chttp2_transport::grpc_chttp2_transport( - const grpc_core::ChannelArgs& channel_args, grpc_endpoint* ep, - bool is_client) - : ep(ep), + const grpc_core::ChannelArgs& channel_args, + grpc_core::OrphanablePtr endpoint, bool is_client) + : ep(std::move(endpoint)), peer_string( - grpc_core::Slice::FromCopiedString(grpc_endpoint_get_peer(ep))), + grpc_core::Slice::FromCopiedString(grpc_endpoint_get_peer(ep.get()))), memory_owner(channel_args.GetObject() ->memory_quota() ->CreateMemoryOwner()), @@ -617,10 +616,11 @@ grpc_chttp2_transport::grpc_chttp2_transport( context_list = new grpc_core::ContextList(); if (channel_args.GetBool(GRPC_ARG_TCP_TRACING_ENABLED).value_or(false) && - grpc_event_engine::experimental::grpc_is_event_engine_endpoint(ep)) { + grpc_event_engine::experimental::grpc_is_event_engine_endpoint( + ep.get())) { auto epte = QueryExtension( grpc_event_engine::experimental::grpc_get_wrapped_event_engine_endpoint( - ep)); + ep.get())); if (epte != nullptr) { epte->InitializeAndReturnTcpTracer(); } @@ -763,17 +763,16 @@ static void close_transport_locked(grpc_chttp2_transport* t, CHECK(t->write_state == GRPC_CHTTP2_WRITE_STATE_IDLE); if (t->interested_parties_until_recv_settings != nullptr) { grpc_endpoint_delete_from_pollset_set( - t->ep, t->interested_parties_until_recv_settings); + t->ep.get(), t->interested_parties_until_recv_settings); t->interested_parties_until_recv_settings = nullptr; } grpc_core::MutexLock lock(&t->ep_destroy_mu); - grpc_endpoint_destroy(t->ep); - t->ep = nullptr; + t->ep.reset(); } if (t->notify_on_receive_settings != nullptr) { if (t->interested_parties_until_recv_settings != nullptr) { grpc_endpoint_delete_from_pollset_set( - t->ep, t->interested_parties_until_recv_settings); + t->ep.get(), t->interested_parties_until_recv_settings); t->interested_parties_until_recv_settings = nullptr; } grpc_core::ExecCtx::Run(DEBUG_LOCATION, t->notify_on_receive_settings, @@ -1061,7 +1060,7 @@ static void write_action(grpc_chttp2_transport* t) { << (t->is_client ? "CLIENT" : "SERVER") << "[" << t << "]: Write " << t->outbuf.Length() << " bytes"; t->write_size_policy.BeginWrite(t->outbuf.Length()); - grpc_endpoint_write(t->ep, t->outbuf.c_slice_buffer(), + grpc_endpoint_write(t->ep.get(), t->outbuf.c_slice_buffer(), grpc_core::InitTransportClosure( t->Ref(), &t->write_action_end_locked), cl, max_frame_size); @@ -1939,13 +1938,13 @@ static void perform_transport_op_locked(void* stream_op, if (op->bind_pollset) { if (t->ep != nullptr) { - grpc_endpoint_add_to_pollset(t->ep, op->bind_pollset); + grpc_endpoint_add_to_pollset(t->ep.get(), op->bind_pollset); } } if (op->bind_pollset_set) { if (t->ep != nullptr) { - grpc_endpoint_add_to_pollset_set(t->ep, op->bind_pollset_set); + grpc_endpoint_add_to_pollset_set(t->ep.get(), op->bind_pollset_set); } } @@ -2763,7 +2762,7 @@ static void continue_read_action_locked( grpc_core::RefCountedPtr t) { const bool urgent = !t->goaway_error.ok(); auto* tp = t.get(); - grpc_endpoint_read(tp->ep, &tp->read_buffer, + grpc_endpoint_read(tp->ep.get(), &tp->read_buffer, grpc_core::InitTransportClosure( std::move(t), &tp->read_action_locked), urgent, grpc_chttp2_min_read_progress_size(tp)); @@ -3026,7 +3025,7 @@ void grpc_chttp2_transport::SetPollset(grpc_stream* /*gs*/, // actually uses pollsets. if (strcmp(grpc_get_poll_strategy_name(), "poll") != 0) return; grpc_core::MutexLock lock(&ep_destroy_mu); - if (ep != nullptr) grpc_endpoint_add_to_pollset(ep, pollset); + if (ep != nullptr) grpc_endpoint_add_to_pollset(ep.get(), pollset); } void grpc_chttp2_transport::SetPollsetSet(grpc_stream* /*gs*/, @@ -3036,7 +3035,7 @@ void grpc_chttp2_transport::SetPollsetSet(grpc_stream* /*gs*/, // actually uses pollsets. if (strcmp(grpc_get_poll_strategy_name(), "poll") != 0) return; grpc_core::MutexLock lock(&ep_destroy_mu); - if (ep != nullptr) grpc_endpoint_add_to_pollset_set(ep, pollset_set); + if (ep != nullptr) grpc_endpoint_add_to_pollset_set(ep.get(), pollset_set); } // @@ -3215,9 +3214,9 @@ grpc_chttp2_transport_get_socket_node(grpc_core::Transport* transport) { } grpc_core::Transport* grpc_create_chttp2_transport( - const grpc_core::ChannelArgs& channel_args, grpc_endpoint* ep, - bool is_client) { - return new grpc_chttp2_transport(channel_args, ep, is_client); + const grpc_core::ChannelArgs& channel_args, + grpc_core::OrphanablePtr ep, bool is_client) { + return new grpc_chttp2_transport(channel_args, std::move(ep), is_client); } void grpc_chttp2_transport_start_reading( @@ -3228,7 +3227,6 @@ void grpc_chttp2_transport_start_reading( auto t = reinterpret_cast(transport)->Ref(); if (read_buffer != nullptr) { grpc_slice_buffer_move_into(read_buffer, &t->read_buffer); - gpr_free(read_buffer); } auto* tp = t.get(); tp->combiner->Run( @@ -3240,7 +3238,7 @@ void grpc_chttp2_transport_start_reading( if (t->ep != nullptr && interested_parties_until_recv_settings != nullptr) { grpc_endpoint_delete_from_pollset_set( - t->ep, interested_parties_until_recv_settings); + t->ep.get(), interested_parties_until_recv_settings); } grpc_core::ExecCtx::Run(DEBUG_LOCATION, notify_on_receive_settings, t->closed_with_error); diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.h b/src/core/ext/transport/chttp2/transport/chttp2_transport.h index 1bcb8a9ae102d..d2b9329803813 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.h +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.h @@ -44,8 +44,8 @@ /// from the caller; if the caller still needs the resource_user after creating /// a transport, the caller must take another ref. grpc_core::Transport* grpc_create_chttp2_transport( - const grpc_core::ChannelArgs& channel_args, grpc_endpoint* ep, - bool is_client); + const grpc_core::ChannelArgs& channel_args, + grpc_core::OrphanablePtr ep, bool is_client); grpc_core::RefCountedPtr grpc_chttp2_transport_get_socket_node(grpc_core::Transport* transport); diff --git a/src/core/ext/transport/chttp2/transport/frame_settings.cc b/src/core/ext/transport/chttp2/transport/frame_settings.cc index bea244c8187c4..07b07d41b1389 100644 --- a/src/core/ext/transport/chttp2/transport/frame_settings.cc +++ b/src/core/ext/transport/chttp2/transport/frame_settings.cc @@ -110,7 +110,7 @@ grpc_error_handle grpc_chttp2_settings_parser_parse(void* p, if (t->notify_on_receive_settings != nullptr) { if (t->interested_parties_until_recv_settings != nullptr) { grpc_endpoint_delete_from_pollset_set( - t->ep, t->interested_parties_until_recv_settings); + t->ep.get(), t->interested_parties_until_recv_settings); t->interested_parties_until_recv_settings = nullptr; } grpc_core::ExecCtx::Run(DEBUG_LOCATION, diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index 6e077a835ed47..1f41d7ea10f26 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -226,7 +226,8 @@ typedef enum { struct grpc_chttp2_transport final : public grpc_core::FilterStackTransport, public grpc_core::KeepsGrpcInitialized { grpc_chttp2_transport(const grpc_core::ChannelArgs& channel_args, - grpc_endpoint* ep, bool is_client); + grpc_core::OrphanablePtr endpoint, + bool is_client); ~grpc_chttp2_transport() override; void Orphan() override; @@ -257,7 +258,7 @@ struct grpc_chttp2_transport final : public grpc_core::FilterStackTransport, grpc_pollset_set* pollset_set) override; void PerformOp(grpc_transport_op* op) override; - grpc_endpoint* ep; + grpc_core::OrphanablePtr ep; grpc_core::Mutex ep_destroy_mu; // Guards endpoint destruction only. grpc_core::Slice peer_string; diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc index e0a8297b31aae..851670b55559e 100644 --- a/src/core/ext/transport/chttp2/transport/parsing.cc +++ b/src/core/ext/transport/chttp2/transport/parsing.cc @@ -717,7 +717,7 @@ static grpc_error_handle init_header_frame_parser(grpc_chttp2_transport* t, gpr_log(GPR_INFO, "[t:%p fd:%d peer:%s] Accepting new stream; " "num_incoming_streams_before_settings_ack=%u", - t, grpc_endpoint_get_fd(t->ep), + t, grpc_endpoint_get_fd(t->ep.get()), std::string(t->peer_string.as_string_view()).c_str(), t->num_incoming_streams_before_settings_ack); } diff --git a/src/core/ext/transport/chttp2/transport/writing.cc b/src/core/ext/transport/chttp2/transport/writing.cc index 7112e1ffbf3ea..6ffdcea46f05f 100644 --- a/src/core/ext/transport/chttp2/transport/writing.cc +++ b/src/core/ext/transport/chttp2/transport/writing.cc @@ -676,7 +676,7 @@ grpc_chttp2_begin_write_result grpc_chttp2_begin_write( num_stream_bytes = t->outbuf.c_slice_buffer()->length - orig_len; s->byte_counter += static_cast(num_stream_bytes); ++s->write_counter; - if (s->traced && grpc_endpoint_can_track_err(t->ep)) { + if (s->traced && grpc_endpoint_can_track_err(t->ep.get())) { grpc_core::CopyContextFn copy_context_fn = grpc_core::GrpcHttp2GetCopyContextFn(); if (copy_context_fn != nullptr && diff --git a/src/core/handshaker/endpoint_info/endpoint_info_handshaker.cc b/src/core/handshaker/endpoint_info/endpoint_info_handshaker.cc index 7db1842d5d8ac..153eddbeeccb1 100644 --- a/src/core/handshaker/endpoint_info/endpoint_info_handshaker.cc +++ b/src/core/handshaker/endpoint_info/endpoint_info_handshaker.cc @@ -17,7 +17,9 @@ #include "src/core/handshaker/endpoint_info/endpoint_info_handshaker.h" #include +#include +#include "absl/functional/any_invocable.h" #include "absl/status/status.h" #include @@ -38,17 +40,17 @@ namespace { class EndpointInfoHandshaker : public Handshaker { public: - const char* name() const override { return "endpoint_info"; } + absl::string_view name() const override { return "endpoint_info"; } - void DoHandshake(grpc_tcp_server_acceptor* /*acceptor*/, - grpc_closure* on_handshake_done, - HandshakerArgs* args) override { + void DoHandshake( + HandshakerArgs* args, + absl::AnyInvocable on_handshake_done) override { args->args = args->args .Set(GRPC_ARG_ENDPOINT_LOCAL_ADDRESS, - grpc_endpoint_get_local_address(args->endpoint)) + grpc_endpoint_get_local_address(args->endpoint.get())) .Set(GRPC_ARG_ENDPOINT_PEER_ADDRESS, - grpc_endpoint_get_peer(args->endpoint)); - ExecCtx::Run(DEBUG_LOCATION, on_handshake_done, absl::OkStatus()); + grpc_endpoint_get_peer(args->endpoint.get())); + InvokeOnHandshakeDone(args, std::move(on_handshake_done), absl::OkStatus()); } void Shutdown(grpc_error_handle /*why*/) override {} diff --git a/src/core/handshaker/handshaker.cc b/src/core/handshaker/handshaker.cc index 4279755cb6d0b..13ad93750533f 100644 --- a/src/core/handshaker/handshaker.cc +++ b/src/core/handshaker/handshaker.cc @@ -23,8 +23,10 @@ #include #include +#include "absl/functional/any_invocable.h" #include "absl/log/check.h" #include "absl/status/status.h" +#include "absl/status/statusor.h" #include "absl/strings/str_format.h" #include @@ -38,23 +40,37 @@ #include "src/core/lib/debug/trace.h" #include "src/core/lib/gprpp/debug_location.h" #include "src/core/lib/gprpp/status_helper.h" +#include "src/core/lib/gprpp/time.h" +#include "src/core/lib/iomgr/endpoint.h" +#include "src/core/lib/iomgr/error.h" #include "src/core/lib/iomgr/event_engine_shims/endpoint.h" #include "src/core/lib/iomgr/exec_ctx.h" +using ::grpc_event_engine::experimental::EventEngine; + namespace grpc_core { -namespace { +void Handshaker::InvokeOnHandshakeDone( + HandshakerArgs* args, + absl::AnyInvocable on_handshake_done, + absl::Status status) { + args->event_engine->Run([on_handshake_done = std::move(on_handshake_done), + status = std::move(status)]() mutable { + ApplicationCallbackExecCtx callback_exec_ctx; + ExecCtx exec_ctx; + on_handshake_done(std::move(status)); + // Destroy callback while ExecCtx is still in scope. + on_handshake_done = nullptr; + }); +} -using ::grpc_event_engine::experimental::EventEngine; +namespace { std::string HandshakerArgsString(HandshakerArgs* args) { - size_t read_buffer_length = - args->read_buffer != nullptr ? args->read_buffer->length : 0; - return absl::StrFormat( - "{endpoint=%p, args=%s, read_buffer=%p (length=%" PRIuPTR - "), exit_early=%d}", - args->endpoint, args->args.ToString(), args->read_buffer, - read_buffer_length, args->exit_early); + return absl::StrFormat("{endpoint=%p, args=%s, read_buffer.Length()=%" PRIuPTR + ", exit_early=%d}", + args->endpoint.get(), args->args.ToString(), + args->read_buffer.Length(), args->exit_early); } } // namespace @@ -69,155 +85,129 @@ void HandshakeManager::Add(RefCountedPtr handshaker) { gpr_log( GPR_INFO, "handshake_manager %p: adding handshaker %s [%p] at index %" PRIuPTR, - this, handshaker->name(), handshaker.get(), handshakers_.size()); + this, std::string(handshaker->name()).c_str(), handshaker.get(), + handshakers_.size()); } handshakers_.push_back(std::move(handshaker)); } -HandshakeManager::~HandshakeManager() { handshakers_.clear(); } +void HandshakeManager::DoHandshake( + OrphanablePtr endpoint, const ChannelArgs& channel_args, + Timestamp deadline, grpc_tcp_server_acceptor* acceptor, + absl::AnyInvocable)> + on_handshake_done) { + MutexLock lock(&mu_); + CHECK_EQ(index_, 0u); + on_handshake_done_ = std::move(on_handshake_done); + // Construct handshaker args. These will be passed through all + // handshakers and eventually be freed by the on_handshake_done callback. + args_.endpoint = std::move(endpoint); + args_.deadline = deadline; + args_.args = channel_args; + args_.event_engine = args_.args.GetObject(); + args_.acceptor = acceptor; + if (acceptor != nullptr && acceptor->external_connection && + acceptor->pending_data != nullptr) { + grpc_slice_buffer_swap(args_.read_buffer.c_slice_buffer(), + &(acceptor->pending_data->data.raw.slice_buffer)); + // TODO(vigneshbabu): For connections accepted through event engine + // listeners, the ownership of the byte buffer received is transferred to + // this callback and it is thus this callback's duty to delete it. + // Make this hack default once event engine is rolled out. + if (grpc_event_engine::experimental::grpc_is_event_engine_endpoint( + args_.endpoint.get())) { + grpc_byte_buffer_destroy(acceptor->pending_data); + } + } + // Start deadline timer, which owns a ref. + const Duration time_to_deadline = deadline - Timestamp::Now(); + deadline_timer_handle_ = + args_.event_engine->RunAfter(time_to_deadline, [self = Ref()]() mutable { + ApplicationCallbackExecCtx callback_exec_ctx; + ExecCtx exec_ctx; + self->Shutdown(GRPC_ERROR_CREATE("Handshake timed out")); + // HandshakeManager deletion might require an active ExecCtx. + self.reset(); + }); + // Start first handshaker. + CallNextHandshakerLocked(absl::OkStatus()); +} -void HandshakeManager::Shutdown(grpc_error_handle why) { - { - MutexLock lock(&mu_); +void HandshakeManager::Shutdown(absl::Status error) { + MutexLock lock(&mu_); + if (!is_shutdown_) { + if (GRPC_TRACE_FLAG_ENABLED(handshaker)) { + gpr_log(GPR_INFO, "handshake_manager %p: Shutdown() called: %s", this, + error.ToString().c_str()); + } + is_shutdown_ = true; // Shutdown the handshaker that's currently in progress, if any. - if (!is_shutdown_ && index_ > 0) { - is_shutdown_ = true; - handshakers_[index_ - 1]->Shutdown(why); + if (index_ > 0) { + if (GRPC_TRACE_FLAG_ENABLED(handshaker)) { + gpr_log( + GPR_INFO, + "handshake_manager %p: shutting down handshaker at index %" PRIuPTR, + this, index_ - 1); + } + handshakers_[index_ - 1]->Shutdown(std::move(error)); } } } -// Helper function to call either the next handshaker or the -// on_handshake_done callback. -// Returns true if we've scheduled the on_handshake_done callback. -bool HandshakeManager::CallNextHandshakerLocked(grpc_error_handle error) { +void HandshakeManager::CallNextHandshakerLocked(absl::Status error) { if (GRPC_TRACE_FLAG_ENABLED(handshaker)) { gpr_log(GPR_INFO, "handshake_manager %p: error=%s shutdown=%d index=%" PRIuPTR ", args=%s", - this, StatusToString(error).c_str(), is_shutdown_, index_, + this, error.ToString().c_str(), is_shutdown_, index_, HandshakerArgsString(&args_).c_str()); } CHECK(index_ <= handshakers_.size()); // If we got an error or we've been shut down or we're exiting early or // we've finished the last handshaker, invoke the on_handshake_done - // callback. Otherwise, call the next handshaker. + // callback. if (!error.ok() || is_shutdown_ || args_.exit_early || index_ == handshakers_.size()) { if (error.ok() && is_shutdown_) { error = GRPC_ERROR_CREATE("handshaker shutdown"); - // It is possible that the endpoint has already been destroyed by - // a shutdown call while this callback was sitting on the ExecCtx - // with no error. - if (args_.endpoint != nullptr) { - grpc_endpoint_destroy(args_.endpoint); - args_.endpoint = nullptr; - } - if (args_.read_buffer != nullptr) { - grpc_slice_buffer_destroy(args_.read_buffer); - gpr_free(args_.read_buffer); - args_.read_buffer = nullptr; - } - args_.args = ChannelArgs(); + args_.endpoint.reset(); } if (GRPC_TRACE_FLAG_ENABLED(handshaker)) { gpr_log(GPR_INFO, "handshake_manager %p: handshaking complete -- scheduling " "on_handshake_done with error=%s", - this, StatusToString(error).c_str()); + this, error.ToString().c_str()); } // Cancel deadline timer, since we're invoking the on_handshake_done // callback now. - event_engine_->Cancel(deadline_timer_handle_); - ExecCtx::Run(DEBUG_LOCATION, &on_handshake_done_, error); + args_.event_engine->Cancel(deadline_timer_handle_); is_shutdown_ = true; - } else { - auto handshaker = handshakers_[index_]; - if (GRPC_TRACE_FLAG_ENABLED(handshaker)) { - gpr_log( - GPR_INFO, - "handshake_manager %p: calling handshaker %s [%p] at index %" PRIuPTR, - this, handshaker->name(), handshaker.get(), index_); - } - handshaker->DoHandshake(acceptor_, &call_next_handshaker_, &args_); - } - ++index_; - return is_shutdown_; -} - -void HandshakeManager::CallNextHandshakerFn(void* arg, - grpc_error_handle error) { - auto* mgr = static_cast(arg); - bool done; - { - MutexLock lock(&mgr->mu_); - done = mgr->CallNextHandshakerLocked(error); - } - // If we're invoked the final callback, we won't be coming back - // to this function, so we can release our reference to the - // handshake manager. - if (done) { - mgr->Unref(); - } -} - -void HandshakeManager::DoHandshake(grpc_endpoint* endpoint, - const ChannelArgs& channel_args, - Timestamp deadline, - grpc_tcp_server_acceptor* acceptor, - grpc_iomgr_cb_func on_handshake_done, - void* user_data) { - bool done; - { - MutexLock lock(&mu_); - CHECK_EQ(index_, 0u); - // Construct handshaker args. These will be passed through all - // handshakers and eventually be freed by the on_handshake_done callback. - args_.endpoint = endpoint; - args_.deadline = deadline; - args_.args = channel_args; - args_.user_data = user_data; - args_.read_buffer = - static_cast(gpr_malloc(sizeof(*args_.read_buffer))); - grpc_slice_buffer_init(args_.read_buffer); - if (acceptor != nullptr && acceptor->external_connection && - acceptor->pending_data != nullptr) { - grpc_slice_buffer_swap(args_.read_buffer, - &(acceptor->pending_data->data.raw.slice_buffer)); - // TODO(vigneshbabu): For connections accepted through event engine - // listeners, the ownership of the byte buffer received is transferred to - // this callback and it is thus this callback's duty to delete it. - // Make this hack default once event engine is rolled out. - if (grpc_event_engine::experimental::grpc_is_event_engine_endpoint( - endpoint)) { - grpc_byte_buffer_destroy(acceptor->pending_data); - } - } - // Initialize state needed for calling handshakers. - acceptor_ = acceptor; - GRPC_CLOSURE_INIT(&call_next_handshaker_, - &HandshakeManager::CallNextHandshakerFn, this, - grpc_schedule_on_exec_ctx); - GRPC_CLOSURE_INIT(&on_handshake_done_, on_handshake_done, &args_, - grpc_schedule_on_exec_ctx); - // Start deadline timer, which owns a ref. - const Duration time_to_deadline = deadline - Timestamp::Now(); - event_engine_ = args_.args.GetObjectRef(); - deadline_timer_handle_ = - event_engine_->RunAfter(time_to_deadline, [self = Ref()]() mutable { - ApplicationCallbackExecCtx callback_exec_ctx; - ExecCtx exec_ctx; - self->Shutdown(GRPC_ERROR_CREATE("Handshake timed out")); - // HandshakeManager deletion might require an active ExecCtx. - self.reset(); - }); - // Start first handshaker, which also owns a ref. - Ref().release(); - done = CallNextHandshakerLocked(absl::OkStatus()); + absl::StatusOr result(&args_); + if (!error.ok()) result = std::move(error); + args_.event_engine->Run([on_handshake_done = std::move(on_handshake_done_), + result = std::move(result)]() mutable { + ApplicationCallbackExecCtx callback_exec_ctx; + ExecCtx exec_ctx; + on_handshake_done(std::move(result)); + // Destroy callback while ExecCtx is still in scope. + on_handshake_done = nullptr; + }); + return; } - if (done) { - Unref(); + // Call the next handshaker. + auto handshaker = handshakers_[index_]; + if (GRPC_TRACE_FLAG_ENABLED(handshaker)) { + gpr_log( + GPR_INFO, + "handshake_manager %p: calling handshaker %s [%p] at index %" PRIuPTR, + this, std::string(handshaker->name()).c_str(), handshaker.get(), + index_); } + ++index_; + handshaker->DoHandshake(&args_, [self = Ref()](absl::Status error) mutable { + MutexLock lock(&self->mu_); + self->CallNextHandshakerLocked(std::move(error)); + }); } } // namespace grpc_core diff --git a/src/core/handshaker/handshaker.h b/src/core/handshaker/handshaker.h index f5df3824081c6..04beed4a96612 100644 --- a/src/core/handshaker/handshaker.h +++ b/src/core/handshaker/handshaker.h @@ -31,6 +31,7 @@ #include #include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/gprpp/sync.h" @@ -39,6 +40,7 @@ #include "src/core/lib/iomgr/endpoint.h" #include "src/core/lib/iomgr/error.h" #include "src/core/lib/iomgr/tcp_server.h" +#include "src/core/lib/slice/slice_buffer.h" namespace grpc_core { @@ -49,34 +51,35 @@ namespace grpc_core { /// /// In general, handshakers should be used via a handshake manager. -/// Arguments passed through handshakers and to the on_handshake_done callback. +/// Arguments passed through handshakers and back to the caller. /// /// For handshakers, all members are input/output parameters; for /// example, a handshaker may read from or write to \a endpoint and /// then later replace it with a wrapped endpoint. Similarly, a /// handshaker may modify \a args. /// -/// A handshaker takes ownership of the members while a handshake is in -/// progress. Upon failure or shutdown of an in-progress handshaker, -/// the handshaker is responsible for destroying the members and setting -/// them to NULL before invoking the on_handshake_done callback. -/// -/// For the on_handshake_done callback, all members are input arguments, -/// which the callback takes ownership of. +/// A handshaker takes ownership of the members when this struct is +/// passed to DoHandshake(). It passes ownership back to the caller +/// when it invokes on_handshake_done. struct HandshakerArgs { - grpc_endpoint* endpoint = nullptr; + OrphanablePtr endpoint; ChannelArgs args; - grpc_slice_buffer* read_buffer = nullptr; + // Any bytes read from the endpoint that are not consumed by the + // handshaker must be passed back via this buffer. + SliceBuffer read_buffer; // A handshaker may set this to true before invoking on_handshake_done // to indicate that subsequent handshakers should be skipped. bool exit_early = false; - // User data passed through the handshake manager. Not used by - // individual handshakers. - void* user_data = nullptr; + // EventEngine to use for async work. + // (This is just a convenience to avoid digging it out of args.) + grpc_event_engine::experimental::EventEngine* event_engine = nullptr; // Deadline associated with the handshake. // TODO(anramach): Move this out of handshake args after EventEngine // is the default. Timestamp deadline; + // TODO(roth): Make this go away somehow as part of the EventEngine + // migration? + grpc_tcp_server_acceptor* acceptor = nullptr; }; /// @@ -86,11 +89,23 @@ struct HandshakerArgs { class Handshaker : public RefCounted { public: ~Handshaker() override = default; - virtual void Shutdown(grpc_error_handle why) = 0; - virtual void DoHandshake(grpc_tcp_server_acceptor* acceptor, - grpc_closure* on_handshake_done, - HandshakerArgs* args) = 0; - virtual const char* name() const = 0; + virtual absl::string_view name() const = 0; + virtual void DoHandshake( + HandshakerArgs* args, + absl::AnyInvocable on_handshake_done) = 0; + virtual void Shutdown(absl::Status error) = 0; + + protected: + // Helper function to safely invoke on_handshake_done asynchronously. + // + // Note that on_handshake_done may complete in another thread as soon + // as this method returns, so the handshaker object may be destroyed + // by the callback unless the caller of this method is holding its own + // ref to the handshaker. + static void InvokeOnHandshakeDone( + HandshakerArgs* args, + absl::AnyInvocable on_handshake_done, + absl::Status status); }; // @@ -100,16 +115,11 @@ class Handshaker : public RefCounted { class HandshakeManager : public RefCounted { public: HandshakeManager(); - ~HandshakeManager() override; /// Adds a handshaker to the handshake manager. /// Takes ownership of \a handshaker. void Add(RefCountedPtr handshaker) ABSL_LOCKS_EXCLUDED(mu_); - /// Shuts down the handshake manager (e.g., to clean up when the operation is - /// aborted in the middle). - void Shutdown(grpc_error_handle why) ABSL_LOCKS_EXCLUDED(mu_); - /// Invokes handshakers in the order they were added. /// Takes ownership of \a endpoint, and then passes that ownership to /// the \a on_handshake_done callback. @@ -122,41 +132,39 @@ class HandshakeManager : public RefCounted { /// absl::OkStatus(), then handshaking failed and the handshaker has done /// the necessary clean-up. Otherwise, the callback takes ownership of /// the arguments. - void DoHandshake(grpc_endpoint* endpoint, const ChannelArgs& channel_args, - Timestamp deadline, grpc_tcp_server_acceptor* acceptor, - grpc_iomgr_cb_func on_handshake_done, void* user_data) - ABSL_LOCKS_EXCLUDED(mu_); + void DoHandshake(OrphanablePtr endpoint, + const ChannelArgs& channel_args, Timestamp deadline, + grpc_tcp_server_acceptor* acceptor, + absl::AnyInvocable)> + on_handshake_done) ABSL_LOCKS_EXCLUDED(mu_); - private: - bool CallNextHandshakerLocked(grpc_error_handle error) - ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_); + /// Shuts down the handshake manager (e.g., to clean up when the operation is + /// aborted in the middle). + void Shutdown(absl::Status error) ABSL_LOCKS_EXCLUDED(mu_); + private: // A function used as the handshaker-done callback when chaining // handshakers together. - static void CallNextHandshakerFn(void* arg, grpc_error_handle error) - ABSL_LOCKS_EXCLUDED(mu_); + void CallNextHandshakerLocked(absl::Status error) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_); - static const size_t HANDSHAKERS_INIT_SIZE = 2; + static const size_t kHandshakerListInlineSize = 2; Mutex mu_; bool is_shutdown_ ABSL_GUARDED_BY(mu_) = false; - // An array of handshakers added via grpc_handshake_manager_add(). - absl::InlinedVector, HANDSHAKERS_INIT_SIZE> - handshakers_ ABSL_GUARDED_BY(mu_); // The index of the handshaker to invoke next and closure to invoke it. size_t index_ ABSL_GUARDED_BY(mu_) = 0; - grpc_closure call_next_handshaker_ ABSL_GUARDED_BY(mu_); - // The acceptor to call the handshakers with. - grpc_tcp_server_acceptor* acceptor_ ABSL_GUARDED_BY(mu_); - // The final callback and user_data to invoke after the last handshaker. - grpc_closure on_handshake_done_ ABSL_GUARDED_BY(mu_); + // An array of handshakers added via Add(). + absl::InlinedVector, kHandshakerListInlineSize> + handshakers_ ABSL_GUARDED_BY(mu_); // Handshaker args. HandshakerArgs args_ ABSL_GUARDED_BY(mu_); + // The final callback to invoke after the last handshaker. + absl::AnyInvocable)> on_handshake_done_ + ABSL_GUARDED_BY(mu_); // Deadline timer across all handshakers. grpc_event_engine::experimental::EventEngine::TaskHandle deadline_timer_handle_ ABSL_GUARDED_BY(mu_); - std::shared_ptr event_engine_ - ABSL_GUARDED_BY(mu_); }; } // namespace grpc_core diff --git a/src/core/handshaker/http_connect/http_connect_handshaker.cc b/src/core/handshaker/http_connect/http_connect_handshaker.cc index de3de3b843f02..ae9d926b491f9 100644 --- a/src/core/handshaker/http_connect/http_connect_handshaker.cc +++ b/src/core/handshaker/http_connect/http_connect_handshaker.cc @@ -23,6 +23,7 @@ #include #include +#include #include "absl/base/thread_annotations.h" #include "absl/status/status.h" @@ -50,6 +51,8 @@ #include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/iomgr/iomgr_fwd.h" #include "src/core/lib/iomgr/tcp_server.h" +#include "src/core/lib/slice/slice.h" +#include "src/core/lib/slice/slice_buffer.h" #include "src/core/util/http_client/format_request.h" #include "src/core/util/http_client/parser.h" #include "src/core/util/string.h" @@ -61,165 +64,148 @@ namespace { class HttpConnectHandshaker : public Handshaker { public: HttpConnectHandshaker(); - void Shutdown(grpc_error_handle why) override; - void DoHandshake(grpc_tcp_server_acceptor* acceptor, - grpc_closure* on_handshake_done, - HandshakerArgs* args) override; - const char* name() const override { return "http_connect"; } + absl::string_view name() const override { return "http_connect"; } + void DoHandshake( + HandshakerArgs* args, + absl::AnyInvocable on_handshake_done) override; + void Shutdown(absl::Status error) override; private: ~HttpConnectHandshaker() override; - void CleanupArgsForFailureLocked() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_); - void HandshakeFailedLocked(grpc_error_handle error) + void HandshakeFailedLocked(absl::Status error) ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_); - static void OnWriteDone(void* arg, grpc_error_handle error); - static void OnReadDone(void* arg, grpc_error_handle error); + void FinishLocked(absl::Status error) ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_); + void OnWriteDone(absl::Status error); + void OnReadDone(absl::Status error); + bool OnReadDoneLocked(absl::Status error) ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_); static void OnWriteDoneScheduler(void* arg, grpc_error_handle error); static void OnReadDoneScheduler(void* arg, grpc_error_handle error); Mutex mu_; - bool is_shutdown_ ABSL_GUARDED_BY(mu_) = false; - // Read buffer to destroy after a shutdown. - grpc_slice_buffer* read_buffer_to_destroy_ ABSL_GUARDED_BY(mu_) = nullptr; - // State saved while performing the handshake. HandshakerArgs* args_ = nullptr; - grpc_closure* on_handshake_done_ = nullptr; + absl::AnyInvocable on_handshake_done_ + ABSL_GUARDED_BY(mu_); // Objects for processing the HTTP CONNECT request and response. - grpc_slice_buffer write_buffer_ ABSL_GUARDED_BY(mu_); - grpc_closure request_done_closure_ ABSL_GUARDED_BY(mu_); - grpc_closure response_read_closure_ ABSL_GUARDED_BY(mu_); + SliceBuffer write_buffer_ ABSL_GUARDED_BY(mu_); + grpc_closure on_write_done_scheduler_ ABSL_GUARDED_BY(mu_); + grpc_closure on_read_done_scheduler_ ABSL_GUARDED_BY(mu_); grpc_http_parser http_parser_ ABSL_GUARDED_BY(mu_); grpc_http_response http_response_ ABSL_GUARDED_BY(mu_); }; HttpConnectHandshaker::~HttpConnectHandshaker() { - if (read_buffer_to_destroy_ != nullptr) { - grpc_slice_buffer_destroy(read_buffer_to_destroy_); - gpr_free(read_buffer_to_destroy_); - } - grpc_slice_buffer_destroy(&write_buffer_); grpc_http_parser_destroy(&http_parser_); grpc_http_response_destroy(&http_response_); } -// Set args fields to nullptr, saving the endpoint and read buffer for -// later destruction. -void HttpConnectHandshaker::CleanupArgsForFailureLocked() { - read_buffer_to_destroy_ = args_->read_buffer; - args_->read_buffer = nullptr; - args_->args = ChannelArgs(); -} - // If the handshake failed or we're shutting down, clean up and invoke the // callback with the error. -void HttpConnectHandshaker::HandshakeFailedLocked(grpc_error_handle error) { +void HttpConnectHandshaker::HandshakeFailedLocked(absl::Status error) { if (error.ok()) { // If we were shut down after an endpoint operation succeeded but // before the endpoint callback was invoked, we need to generate our // own error. error = GRPC_ERROR_CREATE("Handshaker shutdown"); } - if (!is_shutdown_) { - // Not shutting down, so the handshake failed. Clean up before - // invoking the callback. - grpc_endpoint_destroy(args_->endpoint); - args_->endpoint = nullptr; - CleanupArgsForFailureLocked(); - // Set shutdown to true so that subsequent calls to - // http_connect_handshaker_shutdown() do nothing. - is_shutdown_ = true; - } // Invoke callback. - ExecCtx::Run(DEBUG_LOCATION, on_handshake_done_, error); + FinishLocked(std::move(error)); +} + +void HttpConnectHandshaker::FinishLocked(absl::Status error) { + InvokeOnHandshakeDone(args_, std::move(on_handshake_done_), std::move(error)); } // This callback can be invoked inline while already holding onto the mutex. To // avoid deadlocks, schedule OnWriteDone on ExecCtx. +// TODO(roth): This hop will no longer be needed when we migrate to the +// EventEngine endpoint API. void HttpConnectHandshaker::OnWriteDoneScheduler(void* arg, grpc_error_handle error) { auto* handshaker = static_cast(arg); - ExecCtx::Run(DEBUG_LOCATION, - GRPC_CLOSURE_INIT(&handshaker->request_done_closure_, - &HttpConnectHandshaker::OnWriteDone, - handshaker, grpc_schedule_on_exec_ctx), - error); + handshaker->args_->event_engine->Run( + [handshaker, error = std::move(error)]() mutable { + ApplicationCallbackExecCtx callback_exec_ctx; + ExecCtx exec_ctx; + handshaker->OnWriteDone(std::move(error)); + }); } // Callback invoked when finished writing HTTP CONNECT request. -void HttpConnectHandshaker::OnWriteDone(void* arg, grpc_error_handle error) { - auto* handshaker = static_cast(arg); - ReleasableMutexLock lock(&handshaker->mu_); - if (!error.ok() || handshaker->is_shutdown_) { +void HttpConnectHandshaker::OnWriteDone(absl::Status error) { + ReleasableMutexLock lock(&mu_); + if (!error.ok() || args_->endpoint == nullptr) { // If the write failed or we're shutting down, clean up and invoke the // callback with the error. - handshaker->HandshakeFailedLocked(error); + HandshakeFailedLocked(error); lock.Release(); - handshaker->Unref(); + Unref(); } else { // Otherwise, read the response. // The read callback inherits our ref to the handshaker. grpc_endpoint_read( - handshaker->args_->endpoint, handshaker->args_->read_buffer, - GRPC_CLOSURE_INIT(&handshaker->response_read_closure_, - &HttpConnectHandshaker::OnReadDoneScheduler, - handshaker, grpc_schedule_on_exec_ctx), + args_->endpoint.get(), args_->read_buffer.c_slice_buffer(), + GRPC_CLOSURE_INIT(&on_read_done_scheduler_, + &HttpConnectHandshaker::OnReadDoneScheduler, this, + grpc_schedule_on_exec_ctx), /*urgent=*/true, /*min_progress_size=*/1); } } // This callback can be invoked inline while already holding onto the mutex. To // avoid deadlocks, schedule OnReadDone on ExecCtx. +// TODO(roth): This hop will no longer be needed when we migrate to the +// EventEngine endpoint API. void HttpConnectHandshaker::OnReadDoneScheduler(void* arg, grpc_error_handle error) { auto* handshaker = static_cast(arg); - ExecCtx::Run(DEBUG_LOCATION, - GRPC_CLOSURE_INIT(&handshaker->response_read_closure_, - &HttpConnectHandshaker::OnReadDone, handshaker, - grpc_schedule_on_exec_ctx), - error); + handshaker->args_->event_engine->Run( + [handshaker, error = std::move(error)]() mutable { + ApplicationCallbackExecCtx callback_exec_ctx; + ExecCtx exec_ctx; + handshaker->OnReadDone(std::move(error)); + }); } // Callback invoked for reading HTTP CONNECT response. -void HttpConnectHandshaker::OnReadDone(void* arg, grpc_error_handle error) { - auto* handshaker = static_cast(arg); - ReleasableMutexLock lock(&handshaker->mu_); - if (!error.ok() || handshaker->is_shutdown_) { +void HttpConnectHandshaker::OnReadDone(absl::Status error) { + bool done; + { + MutexLock lock(&mu_); + done = OnReadDoneLocked(std::move(error)); + } + if (done) Unref(); +} + +bool HttpConnectHandshaker::OnReadDoneLocked(absl::Status error) { + if (!error.ok() || args_->endpoint == nullptr) { // If the read failed or we're shutting down, clean up and invoke the // callback with the error. - handshaker->HandshakeFailedLocked(error); - goto done; + HandshakeFailedLocked(std::move(error)); + return true; } // Add buffer to parser. - for (size_t i = 0; i < handshaker->args_->read_buffer->count; ++i) { - if (GRPC_SLICE_LENGTH(handshaker->args_->read_buffer->slices[i]) > 0) { + for (size_t i = 0; i < args_->read_buffer.Count(); ++i) { + Slice slice = args_->read_buffer.TakeFirst(); + if (slice.length() > 0) { size_t body_start_offset = 0; - error = grpc_http_parser_parse(&handshaker->http_parser_, - handshaker->args_->read_buffer->slices[i], + error = grpc_http_parser_parse(&http_parser_, slice.c_slice(), &body_start_offset); if (!error.ok()) { - handshaker->HandshakeFailedLocked(error); - goto done; + HandshakeFailedLocked(std::move(error)); + return true; } - if (handshaker->http_parser_.state == GRPC_HTTP_BODY) { + if (http_parser_.state == GRPC_HTTP_BODY) { // Remove the data we've already read from the read buffer, // leaving only the leftover bytes (if any). - grpc_slice_buffer tmp_buffer; - grpc_slice_buffer_init(&tmp_buffer); - if (body_start_offset < - GRPC_SLICE_LENGTH(handshaker->args_->read_buffer->slices[i])) { - grpc_slice_buffer_add( - &tmp_buffer, - grpc_slice_split_tail(&handshaker->args_->read_buffer->slices[i], - body_start_offset)); + SliceBuffer tmp_buffer; + if (body_start_offset < slice.length()) { + tmp_buffer.Append(slice.Split(body_start_offset)); } - grpc_slice_buffer_addn(&tmp_buffer, - &handshaker->args_->read_buffer->slices[i + 1], - handshaker->args_->read_buffer->count - i - 1); - grpc_slice_buffer_swap(handshaker->args_->read_buffer, &tmp_buffer); - grpc_slice_buffer_destroy(&tmp_buffer); + tmp_buffer.TakeAndAppend(args_->read_buffer); + tmp_buffer.Swap(&args_->read_buffer); break; } } @@ -235,65 +221,46 @@ void HttpConnectHandshaker::OnReadDone(void* arg, grpc_error_handle error) { // need to fix the HTTP parser to understand when the body is // complete (e.g., handling chunked transfer encoding or looking // at the Content-Length: header). - if (handshaker->http_parser_.state != GRPC_HTTP_BODY) { - grpc_slice_buffer_reset_and_unref(handshaker->args_->read_buffer); + if (http_parser_.state != GRPC_HTTP_BODY) { + args_->read_buffer.Clear(); grpc_endpoint_read( - handshaker->args_->endpoint, handshaker->args_->read_buffer, - GRPC_CLOSURE_INIT(&handshaker->response_read_closure_, - &HttpConnectHandshaker::OnReadDoneScheduler, - handshaker, grpc_schedule_on_exec_ctx), + args_->endpoint.get(), args_->read_buffer.c_slice_buffer(), + GRPC_CLOSURE_INIT(&on_read_done_scheduler_, + &HttpConnectHandshaker::OnReadDoneScheduler, this, + grpc_schedule_on_exec_ctx), /*urgent=*/true, /*min_progress_size=*/1); - return; + return false; } // Make sure we got a 2xx response. - if (handshaker->http_response_.status < 200 || - handshaker->http_response_.status >= 300) { + if (http_response_.status < 200 || http_response_.status >= 300) { error = GRPC_ERROR_CREATE(absl::StrCat("HTTP proxy returned response code ", - handshaker->http_response_.status)); - handshaker->HandshakeFailedLocked(error); - goto done; + http_response_.status)); + HandshakeFailedLocked(std::move(error)); + return true; } // Success. Invoke handshake-done callback. - ExecCtx::Run(DEBUG_LOCATION, handshaker->on_handshake_done_, error); -done: - // Set shutdown to true so that subsequent calls to - // http_connect_handshaker_shutdown() do nothing. - handshaker->is_shutdown_ = true; - lock.Release(); - handshaker->Unref(); + FinishLocked(absl::OkStatus()); + return true; } // // Public handshaker methods // -void HttpConnectHandshaker::Shutdown(grpc_error_handle /*why*/) { - { - MutexLock lock(&mu_); - if (!is_shutdown_) { - is_shutdown_ = true; - grpc_endpoint_destroy(args_->endpoint); - args_->endpoint = nullptr; - CleanupArgsForFailureLocked(); - } - } +void HttpConnectHandshaker::Shutdown(absl::Status /*error*/) { + MutexLock lock(&mu_); + if (on_handshake_done_ != nullptr) args_->endpoint.reset(); } -void HttpConnectHandshaker::DoHandshake(grpc_tcp_server_acceptor* /*acceptor*/, - grpc_closure* on_handshake_done, - HandshakerArgs* args) { +void HttpConnectHandshaker::DoHandshake( + HandshakerArgs* args, + absl::AnyInvocable on_handshake_done) { // Check for HTTP CONNECT channel arg. // If not found, invoke on_handshake_done without doing anything. absl::optional server_name = args->args.GetString(GRPC_ARG_HTTP_CONNECT_SERVER); if (!server_name.has_value()) { - // Set shutdown to true so that subsequent calls to - // http_connect_handshaker_shutdown() do nothing. - { - MutexLock lock(&mu_); - is_shutdown_ = true; - } - ExecCtx::Run(DEBUG_LOCATION, on_handshake_done, absl::OkStatus()); + InvokeOnHandshakeDone(args, std::move(on_handshake_done), absl::OkStatus()); return; } // Get headers from channel args. @@ -311,7 +278,6 @@ void HttpConnectHandshaker::DoHandshake(grpc_tcp_server_acceptor* /*acceptor*/, gpr_malloc(sizeof(grpc_http_header) * num_header_strings)); for (size_t i = 0; i < num_header_strings; ++i) { char* sep = strchr(header_strings[i], ':'); - if (sep == nullptr) { gpr_log(GPR_ERROR, "skipping unparseable HTTP CONNECT header: %s", header_strings[i]); @@ -326,9 +292,9 @@ void HttpConnectHandshaker::DoHandshake(grpc_tcp_server_acceptor* /*acceptor*/, // Save state in the handshaker object. MutexLock lock(&mu_); args_ = args; - on_handshake_done_ = on_handshake_done; + on_handshake_done_ = std::move(on_handshake_done); // Log connection via proxy. - std::string proxy_name(grpc_endpoint_get_peer(args->endpoint)); + std::string proxy_name(grpc_endpoint_get_peer(args->endpoint.get())); std::string server_name_string(*server_name); gpr_log(GPR_INFO, "Connecting to server %s via HTTP proxy %s", server_name_string.c_str(), proxy_name.c_str()); @@ -342,7 +308,7 @@ void HttpConnectHandshaker::DoHandshake(grpc_tcp_server_acceptor* /*acceptor*/, request.body = nullptr; grpc_slice request_slice = grpc_httpcli_format_connect_request( &request, server_name_string.c_str(), server_name_string.c_str()); - grpc_slice_buffer_add(&write_buffer_, request_slice); + write_buffer_.Append(Slice(request_slice)); // Clean up. gpr_free(headers); for (size_t i = 0; i < num_header_strings; ++i) { @@ -352,15 +318,14 @@ void HttpConnectHandshaker::DoHandshake(grpc_tcp_server_acceptor* /*acceptor*/, // Take a new ref to be held by the write callback. Ref().release(); grpc_endpoint_write( - args->endpoint, &write_buffer_, - GRPC_CLOSURE_INIT(&request_done_closure_, + args->endpoint.get(), write_buffer_.c_slice_buffer(), + GRPC_CLOSURE_INIT(&on_write_done_scheduler_, &HttpConnectHandshaker::OnWriteDoneScheduler, this, grpc_schedule_on_exec_ctx), nullptr, /*max_frame_size=*/INT_MAX); } HttpConnectHandshaker::HttpConnectHandshaker() { - grpc_slice_buffer_init(&write_buffer_); grpc_http_parser_init(&http_parser_, GRPC_HTTP_RESPONSE, &http_response_); } diff --git a/src/core/handshaker/security/secure_endpoint.cc b/src/core/handshaker/security/secure_endpoint.cc index 18fde152438c2..972b54d619f89 100644 --- a/src/core/handshaker/security/secure_endpoint.cc +++ b/src/core/handshaker/security/secure_endpoint.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include "absl/base/thread_annotations.h" #include "absl/log/check.h" @@ -43,9 +44,11 @@ #include "src/core/lib/debug/trace.h" #include "src/core/lib/gprpp/debug_location.h" +#include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/gprpp/sync.h" #include "src/core/lib/iomgr/closure.h" +#include "src/core/lib/iomgr/endpoint.h" #include "src/core/lib/iomgr/error.h" #include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/iomgr/iomgr_fwd.h" @@ -64,17 +67,18 @@ static void on_read(void* user_data, grpc_error_handle error); static void on_write(void* user_data, grpc_error_handle error); namespace { -struct secure_endpoint { - secure_endpoint(const grpc_endpoint_vtable* vtable, +struct secure_endpoint : public grpc_endpoint { + secure_endpoint(const grpc_endpoint_vtable* vtbl, tsi_frame_protector* protector, tsi_zero_copy_grpc_protector* zero_copy_protector, - grpc_endpoint* transport, grpc_slice* leftover_slices, + grpc_core::OrphanablePtr endpoint, + grpc_slice* leftover_slices, const grpc_channel_args* channel_args, size_t leftover_nslices) - : wrapped_ep(transport), + : wrapped_ep(std::move(endpoint)), protector(protector), zero_copy_protector(zero_copy_protector) { - base.vtable = vtable; + this->vtable = vtbl; gpr_mu_init(&protector_mu); GRPC_CLOSURE_INIT(&on_read, ::on_read, this, grpc_schedule_on_exec_ctx); GRPC_CLOSURE_INIT(&on_write, ::on_write, this, grpc_schedule_on_exec_ctx); @@ -117,8 +121,7 @@ struct secure_endpoint { gpr_mu_destroy(&protector_mu); } - grpc_endpoint base; - grpc_endpoint* wrapped_ep; + grpc_core::OrphanablePtr wrapped_ep; struct tsi_frame_protector* protector; struct tsi_zero_copy_grpc_protector* zero_copy_protector; gpr_mu protector_mu; @@ -365,8 +368,8 @@ static void endpoint_read(grpc_endpoint* secure_ep, grpc_slice_buffer* slices, return; } - grpc_endpoint_read(ep->wrapped_ep, &ep->source_buffer, &ep->on_read, urgent, - /*min_progress_size=*/ep->min_progress_size); + grpc_endpoint_read(ep->wrapped_ep.get(), &ep->source_buffer, &ep->on_read, + urgent, /*min_progress_size=*/ep->min_progress_size); } static void flush_write_staging_buffer(secure_endpoint* ep, uint8_t** cur, @@ -500,52 +503,52 @@ static void endpoint_write(grpc_endpoint* secure_ep, grpc_slice_buffer* slices, // output_buffer at any time until the write completes. SECURE_ENDPOINT_REF(ep, "write"); ep->write_cb = cb; - grpc_endpoint_write(ep->wrapped_ep, &ep->output_buffer, &ep->on_write, arg, - max_frame_size); + grpc_endpoint_write(ep->wrapped_ep.get(), &ep->output_buffer, &ep->on_write, + arg, max_frame_size); } static void endpoint_destroy(grpc_endpoint* secure_ep) { secure_endpoint* ep = reinterpret_cast(secure_ep); - grpc_endpoint_destroy(ep->wrapped_ep); + ep->wrapped_ep.reset(); SECURE_ENDPOINT_UNREF(ep, "destroy"); } static void endpoint_add_to_pollset(grpc_endpoint* secure_ep, grpc_pollset* pollset) { secure_endpoint* ep = reinterpret_cast(secure_ep); - grpc_endpoint_add_to_pollset(ep->wrapped_ep, pollset); + grpc_endpoint_add_to_pollset(ep->wrapped_ep.get(), pollset); } static void endpoint_add_to_pollset_set(grpc_endpoint* secure_ep, grpc_pollset_set* pollset_set) { secure_endpoint* ep = reinterpret_cast(secure_ep); - grpc_endpoint_add_to_pollset_set(ep->wrapped_ep, pollset_set); + grpc_endpoint_add_to_pollset_set(ep->wrapped_ep.get(), pollset_set); } static void endpoint_delete_from_pollset_set(grpc_endpoint* secure_ep, grpc_pollset_set* pollset_set) { secure_endpoint* ep = reinterpret_cast(secure_ep); - grpc_endpoint_delete_from_pollset_set(ep->wrapped_ep, pollset_set); + grpc_endpoint_delete_from_pollset_set(ep->wrapped_ep.get(), pollset_set); } static absl::string_view endpoint_get_peer(grpc_endpoint* secure_ep) { secure_endpoint* ep = reinterpret_cast(secure_ep); - return grpc_endpoint_get_peer(ep->wrapped_ep); + return grpc_endpoint_get_peer(ep->wrapped_ep.get()); } static absl::string_view endpoint_get_local_address(grpc_endpoint* secure_ep) { secure_endpoint* ep = reinterpret_cast(secure_ep); - return grpc_endpoint_get_local_address(ep->wrapped_ep); + return grpc_endpoint_get_local_address(ep->wrapped_ep.get()); } static int endpoint_get_fd(grpc_endpoint* secure_ep) { secure_endpoint* ep = reinterpret_cast(secure_ep); - return grpc_endpoint_get_fd(ep->wrapped_ep); + return grpc_endpoint_get_fd(ep->wrapped_ep.get()); } static bool endpoint_can_track_err(grpc_endpoint* secure_ep) { secure_endpoint* ep = reinterpret_cast(secure_ep); - return grpc_endpoint_can_track_err(ep->wrapped_ep); + return grpc_endpoint_can_track_err(ep->wrapped_ep.get()); } static const grpc_endpoint_vtable vtable = {endpoint_read, @@ -559,13 +562,13 @@ static const grpc_endpoint_vtable vtable = {endpoint_read, endpoint_get_fd, endpoint_can_track_err}; -grpc_endpoint* grpc_secure_endpoint_create( +grpc_core::OrphanablePtr grpc_secure_endpoint_create( struct tsi_frame_protector* protector, struct tsi_zero_copy_grpc_protector* zero_copy_protector, - grpc_endpoint* to_wrap, grpc_slice* leftover_slices, - const grpc_channel_args* channel_args, size_t leftover_nslices) { - secure_endpoint* ep = - new secure_endpoint(&vtable, protector, zero_copy_protector, to_wrap, - leftover_slices, channel_args, leftover_nslices); - return &ep->base; + grpc_core::OrphanablePtr to_wrap, + grpc_slice* leftover_slices, const grpc_channel_args* channel_args, + size_t leftover_nslices) { + return grpc_core::MakeOrphanable( + &vtable, protector, zero_copy_protector, std::move(to_wrap), + leftover_slices, channel_args, leftover_nslices); } diff --git a/src/core/handshaker/security/secure_endpoint.h b/src/core/handshaker/security/secure_endpoint.h index a9d6d2088c489..43add1a816b7a 100644 --- a/src/core/handshaker/security/secure_endpoint.h +++ b/src/core/handshaker/security/secure_endpoint.h @@ -26,15 +26,17 @@ #include #include "src/core/lib/debug/trace.h" +#include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/iomgr/endpoint.h" // Takes ownership of protector, zero_copy_protector, and to_wrap, and refs // leftover_slices. If zero_copy_protector is not NULL, protector will never be // used. -grpc_endpoint* grpc_secure_endpoint_create( +grpc_core::OrphanablePtr grpc_secure_endpoint_create( struct tsi_frame_protector* protector, struct tsi_zero_copy_grpc_protector* zero_copy_protector, - grpc_endpoint* to_wrap, grpc_slice* leftover_slices, - const grpc_channel_args* channel_args, size_t leftover_nslices); + grpc_core::OrphanablePtr to_wrap, + grpc_slice* leftover_slices, const grpc_channel_args* channel_args, + size_t leftover_nslices); #endif // GRPC_SRC_CORE_HANDSHAKER_SECURITY_SECURE_ENDPOINT_H diff --git a/src/core/handshaker/security/security_handshaker.cc b/src/core/handshaker/security/security_handshaker.cc index dba7f399e1e2d..58c9a16eaeebd 100644 --- a/src/core/handshaker/security/security_handshaker.cc +++ b/src/core/handshaker/security/security_handshaker.cc @@ -28,6 +28,7 @@ #include #include "absl/base/attributes.h" +#include "absl/functional/any_invocable.h" #include "absl/log/check.h" #include "absl/status/status.h" #include "absl/strings/str_cat.h" @@ -79,11 +80,11 @@ class SecurityHandshaker : public Handshaker { grpc_security_connector* connector, const ChannelArgs& args); ~SecurityHandshaker() override; - void Shutdown(grpc_error_handle why) override; - void DoHandshake(grpc_tcp_server_acceptor* acceptor, - grpc_closure* on_handshake_done, - HandshakerArgs* args) override; - const char* name() const override { return "security"; } + absl::string_view name() const override { return "security"; } + void DoHandshake( + HandshakerArgs* args, + absl::AnyInvocable on_handshake_done) override; + void Shutdown(absl::Status error) override; private: grpc_error_handle DoHandshakerNextLocked(const unsigned char* bytes_received, @@ -92,12 +93,11 @@ class SecurityHandshaker : public Handshaker { grpc_error_handle OnHandshakeNextDoneLocked( tsi_result result, const unsigned char* bytes_to_send, size_t bytes_to_send_size, tsi_handshaker_result* handshaker_result); - void HandshakeFailedLocked(grpc_error_handle error); - void CleanupArgsForFailureLocked(); + void HandshakeFailedLocked(absl::Status error); + void Finish(absl::Status status); - static void OnHandshakeDataReceivedFromPeerFn(void* arg, - grpc_error_handle error); - static void OnHandshakeDataSentToPeerFn(void* arg, grpc_error_handle error); + void OnHandshakeDataReceivedFromPeerFn(absl::Status error); + void OnHandshakeDataSentToPeerFn(absl::Status error); static void OnHandshakeDataReceivedFromPeerFnScheduler( void* arg, grpc_error_handle error); static void OnHandshakeDataSentToPeerFnScheduler(void* arg, @@ -117,16 +117,14 @@ class SecurityHandshaker : public Handshaker { Mutex mu_; bool is_shutdown_ = false; - // Read buffer to destroy after a shutdown. - grpc_slice_buffer* read_buffer_to_destroy_ = nullptr; // State saved while performing the handshake. HandshakerArgs* args_ = nullptr; - grpc_closure* on_handshake_done_ = nullptr; + absl::AnyInvocable on_handshake_done_; size_t handshake_buffer_size_; unsigned char* handshake_buffer_; - grpc_slice_buffer outgoing_; + SliceBuffer outgoing_; grpc_closure on_handshake_data_sent_to_peer_; grpc_closure on_handshake_data_received_from_peer_; grpc_closure on_peer_checked_; @@ -146,7 +144,6 @@ SecurityHandshaker::SecurityHandshaker(tsi_handshaker* handshaker, static_cast(gpr_malloc(handshake_buffer_size_))), max_frame_size_( std::max(0, args.GetInt(GRPC_ARG_TSI_MAX_FRAME_SIZE).value_or(0))) { - grpc_slice_buffer_init(&outgoing_); GRPC_CLOSURE_INIT(&on_peer_checked_, &SecurityHandshaker::OnPeerCheckedFn, this, grpc_schedule_on_exec_ctx); } @@ -154,45 +151,30 @@ SecurityHandshaker::SecurityHandshaker(tsi_handshaker* handshaker, SecurityHandshaker::~SecurityHandshaker() { tsi_handshaker_destroy(handshaker_); tsi_handshaker_result_destroy(handshaker_result_); - if (read_buffer_to_destroy_ != nullptr) { - grpc_slice_buffer_destroy(read_buffer_to_destroy_); - gpr_free(read_buffer_to_destroy_); - } gpr_free(handshake_buffer_); - grpc_slice_buffer_destroy(&outgoing_); auth_context_.reset(DEBUG_LOCATION, "handshake"); connector_.reset(DEBUG_LOCATION, "handshake"); } size_t SecurityHandshaker::MoveReadBufferIntoHandshakeBuffer() { - size_t bytes_in_read_buffer = args_->read_buffer->length; + size_t bytes_in_read_buffer = args_->read_buffer.Length(); if (handshake_buffer_size_ < bytes_in_read_buffer) { handshake_buffer_ = static_cast( gpr_realloc(handshake_buffer_, bytes_in_read_buffer)); handshake_buffer_size_ = bytes_in_read_buffer; } size_t offset = 0; - while (args_->read_buffer->count > 0) { - grpc_slice* next_slice = grpc_slice_buffer_peek_first(args_->read_buffer); - memcpy(handshake_buffer_ + offset, GRPC_SLICE_START_PTR(*next_slice), - GRPC_SLICE_LENGTH(*next_slice)); - offset += GRPC_SLICE_LENGTH(*next_slice); - grpc_slice_buffer_remove_first(args_->read_buffer); + while (args_->read_buffer.Count() > 0) { + Slice slice = args_->read_buffer.TakeFirst(); + memcpy(handshake_buffer_ + offset, slice.data(), slice.size()); + offset += slice.size(); } return bytes_in_read_buffer; } -// Set args_ fields to NULL, saving the endpoint and read buffer for -// later destruction. -void SecurityHandshaker::CleanupArgsForFailureLocked() { - read_buffer_to_destroy_ = args_->read_buffer; - args_->read_buffer = nullptr; - args_->args = ChannelArgs(); -} - // If the handshake failed or we're shutting down, clean up and invoke the // callback with the error. -void SecurityHandshaker::HandshakeFailedLocked(grpc_error_handle error) { +void SecurityHandshaker::HandshakeFailedLocked(absl::Status error) { if (error.ok()) { // If we were shut down after the handshake succeeded but before an // endpoint callback was invoked, we need to generate our own error. @@ -200,17 +182,17 @@ void SecurityHandshaker::HandshakeFailedLocked(grpc_error_handle error) { } if (!is_shutdown_) { tsi_handshaker_shutdown(handshaker_); - grpc_endpoint_destroy(args_->endpoint); - args_->endpoint = nullptr; - // Not shutting down, so the write failed. Clean up before - // invoking the callback. - CleanupArgsForFailureLocked(); // Set shutdown to true so that subsequent calls to // security_handshaker_shutdown() do nothing. is_shutdown_ = true; } // Invoke callback. - ExecCtx::Run(DEBUG_LOCATION, on_handshake_done_, error); + Finish(std::move(error)); +} + +void SecurityHandshaker::Finish(absl::Status status) { + InvokeOnHandshakeDone(args_, std::move(on_handshake_done_), + std::move(status)); } namespace { @@ -306,19 +288,18 @@ void SecurityHandshaker::OnPeerCheckedInner(grpc_error_handle error) { grpc_slice slice = grpc_slice_from_copied_buffer( reinterpret_cast(unused_bytes), unused_bytes_size); args_->endpoint = grpc_secure_endpoint_create( - protector, zero_copy_protector, args_->endpoint, &slice, + protector, zero_copy_protector, std::move(args_->endpoint), &slice, args_->args.ToC().get(), 1); CSliceUnref(slice); } else { args_->endpoint = grpc_secure_endpoint_create( - protector, zero_copy_protector, args_->endpoint, nullptr, + protector, zero_copy_protector, std::move(args_->endpoint), nullptr, args_->args.ToC().get(), 0); } } else if (unused_bytes_size > 0) { // Not wrapping the endpoint, so just pass along unused bytes. - grpc_slice slice = grpc_slice_from_copied_buffer( - reinterpret_cast(unused_bytes), unused_bytes_size); - grpc_slice_buffer_add(args_->read_buffer, slice); + args_->read_buffer.Append(Slice::FromCopiedBuffer( + reinterpret_cast(unused_bytes), unused_bytes_size)); } // Done with handshaker result. tsi_handshaker_result_destroy(handshaker_result_); @@ -329,11 +310,11 @@ void SecurityHandshaker::OnPeerCheckedInner(grpc_error_handle error) { args_->args = args_->args.SetObject( MakeChannelzSecurityFromAuthContext(auth_context_.get())); } - // Invoke callback. - ExecCtx::Run(DEBUG_LOCATION, on_handshake_done_, absl::OkStatus()); // Set shutdown to true so that subsequent calls to // security_handshaker_shutdown() do nothing. is_shutdown_ = true; + // Invoke callback. + Finish(absl::OkStatus()); } void SecurityHandshaker::OnPeerCheckedFn(void* arg, grpc_error_handle error) { @@ -349,8 +330,8 @@ grpc_error_handle SecurityHandshaker::CheckPeerLocked() { return GRPC_ERROR_CREATE(absl::StrCat("Peer extraction failed (", tsi_result_to_string(result), ")")); } - connector_->check_peer(peer, args_->endpoint, args_->args, &auth_context_, - &on_peer_checked_); + connector_->check_peer(peer, args_->endpoint.get(), args_->args, + &auth_context_, &on_peer_checked_); grpc_auth_property_iterator it = grpc_auth_context_find_properties_by_name( auth_context_.get(), GRPC_TRANSPORT_SECURITY_LEVEL_PROPERTY_NAME); const grpc_auth_property* prop = grpc_auth_property_iterator_next(&it); @@ -374,7 +355,7 @@ grpc_error_handle SecurityHandshaker::OnHandshakeNextDoneLocked( if (result == TSI_INCOMPLETE_DATA) { CHECK_EQ(bytes_to_send_size, 0u); grpc_endpoint_read( - args_->endpoint, args_->read_buffer, + args_->endpoint.get(), args_->read_buffer.c_slice_buffer(), GRPC_CLOSURE_INIT( &on_handshake_data_received_from_peer_, &SecurityHandshaker::OnHandshakeDataReceivedFromPeerFnScheduler, @@ -388,6 +369,8 @@ grpc_error_handle SecurityHandshaker::OnHandshakeNextDoneLocked( if (security_connector != nullptr) { connector_type = security_connector->type().name(); } + // TODO(roth): Get a better signal from the TSI layer as to what + // status code we should use here. return GRPC_ERROR_CREATE(absl::StrCat( connector_type, " handshake failed (", tsi_result_to_string(result), ")", (tsi_handshake_error_.empty() ? "" : ": "), tsi_handshake_error_)); @@ -399,12 +382,11 @@ grpc_error_handle SecurityHandshaker::OnHandshakeNextDoneLocked( } if (bytes_to_send_size > 0) { // Send data to peer, if needed. - grpc_slice to_send = grpc_slice_from_copied_buffer( - reinterpret_cast(bytes_to_send), bytes_to_send_size); - grpc_slice_buffer_reset_and_unref(&outgoing_); - grpc_slice_buffer_add(&outgoing_, to_send); + outgoing_.Clear(); + outgoing_.Append(Slice::FromCopiedBuffer( + reinterpret_cast(bytes_to_send), bytes_to_send_size)); grpc_endpoint_write( - args_->endpoint, &outgoing_, + args_->endpoint.get(), outgoing_.c_slice_buffer(), GRPC_CLOSURE_INIT( &on_handshake_data_sent_to_peer_, &SecurityHandshaker::OnHandshakeDataSentToPeerFnScheduler, this, @@ -413,7 +395,7 @@ grpc_error_handle SecurityHandshaker::OnHandshakeNextDoneLocked( } else if (handshaker_result == nullptr) { // There is nothing to send, but need to read from peer. grpc_endpoint_read( - args_->endpoint, args_->read_buffer, + args_->endpoint.get(), args_->read_buffer.c_slice_buffer(), GRPC_CLOSURE_INIT( &on_handshake_data_received_from_peer_, &SecurityHandshaker::OnHandshakeDataReceivedFromPeerFnScheduler, @@ -435,7 +417,7 @@ void SecurityHandshaker::OnHandshakeNextDoneGrpcWrapper( grpc_error_handle error = h->OnHandshakeNextDoneLocked( result, bytes_to_send, bytes_to_send_size, handshaker_result); if (!error.ok()) { - h->HandshakeFailedLocked(error); + h->HandshakeFailedLocked(std::move(error)); } else { h.release(); // Avoid unref } @@ -463,102 +445,102 @@ grpc_error_handle SecurityHandshaker::DoHandshakerNextLocked( } // This callback might be run inline while we are still holding on to the mutex, -// so schedule OnHandshakeDataReceivedFromPeerFn on ExecCtx to avoid a deadlock. +// so run OnHandshakeDataReceivedFromPeerFn asynchronously to avoid a deadlock. +// TODO(roth): This will no longer be necessary once we migrate to the +// EventEngine endpoint API. void SecurityHandshaker::OnHandshakeDataReceivedFromPeerFnScheduler( void* arg, grpc_error_handle error) { - SecurityHandshaker* h = static_cast(arg); - ExecCtx::Run( - DEBUG_LOCATION, - GRPC_CLOSURE_INIT(&h->on_handshake_data_received_from_peer_, - &SecurityHandshaker::OnHandshakeDataReceivedFromPeerFn, - h, grpc_schedule_on_exec_ctx), - error); + SecurityHandshaker* handshaker = static_cast(arg); + handshaker->args_->event_engine->Run( + [handshaker, error = std::move(error)]() mutable { + ApplicationCallbackExecCtx callback_exec_ctx; + ExecCtx exec_ctx; + handshaker->OnHandshakeDataReceivedFromPeerFn(std::move(error)); + }); } -void SecurityHandshaker::OnHandshakeDataReceivedFromPeerFn( - void* arg, grpc_error_handle error) { - RefCountedPtr h(static_cast(arg)); - MutexLock lock(&h->mu_); - if (!error.ok() || h->is_shutdown_) { - h->HandshakeFailedLocked( +void SecurityHandshaker::OnHandshakeDataReceivedFromPeerFn(absl::Status error) { + RefCountedPtr handshaker(this); + MutexLock lock(&mu_); + if (!error.ok() || is_shutdown_) { + HandshakeFailedLocked( GRPC_ERROR_CREATE_REFERENCING("Handshake read failed", &error, 1)); return; } // Copy all slices received. - size_t bytes_received_size = h->MoveReadBufferIntoHandshakeBuffer(); + size_t bytes_received_size = MoveReadBufferIntoHandshakeBuffer(); // Call TSI handshaker. - error = h->DoHandshakerNextLocked(h->handshake_buffer_, bytes_received_size); + error = DoHandshakerNextLocked(handshake_buffer_, bytes_received_size); if (!error.ok()) { - h->HandshakeFailedLocked(error); + HandshakeFailedLocked(std::move(error)); } else { - h.release(); // Avoid unref + handshaker.release(); // Avoid unref } } // This callback might be run inline while we are still holding on to the mutex, -// so schedule OnHandshakeDataSentToPeerFn on ExecCtx to avoid a deadlock. +// so run OnHandshakeDataSentToPeerFn asynchronously to avoid a deadlock. +// TODO(roth): This will no longer be necessary once we migrate to the +// EventEngine endpoint API. void SecurityHandshaker::OnHandshakeDataSentToPeerFnScheduler( void* arg, grpc_error_handle error) { - SecurityHandshaker* h = static_cast(arg); - ExecCtx::Run( - DEBUG_LOCATION, - GRPC_CLOSURE_INIT(&h->on_handshake_data_sent_to_peer_, - &SecurityHandshaker::OnHandshakeDataSentToPeerFn, h, - grpc_schedule_on_exec_ctx), - error); + SecurityHandshaker* handshaker = static_cast(arg); + handshaker->args_->event_engine->Run( + [handshaker, error = std::move(error)]() mutable { + ApplicationCallbackExecCtx callback_exec_ctx; + ExecCtx exec_ctx; + handshaker->OnHandshakeDataSentToPeerFn(std::move(error)); + }); } -void SecurityHandshaker::OnHandshakeDataSentToPeerFn(void* arg, - grpc_error_handle error) { - RefCountedPtr h(static_cast(arg)); - MutexLock lock(&h->mu_); - if (!error.ok() || h->is_shutdown_) { - h->HandshakeFailedLocked( +void SecurityHandshaker::OnHandshakeDataSentToPeerFn(absl::Status error) { + RefCountedPtr handshaker(this); + MutexLock lock(&mu_); + if (!error.ok() || is_shutdown_) { + HandshakeFailedLocked( GRPC_ERROR_CREATE_REFERENCING("Handshake write failed", &error, 1)); return; } // We may be done. - if (h->handshaker_result_ == nullptr) { + if (handshaker_result_ == nullptr) { grpc_endpoint_read( - h->args_->endpoint, h->args_->read_buffer, + args_->endpoint.get(), args_->read_buffer.c_slice_buffer(), GRPC_CLOSURE_INIT( - &h->on_handshake_data_received_from_peer_, + &on_handshake_data_received_from_peer_, &SecurityHandshaker::OnHandshakeDataReceivedFromPeerFnScheduler, - h.get(), grpc_schedule_on_exec_ctx), + this, grpc_schedule_on_exec_ctx), /*urgent=*/true, /*min_progress_size=*/1); } else { - error = h->CheckPeerLocked(); + error = CheckPeerLocked(); if (!error.ok()) { - h->HandshakeFailedLocked(error); + HandshakeFailedLocked(error); return; } } - h.release(); // Avoid unref + handshaker.release(); // Avoid unref } // // public handshaker API // -void SecurityHandshaker::Shutdown(grpc_error_handle why) { +void SecurityHandshaker::Shutdown(grpc_error_handle error) { MutexLock lock(&mu_); if (!is_shutdown_) { is_shutdown_ = true; - connector_->cancel_check_peer(&on_peer_checked_, why); + connector_->cancel_check_peer(&on_peer_checked_, std::move(error)); tsi_handshaker_shutdown(handshaker_); - grpc_endpoint_destroy(args_->endpoint); - args_->endpoint = nullptr; - CleanupArgsForFailureLocked(); + args_->endpoint.reset(); } } -void SecurityHandshaker::DoHandshake(grpc_tcp_server_acceptor* /*acceptor*/, - grpc_closure* on_handshake_done, - HandshakerArgs* args) { +void SecurityHandshaker::DoHandshake( + HandshakerArgs* args, + absl::AnyInvocable on_handshake_done) { auto ref = Ref(); MutexLock lock(&mu_); args_ = args; - on_handshake_done_ = on_handshake_done; + on_handshake_done_ = std::move(on_handshake_done); size_t bytes_received_size = MoveReadBufferIntoHandshakeBuffer(); grpc_error_handle error = DoHandshakerNextLocked(handshake_buffer_, bytes_received_size); @@ -576,19 +558,13 @@ void SecurityHandshaker::DoHandshake(grpc_tcp_server_acceptor* /*acceptor*/, class FailHandshaker : public Handshaker { public: explicit FailHandshaker(absl::Status status) : status_(std::move(status)) {} - const char* name() const override { return "security_fail"; } - void Shutdown(grpc_error_handle /*why*/) override {} - void DoHandshake(grpc_tcp_server_acceptor* /*acceptor*/, - grpc_closure* on_handshake_done, - HandshakerArgs* args) override { - grpc_endpoint_destroy(args->endpoint); - args->endpoint = nullptr; - args->args = ChannelArgs(); - grpc_slice_buffer_destroy(args->read_buffer); - gpr_free(args->read_buffer); - args->read_buffer = nullptr; - ExecCtx::Run(DEBUG_LOCATION, on_handshake_done, status_); + absl::string_view name() const override { return "security_fail"; } + void DoHandshake( + HandshakerArgs* args, + absl::AnyInvocable on_handshake_done) override { + InvokeOnHandshakeDone(args, std::move(on_handshake_done), status_); } + void Shutdown(absl::Status /*error*/) override {} private: ~FailHandshaker() override = default; diff --git a/src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc b/src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc index fd557e47dc51d..7822a5cebeae0 100644 --- a/src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc +++ b/src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc @@ -19,8 +19,10 @@ #include "src/core/handshaker/tcp_connect/tcp_connect_handshaker.h" #include +#include #include "absl/base/thread_annotations.h" +#include "absl/functional/any_invocable.h" #include "absl/log/check.h" #include "absl/status/status.h" #include "absl/status/statusor.h" @@ -61,24 +63,23 @@ namespace { class TCPConnectHandshaker : public Handshaker { public: explicit TCPConnectHandshaker(grpc_pollset_set* pollset_set); - void Shutdown(grpc_error_handle why) override; - void DoHandshake(grpc_tcp_server_acceptor* /*acceptor*/, - grpc_closure* on_handshake_done, - HandshakerArgs* args) override; - const char* name() const override { return "tcp_connect"; } + absl::string_view name() const override { return "tcp_connect"; } + void DoHandshake( + HandshakerArgs* args, + absl::AnyInvocable on_handshake_done) override; + void Shutdown(absl::Status error) override; private: ~TCPConnectHandshaker() override; - void CleanupArgsForFailureLocked() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_); - void FinishLocked(grpc_error_handle error) ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_); + void FinishLocked(absl::Status error) ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_); static void Connected(void* arg, grpc_error_handle error); Mutex mu_; bool shutdown_ ABSL_GUARDED_BY(mu_) = false; - // Endpoint and read buffer to destroy after a shutdown. + // Endpoint to destroy after a shutdown. grpc_endpoint* endpoint_to_destroy_ ABSL_GUARDED_BY(mu_) = nullptr; - grpc_slice_buffer* read_buffer_to_destroy_ ABSL_GUARDED_BY(mu_) = nullptr; - grpc_closure* on_handshake_done_ ABSL_GUARDED_BY(mu_) = nullptr; + absl::AnyInvocable on_handshake_done_ + ABSL_GUARDED_BY(mu_); grpc_pollset_set* interested_parties_ = nullptr; grpc_polling_entity pollent_; HandshakerArgs* args_ = nullptr; @@ -99,33 +100,32 @@ TCPConnectHandshaker::TCPConnectHandshaker(grpc_pollset_set* pollset_set) GRPC_CLOSURE_INIT(&connected_, Connected, this, grpc_schedule_on_exec_ctx); } -void TCPConnectHandshaker::Shutdown(grpc_error_handle /*why*/) { +void TCPConnectHandshaker::Shutdown(absl::Status /*error*/) { // TODO(anramach): After migration to EventEngine, cancel the in-progress // TCP connection attempt. - { - MutexLock lock(&mu_); - if (!shutdown_) { - shutdown_ = true; - // If we are shutting down while connecting, respond back with - // handshake done. - // The callback from grpc_tcp_client_connect will perform - // the necessary clean up. - if (on_handshake_done_ != nullptr) { - CleanupArgsForFailureLocked(); - FinishLocked(GRPC_ERROR_CREATE("tcp handshaker shutdown")); - } + MutexLock lock(&mu_); + if (!shutdown_) { + shutdown_ = true; + // If we are shutting down while connecting, respond back with + // handshake done. + // The callback from grpc_tcp_client_connect will perform + // the necessary clean up. + if (on_handshake_done_ != nullptr) { + // TODO(roth): When we remove the legacy grpc_error APIs, propagate the + // status passed to shutdown as part of the message here. + FinishLocked(GRPC_ERROR_CREATE("tcp handshaker shutdown")); } } } -void TCPConnectHandshaker::DoHandshake(grpc_tcp_server_acceptor* /*acceptor*/, - grpc_closure* on_handshake_done, - HandshakerArgs* args) { +void TCPConnectHandshaker::DoHandshake( + HandshakerArgs* args, + absl::AnyInvocable on_handshake_done) { { MutexLock lock(&mu_); - on_handshake_done_ = on_handshake_done; + on_handshake_done_ = std::move(on_handshake_done); } - CHECK_EQ(args->endpoint, nullptr); + CHECK_EQ(args->endpoint.get(), nullptr); args_ = args; absl::StatusOr uri = URI::Parse( args->args.GetString(GRPC_ARG_TCP_HANDSHAKER_RESOLVED_ADDRESS).value()); @@ -149,7 +149,7 @@ void TCPConnectHandshaker::DoHandshake(grpc_tcp_server_acceptor* /*acceptor*/, Ref().release(); // Ref held by callback. // As we fake the TCP client connection failure when shutdown is called // we don't want to pass args->endpoint directly. - // Instead pass endpoint_ and swap this endpoint to + // Instead pass endpoint_to_destroy_ and swap this endpoint to // args endpoint on success. grpc_tcp_client_connect( &connected_, &endpoint_to_destroy_, interested_parties_, @@ -171,21 +171,19 @@ void TCPConnectHandshaker::Connected(void* arg, grpc_error_handle error) { self->endpoint_to_destroy_ = nullptr; } if (!self->shutdown_) { - self->CleanupArgsForFailureLocked(); self->shutdown_ = true; - self->FinishLocked(error); + self->FinishLocked(std::move(error)); } else { - // The on_handshake_done_ is already as part of shutdown when - // connecting So nothing to be done here other than unrefing the - // error. + // The on_handshake_done_ callback was already invoked as part of + // shutdown when connecting, so nothing to be done here. } return; } CHECK_NE(self->endpoint_to_destroy_, nullptr); - self->args_->endpoint = self->endpoint_to_destroy_; + self->args_->endpoint.reset(self->endpoint_to_destroy_); self->endpoint_to_destroy_ = nullptr; if (self->bind_endpoint_to_pollset_) { - grpc_endpoint_add_to_pollset_set(self->args_->endpoint, + grpc_endpoint_add_to_pollset_set(self->args_->endpoint.get(), self->interested_parties_); } self->FinishLocked(absl::OkStatus()); @@ -196,25 +194,14 @@ TCPConnectHandshaker::~TCPConnectHandshaker() { if (endpoint_to_destroy_ != nullptr) { grpc_endpoint_destroy(endpoint_to_destroy_); } - if (read_buffer_to_destroy_ != nullptr) { - grpc_slice_buffer_destroy(read_buffer_to_destroy_); - gpr_free(read_buffer_to_destroy_); - } grpc_pollset_set_destroy(interested_parties_); } -void TCPConnectHandshaker::CleanupArgsForFailureLocked() { - read_buffer_to_destroy_ = args_->read_buffer; - args_->read_buffer = nullptr; - args_->args = ChannelArgs(); -} - -void TCPConnectHandshaker::FinishLocked(grpc_error_handle error) { +void TCPConnectHandshaker::FinishLocked(absl::Status error) { if (interested_parties_ != nullptr) { grpc_polling_entity_del_from_pollset_set(&pollent_, interested_parties_); } - ExecCtx::Run(DEBUG_LOCATION, on_handshake_done_, error); - on_handshake_done_ = nullptr; + InvokeOnHandshakeDone(args_, std::move(on_handshake_done_), std::move(error)); } // diff --git a/src/core/lib/channel/channel_stack.cc b/src/core/lib/channel/channel_stack.cc index 63e322d6b93ab..36e3461700a95 100644 --- a/src/core/lib/channel/channel_stack.cc +++ b/src/core/lib/channel/channel_stack.cc @@ -262,7 +262,9 @@ void grpc_call_stack_destroy(grpc_call_stack* stack, void grpc_call_next_op(grpc_call_element* elem, grpc_transport_stream_op_batch* op) { grpc_call_element* next_elem = elem + 1; - GRPC_CALL_LOG_OP(GPR_INFO, next_elem, op); + GRPC_TRACE_LOG(channel, INFO) + << "OP[" << elem->filter->name << ":" << elem + << "]: " << grpc_transport_stream_op_batch_string(op, false); next_elem->filter->start_transport_stream_op_batch(next_elem, op); } @@ -292,10 +294,3 @@ grpc_call_stack* grpc_call_stack_from_top_element(grpc_call_element* elem) { void grpc_channel_stack_no_post_init(grpc_channel_stack*, grpc_channel_element*) {} - -void grpc_call_log_op(const char* file, int line, gpr_log_severity severity, - grpc_call_element* elem, - grpc_transport_stream_op_batch* op) { - gpr_log(file, line, severity, "OP[%s:%p]: %s", elem->filter->name, elem, - grpc_transport_stream_op_batch_string(op, false).c_str()); -} diff --git a/src/core/lib/channel/channel_stack.h b/src/core/lib/channel/channel_stack.h index ff0fc291c0a3c..f21c1dc0d8e19 100644 --- a/src/core/lib/channel/channel_stack.h +++ b/src/core/lib/channel/channel_stack.h @@ -63,6 +63,7 @@ #include "src/core/lib/gprpp/manual_constructor.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/gprpp/time.h" +#include "src/core/lib/gprpp/unique_type_name.h" #include "src/core/lib/iomgr/call_combiner.h" #include "src/core/lib/iomgr/closure.h" #include "src/core/lib/iomgr/error.h" @@ -159,7 +160,7 @@ struct grpc_channel_filter { const grpc_channel_info* channel_info); // The name of this filter - const char* name; + grpc_core::UniqueTypeName name; }; // A channel_element tracks its filter and the filter requested memory within // a channel allocation @@ -357,18 +358,7 @@ grpc_channel_stack* grpc_channel_stack_from_top_element( // Given the top element of a call stack, get the call stack itself grpc_call_stack* grpc_call_stack_from_top_element(grpc_call_element* elem); -void grpc_call_log_op(const char* file, int line, gpr_log_severity severity, - grpc_call_element* elem, - grpc_transport_stream_op_batch* op); - void grpc_channel_stack_no_post_init(grpc_channel_stack* stk, grpc_channel_element* elem); -#define GRPC_CALL_LOG_OP(sev, elem, op) \ - do { \ - if (GRPC_TRACE_FLAG_ENABLED(channel)) { \ - grpc_call_log_op(sev, elem, op); \ - } \ - } while (0) - #endif // GRPC_SRC_CORE_LIB_CHANNEL_CHANNEL_STACK_H diff --git a/src/core/lib/channel/connected_channel.cc b/src/core/lib/channel/connected_channel.cc index 5507e1ef65e0d..f24304bac89f8 100644 --- a/src/core/lib/channel/connected_channel.cc +++ b/src/core/lib/channel/connected_channel.cc @@ -267,7 +267,7 @@ const grpc_channel_filter kConnectedFilter{ }, connected_channel_destroy_channel_elem, connected_channel_get_channel_info, - "connected", + GRPC_UNIQUE_TYPE_NAME_HERE("connected"), }; // noop filter for the v3 stack: placeholder for now because other code requires @@ -288,7 +288,7 @@ const grpc_channel_filter kPromiseBasedTransportFilter = { +[](grpc_channel_stack*, grpc_channel_element*) {}, connected_channel_destroy_channel_elem, connected_channel_get_channel_info, - "connected", + GRPC_UNIQUE_TYPE_NAME_HERE("connected"), }; bool TransportSupportsClientPromiseBasedCalls(const ChannelArgs& args) { diff --git a/src/core/lib/channel/promise_based_filter.cc b/src/core/lib/channel/promise_based_filter.cc index e2d9d8cffff24..946be3b2af732 100644 --- a/src/core/lib/channel/promise_based_filter.cc +++ b/src/core/lib/channel/promise_based_filter.cc @@ -1316,7 +1316,7 @@ ClientCallData::~ClientCallData() { } std::string ClientCallData::DebugTag() const { - return absl::StrFormat("PBF_CLIENT[%p]: [%s] ", this, elem()->filter->name); + return absl::StrFormat("PBF_CLIENT[%p]: [%v] ", this, elem()->filter->name); } // Activity implementation. @@ -2012,7 +2012,7 @@ ServerCallData::~ServerCallData() { } std::string ServerCallData::DebugTag() const { - return absl::StrFormat("PBF_SERVER[%p]: [%s] ", this, elem()->filter->name); + return absl::StrFormat("PBF_SERVER[%p]: [%v] ", this, elem()->filter->name); } // Activity implementation. diff --git a/src/core/lib/channel/promise_based_filter.h b/src/core/lib/channel/promise_based_filter.h index c40b1fb096433..046b697644895 100644 --- a/src/core/lib/channel/promise_based_filter.h +++ b/src/core/lib/channel/promise_based_filter.h @@ -1659,7 +1659,7 @@ template absl::enable_if_t::value && !std::is_base_of::value, grpc_channel_filter> -MakePromiseBasedFilter(const char* name) { +MakePromiseBasedFilter() { using CallData = promise_filter_detail::CallData; return grpc_channel_filter{ @@ -1690,14 +1690,14 @@ MakePromiseBasedFilter(const char* name) { // get_channel_info promise_filter_detail::ChannelFilterMethods::GetChannelInfo, // name - name, + UniqueTypeNameFor(), }; } template absl::enable_if_t::value, grpc_channel_filter> -MakePromiseBasedFilter(const char* name) { +MakePromiseBasedFilter() { using CallData = promise_filter_detail::CallData; return grpc_channel_filter{ @@ -1728,7 +1728,7 @@ MakePromiseBasedFilter(const char* name) { // get_channel_info promise_filter_detail::ChannelFilterMethods::GetChannelInfo, // name - name, + UniqueTypeNameFor(), }; } diff --git a/src/core/lib/gprpp/unique_type_name.h b/src/core/lib/gprpp/unique_type_name.h index 2e71ab73f07bd..c60d8ea26fae7 100644 --- a/src/core/lib/gprpp/unique_type_name.h +++ b/src/core/lib/gprpp/unique_type_name.h @@ -64,7 +64,7 @@ class UniqueTypeName { Factory(const Factory&) = delete; Factory& operator=(const Factory&) = delete; - UniqueTypeName Create() { return UniqueTypeName(*name_); } + UniqueTypeName Create() const { return UniqueTypeName(*name_); } private: std::string* name_; @@ -93,12 +93,33 @@ class UniqueTypeName { absl::string_view name() const { return name_; } + template + friend void AbslStringify(Sink& sink, const UniqueTypeName& name) { + sink.Append(name.name_); + } + private: explicit UniqueTypeName(absl::string_view name) : name_(name) {} absl::string_view name_; }; +// Given a type with a member `static absl::string_view TypeName()`, returns a +// UniqueTypeName instance who's string value is the value of TypeName. +template +UniqueTypeName UniqueTypeNameFor() { + static UniqueTypeName::Factory factory(T::TypeName()); + return factory.Create(); +} + } // namespace grpc_core +// Creates a one-off UniqueTypeName in-place. +// Duplicate calls yield different UniqueTypeName instances. +#define GRPC_UNIQUE_TYPE_NAME_HERE(name) \ + ([] { \ + static const ::grpc_core::UniqueTypeName::Factory factory((name)); \ + return factory.Create(); \ + }()) + #endif // GRPC_SRC_CORE_LIB_GPRPP_UNIQUE_TYPE_NAME_H diff --git a/src/core/lib/iomgr/endpoint.h b/src/core/lib/iomgr/endpoint.h index a0f5d8429f5ae..c4b70abe4f3c4 100644 --- a/src/core/lib/iomgr/endpoint.h +++ b/src/core/lib/iomgr/endpoint.h @@ -101,6 +101,8 @@ bool grpc_endpoint_can_track_err(grpc_endpoint* ep); struct grpc_endpoint { const grpc_endpoint_vtable* vtable; + + void Orphan() { grpc_endpoint_destroy(this); } }; #endif // GRPC_SRC_CORE_LIB_IOMGR_ENDPOINT_H diff --git a/src/core/lib/security/authorization/grpc_server_authz_filter.cc b/src/core/lib/security/authorization/grpc_server_authz_filter.cc index 176f49fdb78c3..04733c341a291 100644 --- a/src/core/lib/security/authorization/grpc_server_authz_filter.cc +++ b/src/core/lib/security/authorization/grpc_server_authz_filter.cc @@ -114,7 +114,6 @@ absl::Status GrpcServerAuthzFilter::Call::OnClientInitialMetadata( } const grpc_channel_filter GrpcServerAuthzFilter::kFilter = - MakePromiseBasedFilter( - "grpc-server-authz"); + MakePromiseBasedFilter(); } // namespace grpc_core diff --git a/src/core/lib/security/authorization/grpc_server_authz_filter.h b/src/core/lib/security/authorization/grpc_server_authz_filter.h index 742b3979d88a1..cfd9c3bbd26cc 100644 --- a/src/core/lib/security/authorization/grpc_server_authz_filter.h +++ b/src/core/lib/security/authorization/grpc_server_authz_filter.h @@ -37,6 +37,8 @@ class GrpcServerAuthzFilter final public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "grpc-server-authz"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args); diff --git a/src/core/lib/security/transport/auth_filters.h b/src/core/lib/security/transport/auth_filters.h index 3970ae1e4f370..c3421f988761b 100644 --- a/src/core/lib/security/transport/auth_filters.h +++ b/src/core/lib/security/transport/auth_filters.h @@ -43,6 +43,8 @@ class ClientAuthFilter final : public ChannelFilter { public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "client-auth-filter"; } + ClientAuthFilter( RefCountedPtr security_connector, RefCountedPtr auth_context); @@ -96,6 +98,8 @@ class ServerAuthFilter final : public ImplementChannelFilter { public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "server-auth"; } + ServerAuthFilter(RefCountedPtr server_credentials, RefCountedPtr auth_context); diff --git a/src/core/lib/security/transport/client_auth_filter.cc b/src/core/lib/security/transport/client_auth_filter.cc index a3d25294e96fa..dcbeeccf14928 100644 --- a/src/core/lib/security/transport/client_auth_filter.cc +++ b/src/core/lib/security/transport/client_auth_filter.cc @@ -216,7 +216,6 @@ absl::StatusOr> ClientAuthFilter::Create( } const grpc_channel_filter ClientAuthFilter::kFilter = - MakePromiseBasedFilter( - "client-auth-filter"); + MakePromiseBasedFilter(); } // namespace grpc_core diff --git a/src/core/lib/security/transport/server_auth_filter.cc b/src/core/lib/security/transport/server_auth_filter.cc index 15a5e0d15fe03..3b44a84edc2a3 100644 --- a/src/core/lib/security/transport/server_auth_filter.cc +++ b/src/core/lib/security/transport/server_auth_filter.cc @@ -62,8 +62,7 @@ namespace grpc_core { const grpc_channel_filter ServerAuthFilter::kFilter = - MakePromiseBasedFilter( - "server-auth"); + MakePromiseBasedFilter(); const NoInterceptor ServerAuthFilter::Call::OnClientToServerMessage; const NoInterceptor ServerAuthFilter::Call::OnClientToServerHalfClose; diff --git a/src/core/lib/surface/channel_init.cc b/src/core/lib/surface/channel_init.cc index 4adb4241402d3..24cebf260e5ba 100644 --- a/src/core/lib/surface/channel_init.cc +++ b/src/core/lib/surface/channel_init.cc @@ -43,13 +43,15 @@ namespace grpc_core { -const char* (*NameFromChannelFilter)(const grpc_channel_filter*); +UniqueTypeName (*NameFromChannelFilter)(const grpc_channel_filter*); namespace { struct CompareChannelFiltersByName { bool operator()(const grpc_channel_filter* a, const grpc_channel_filter* b) const { - return strcmp(NameFromChannelFilter(a), NameFromChannelFilter(b)) < 0; + // Compare lexicographically instead of by pointer value so that different + // builds make the same choices. + return NameFromChannelFilter(a).name() < NameFromChannelFilter(b).name(); } }; } // namespace @@ -260,8 +262,8 @@ ChannelInit::StackConfig ChannelInit::BuildStackConfig( auto add_loc_str = [&max_loc_str_len, &loc_strs, &filter_to_registration, &max_filter_name_len]( const grpc_channel_filter* filter) { - max_filter_name_len = - std::max(strlen(NameFromChannelFilter(filter)), max_filter_name_len); + max_filter_name_len = std::max( + NameFromChannelFilter(filter).name().length(), max_filter_name_len); const auto registration = filter_to_registration[filter]->registration_source_; absl::string_view file = registration.file(); @@ -300,14 +302,16 @@ ChannelInit::StackConfig ChannelInit::BuildStackConfig( std::string after_str; if (dep_it != original.end() && !dep_it->second.empty()) { after_str = absl::StrCat( - std::string(max_filter_name_len + 1 - - strlen(NameFromChannelFilter(filter.filter)), - ' '), + std::string( + max_filter_name_len + 1 - + NameFromChannelFilter(filter.filter).name().length(), + ' '), "after ", absl::StrJoin( dep_it->second, ", ", [](std::string* out, const grpc_channel_filter* filter) { - out->append(NameFromChannelFilter(filter)); + out->append( + std::string(NameFromChannelFilter(filter).name())); })); } const auto filter_str = @@ -321,9 +325,10 @@ ChannelInit::StackConfig ChannelInit::BuildStackConfig( const auto filter_str = absl::StrCat( " ", loc_strs[terminal.filter], NameFromChannelFilter(terminal.filter), - std::string(max_filter_name_len + 1 - - strlen(NameFromChannelFilter(terminal.filter)), - ' '), + std::string( + max_filter_name_len + 1 - + NameFromChannelFilter(terminal.filter).name().length(), + ' '), "[terminal]"); LOG(INFO) << filter_str; } diff --git a/src/core/lib/surface/channel_init.h b/src/core/lib/surface/channel_init.h index c5067394014d5..e756160108aab 100644 --- a/src/core/lib/surface/channel_init.h +++ b/src/core/lib/surface/channel_init.h @@ -36,6 +36,7 @@ #include "src/core/lib/channel/channel_fwd.h" #include "src/core/lib/channel/channel_stack_builder.h" #include "src/core/lib/gprpp/debug_location.h" +#include "src/core/lib/gprpp/unique_type_name.h" #include "src/core/lib/surface/channel_stack_type.h" #include "src/core/lib/transport/call_filters.h" #include "src/core/lib/transport/interception_chain.h" @@ -61,7 +62,7 @@ namespace grpc_core { // TODO(ctiller): remove this. When we define a FilterFactory type, that type // can be specified with the right constraints to be depended upon by this code, // and that type can export a `string_view Name()` method. -extern const char* (*NameFromChannelFilter)(const grpc_channel_filter*); +extern UniqueTypeName (*NameFromChannelFilter)(const grpc_channel_filter*); class ChannelInit { public: diff --git a/src/core/lib/surface/filter_stack_call.cc b/src/core/lib/surface/filter_stack_call.cc index 40bc6ef3e9d10..e8aa35626500a 100644 --- a/src/core/lib/surface/filter_stack_call.cc +++ b/src/core/lib/surface/filter_stack_call.cc @@ -298,7 +298,9 @@ void FilterStackCall::ExecuteBatch(grpc_transport_stream_op_batch* batch, auto* call = static_cast(batch->handler_private.extra_arg); grpc_call_element* elem = call->call_elem(0); - GRPC_CALL_LOG_OP(GPR_INFO, elem, batch); + GRPC_TRACE_LOG(channel, INFO) + << "OP[" << elem->filter->name << ":" << elem + << "]: " << grpc_transport_stream_op_batch_string(batch, false); elem->filter->start_transport_stream_op_batch(elem, batch); }; batch->handler_private.extra_arg = this; diff --git a/src/core/lib/surface/lame_client.cc b/src/core/lib/surface/lame_client.cc index f573194a42ef7..5eaa6dc754704 100644 --- a/src/core/lib/surface/lame_client.cc +++ b/src/core/lib/surface/lame_client.cc @@ -57,7 +57,7 @@ namespace grpc_core { const grpc_channel_filter LameClientFilter::kFilter = MakePromiseBasedFilter("lame-client"); + kFilterIsLast>(); absl::StatusOr> LameClientFilter::Create( const ChannelArgs& args, ChannelFilter::Args) { diff --git a/src/core/lib/surface/lame_client.h b/src/core/lib/surface/lame_client.h index 1f8d323d35294..c5ff64d2a4da0 100644 --- a/src/core/lib/surface/lame_client.h +++ b/src/core/lib/surface/lame_client.h @@ -47,6 +47,8 @@ class LameClientFilter : public ChannelFilter { public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "lame-client"; } + explicit LameClientFilter(absl::Status error); static absl::StatusOr> Create( diff --git a/src/core/load_balancing/grpclb/client_load_reporting_filter.cc b/src/core/load_balancing/grpclb/client_load_reporting_filter.cc index e03fe8d70ce7a..bb86bdc1048c8 100644 --- a/src/core/load_balancing/grpclb/client_load_reporting_filter.cc +++ b/src/core/load_balancing/grpclb/client_load_reporting_filter.cc @@ -46,8 +46,7 @@ const NoInterceptor ClientLoadReportingFilter::Call::OnFinalize; const grpc_channel_filter ClientLoadReportingFilter::kFilter = MakePromiseBasedFilter( - "client_load_reporting"); + kFilterExaminesServerInitialMetadata>(); absl::StatusOr> ClientLoadReportingFilter::Create(const ChannelArgs&, ChannelFilter::Args) { diff --git a/src/core/load_balancing/grpclb/client_load_reporting_filter.h b/src/core/load_balancing/grpclb/client_load_reporting_filter.h index f8c5b7fbcc4a5..a9d979634ff10 100644 --- a/src/core/load_balancing/grpclb/client_load_reporting_filter.h +++ b/src/core/load_balancing/grpclb/client_load_reporting_filter.h @@ -36,6 +36,8 @@ class ClientLoadReportingFilter final public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "client_load_reporting"; } + class Call { public: void OnClientInitialMetadata(ClientMetadata& client_initial_metadata); diff --git a/src/core/resolver/xds/xds_resolver.cc b/src/core/resolver/xds/xds_resolver.cc index 286f4e0dedd16..a91708c67f458 100644 --- a/src/core/resolver/xds/xds_resolver.cc +++ b/src/core/resolver/xds/xds_resolver.cc @@ -318,6 +318,8 @@ class XdsResolver final : public Resolver { public: const static grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "cluster_selection_filter"; } + static absl::StatusOr> Create( const ChannelArgs& /* unused */, ChannelFilter::Args /* filter_args */) { @@ -866,8 +868,7 @@ XdsResolver::XdsRouteStateAttributeImpl::LockAndGetCluster( const grpc_channel_filter XdsResolver::ClusterSelectionFilter::kFilter = MakePromiseBasedFilter( - "cluster_selection_filter"); + kFilterExaminesServerInitialMetadata>(); void XdsResolver::ClusterSelectionFilter::Call::OnClientInitialMetadata( ClientMetadata&) { diff --git a/src/core/server/server.cc b/src/core/server/server.cc index c4796d6ae9b4d..6b1ead984dba6 100644 --- a/src/core/server/server.cc +++ b/src/core/server/server.cc @@ -779,7 +779,7 @@ const grpc_channel_filter Server::kServerTopFilter = { grpc_channel_stack_no_post_init, Server::ChannelData::DestroyChannelElement, grpc_channel_next_get_info, - "server", + GRPC_UNIQUE_TYPE_NAME_HERE("server"), }; namespace { diff --git a/src/core/server/server_call_tracer_filter.cc b/src/core/server/server_call_tracer_filter.cc index 1454a4f5e9286..78f8589cea075 100644 --- a/src/core/server/server_call_tracer_filter.cc +++ b/src/core/server/server_call_tracer_filter.cc @@ -49,6 +49,8 @@ class ServerCallTracerFilter public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "server_call_tracer"; } + static absl::StatusOr> Create( const ChannelArgs& /*args*/, ChannelFilter::Args /*filter_args*/); @@ -90,8 +92,7 @@ const NoInterceptor ServerCallTracerFilter::Call::OnServerToClientMessage; const grpc_channel_filter ServerCallTracerFilter::kFilter = MakePromiseBasedFilter( - "server_call_tracer"); + kFilterExaminesServerInitialMetadata>(); absl::StatusOr> ServerCallTracerFilter::Create(const ChannelArgs& /*args*/, diff --git a/src/core/server/server_config_selector_filter.cc b/src/core/server/server_config_selector_filter.cc index d04b10a0d861d..4dfe26fb5c5c9 100644 --- a/src/core/server/server_config_selector_filter.cc +++ b/src/core/server/server_config_selector_filter.cc @@ -55,6 +55,10 @@ class ServerConfigSelectorFilter final RefCountedPtr server_config_selector_provider); + static absl::string_view TypeName() { + return "server_config_selector_filter"; + } + ServerConfigSelectorFilter(const ServerConfigSelectorFilter&) = delete; ServerConfigSelectorFilter& operator=(const ServerConfigSelectorFilter&) = delete; @@ -164,7 +168,7 @@ const NoInterceptor ServerConfigSelectorFilter::Call::OnFinalize; } // namespace const grpc_channel_filter kServerConfigSelectorFilter = - MakePromiseBasedFilter( - "server_config_selector_filter"); + MakePromiseBasedFilter(); } // namespace grpc_core diff --git a/src/core/server/xds_channel_stack_modifier.cc b/src/core/server/xds_channel_stack_modifier.cc index f9e280ea28afe..15d2d3378f36e 100644 --- a/src/core/server/xds_channel_stack_modifier.cc +++ b/src/core/server/xds_channel_stack_modifier.cc @@ -63,7 +63,7 @@ void XdsChannelStackModifier::ModifyChannelStack(ChannelStackBuilder& builder) { for (auto it = builder.mutable_stack()->begin(); it != builder.mutable_stack()->end(); ++it) { for (absl::string_view predicate_name : {"server", "census_server"}) { - if (predicate_name == (*it)->name) insert_before = it + 1; + if (predicate_name == (*it)->name.name()) insert_before = it + 1; } } for (const grpc_channel_filter* filter : filters_) { diff --git a/src/core/service_config/service_config_channel_arg_filter.cc b/src/core/service_config/service_config_channel_arg_filter.cc index 8b06f26ebcf0f..96a5b3f8b81e3 100644 --- a/src/core/service_config/service_config_channel_arg_filter.cc +++ b/src/core/service_config/service_config_channel_arg_filter.cc @@ -58,6 +58,8 @@ class ServiceConfigChannelArgFilter final public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "service_config_channel_arg"; } + static absl::StatusOr> Create( const ChannelArgs& args, ChannelFilter::Args) { return std::make_unique(args); @@ -119,8 +121,7 @@ void ServiceConfigChannelArgFilter::Call::OnClientInitialMetadata( const grpc_channel_filter ServiceConfigChannelArgFilter::kFilter = MakePromiseBasedFilter( - "service_config_channel_arg"); + FilterEndpoint::kClient>(); } // namespace diff --git a/src/core/util/http_client/httpcli.cc b/src/core/util/http_client/httpcli.cc index 5921496ef24e2..12f13347470cc 100644 --- a/src/core/util/http_client/httpcli.cc +++ b/src/core/util/http_client/httpcli.cc @@ -35,6 +35,7 @@ #include #include +#include "src/core/handshaker/handshaker.h" #include "src/core/handshaker/handshaker_registry.h" #include "src/core/handshaker/tcp_connect/tcp_connect_handshaker.h" #include "src/core/lib/address_utils/sockaddr_utils.h" @@ -192,9 +193,7 @@ HttpRequest::HttpRequest( HttpRequest::~HttpRequest() { grpc_channel_args_destroy(channel_args_); grpc_http_parser_destroy(&parser_); - if (own_endpoint_ && ep_ != nullptr) { - grpc_endpoint_destroy(ep_); - } + ep_.reset(); CSliceUnref(request_text_); grpc_iomgr_unregister_object(&iomgr_obj_); grpc_slice_buffer_destroy(&incoming_); @@ -231,10 +230,7 @@ void HttpRequest::Orphan() { handshake_mgr_->Shutdown( GRPC_ERROR_CREATE("HTTP request cancelled during handshake")); } - if (own_endpoint_ && ep_ != nullptr) { - grpc_endpoint_destroy(ep_); - ep_ = nullptr; - } + ep_.reset(); } Unref(); } @@ -288,36 +284,30 @@ void HttpRequest::StartWrite() { CSliceRef(request_text_); grpc_slice_buffer_add(&outgoing_, request_text_); Ref().release(); // ref held by pending write - grpc_endpoint_write(ep_, &outgoing_, &done_write_, nullptr, + grpc_endpoint_write(ep_.get(), &outgoing_, &done_write_, nullptr, /*max_frame_size=*/INT_MAX); } -void HttpRequest::OnHandshakeDone(void* arg, grpc_error_handle error) { - auto* args = static_cast(arg); - RefCountedPtr req(static_cast(args->user_data)); +void HttpRequest::OnHandshakeDone(absl::StatusOr result) { if (g_test_only_on_handshake_done_intercept != nullptr) { // Run this testing intercept before the lock so that it has a chance to // do things like calling Orphan on the request - g_test_only_on_handshake_done_intercept(req.get()); + g_test_only_on_handshake_done_intercept(this); } - MutexLock lock(&req->mu_); - req->own_endpoint_ = true; - if (!error.ok()) { - req->handshake_mgr_.reset(); - req->NextAddress(error); + MutexLock lock(&mu_); + if (!result.ok()) { + handshake_mgr_.reset(); + NextAddress(result.status()); return; } - // Handshake completed, so we own fields in args - grpc_slice_buffer_destroy(args->read_buffer); - gpr_free(args->read_buffer); - req->ep_ = args->endpoint; - req->handshake_mgr_.reset(); - if (req->cancelled_) { - req->NextAddress( - GRPC_ERROR_CREATE("HTTP request cancelled during handshake")); + // Handshake completed, so get the endpoint. + ep_ = std::move((*result)->endpoint); + handshake_mgr_.reset(); + if (cancelled_) { + NextAddress(GRPC_ERROR_CREATE("HTTP request cancelled during handshake")); return; } - req->StartWrite(); + StartWrite(); } void HttpRequest::DoHandshake(const grpc_resolved_address* addr) { @@ -343,13 +333,11 @@ void HttpRequest::DoHandshake(const grpc_resolved_address* addr) { handshake_mgr_ = MakeRefCounted(); CoreConfiguration::Get().handshaker_registry().AddHandshakers( HANDSHAKER_CLIENT, args, pollset_set_, handshake_mgr_.get()); - Ref().release(); // ref held by pending handshake - grpc_endpoint* ep = ep_; - ep_ = nullptr; - own_endpoint_ = false; - handshake_mgr_->DoHandshake(ep, args, deadline_, - /*acceptor=*/nullptr, OnHandshakeDone, - /*user_data=*/this); + handshake_mgr_->DoHandshake( + nullptr, args, deadline_, /*acceptor=*/nullptr, + [self = Ref()](absl::StatusOr result) { + self->OnHandshakeDone(std::move(result)); + }); } void HttpRequest::NextAddress(grpc_error_handle error) { diff --git a/src/core/util/http_client/httpcli.h b/src/core/util/http_client/httpcli.h index 2ad2810f027b8..7101ebf7ed837 100644 --- a/src/core/util/http_client/httpcli.h +++ b/src/core/util/http_client/httpcli.h @@ -186,7 +186,7 @@ class HttpRequest : public InternallyRefCounted { void DoRead() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_) { Ref().release(); // ref held by pending read - grpc_endpoint_read(ep_, &incoming_, &on_read_, /*urgent=*/true, + grpc_endpoint_read(ep_.get(), &incoming_, &on_read_, /*urgent=*/true, /*min_progress_size=*/1); } @@ -221,7 +221,7 @@ class HttpRequest : public InternallyRefCounted { void StartWrite() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_); - static void OnHandshakeDone(void* arg, grpc_error_handle error); + void OnHandshakeDone(absl::StatusOr result); void DoHandshake(const grpc_resolved_address* addr) ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_); @@ -240,7 +240,7 @@ class HttpRequest : public InternallyRefCounted { grpc_closure continue_on_read_after_schedule_on_exec_ctx_; grpc_closure done_write_; grpc_closure continue_done_write_after_schedule_on_exec_ctx_; - grpc_endpoint* ep_ = nullptr; + OrphanablePtr ep_; grpc_closure* on_done_; ResourceQuotaRefPtr resource_quota_; grpc_polling_entity* pollent_; @@ -248,7 +248,6 @@ class HttpRequest : public InternallyRefCounted { const absl::optional> test_only_generate_response_; Mutex mu_; RefCountedPtr handshake_mgr_ ABSL_GUARDED_BY(mu_); - bool own_endpoint_ ABSL_GUARDED_BY(mu_) = true; bool cancelled_ ABSL_GUARDED_BY(mu_) = false; grpc_http_parser parser_ ABSL_GUARDED_BY(mu_); std::vector addresses_ ABSL_GUARDED_BY(mu_); diff --git a/src/cpp/ext/filters/census/client_filter.cc b/src/cpp/ext/filters/census/client_filter.cc index 9ef8bf5a55758..1f15fd4d70f85 100644 --- a/src/cpp/ext/filters/census/client_filter.cc +++ b/src/cpp/ext/filters/census/client_filter.cc @@ -81,8 +81,7 @@ constexpr uint32_t const grpc_channel_filter OpenCensusClientFilter::kFilter = grpc_core::MakePromiseBasedFilter( - "opencensus_client"); + grpc_core::FilterEndpoint::kClient, 0>(); absl::StatusOr> OpenCensusClientFilter::Create(const grpc_core::ChannelArgs& args, diff --git a/src/cpp/ext/filters/census/client_filter.h b/src/cpp/ext/filters/census/client_filter.h index a549510bbe263..8e2f4057c3b2b 100644 --- a/src/cpp/ext/filters/census/client_filter.h +++ b/src/cpp/ext/filters/census/client_filter.h @@ -38,6 +38,8 @@ class OpenCensusClientFilter : public grpc_core::ChannelFilter { public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "opencensus_client"; } + static absl::StatusOr> Create( const grpc_core::ChannelArgs& args, ChannelFilter::Args /*filter_args*/); diff --git a/src/objective-c/BoringSSL-GRPC.podspec b/src/objective-c/BoringSSL-GRPC.podspec index 50267fefd6432..dff312bbdfafd 100644 --- a/src/objective-c/BoringSSL-GRPC.podspec +++ b/src/objective-c/BoringSSL-GRPC.podspec @@ -106,7 +106,7 @@ Pod::Spec.new do |s| # We don't need to inhibit all warnings; only -Wno-shorten-64-to-32. But Cocoapods' linter doesn't # want that for some reason. - s.compiler_flags = '-DOPENSSL_NO_ASM', '-GCC_WARN_INHIBIT_ALL_WARNINGS', '-w', '-DBORINGSSL_PREFIX=GRPC' + s.compiler_flags = '-DOPENSSL_NO_ASM', '-w', '-DBORINGSSL_PREFIX=GRPC' s.requires_arc = false # Like many other C libraries, BoringSSL has its public headers under `include//` and its diff --git a/src/python/grpcio/README.rst b/src/python/grpcio/README.rst index 0c371487c70d7..eb2e8a9aa5990 100644 --- a/src/python/grpcio/README.rst +++ b/src/python/grpcio/README.rst @@ -91,3 +91,16 @@ Help, I ... sudo apt-get install python-dev + +Versioning +~~~~~~~~~~ + +gRPC Python is developed in a monorepo shared with implementations of gRPC in +other programming languages. While the minor versions are released in +lock-step with other languages in the repo (e.g. 1.63.0 is guaranteed to exist +for all languages), patch versions may be specific to only a single +language. For example, if 1.63.1 is a C++-specific patch, 1.63.1 may not be +uploaded to PyPi. As a result, it is __not__ a good assumption that the latest +patch for a given minor version on Github is also the latest patch for that +same minor version on PyPi. + diff --git a/templates/src/objective-c/BoringSSL-GRPC.podspec.template b/templates/src/objective-c/BoringSSL-GRPC.podspec.template index 2c7a26e172123..1380e70fe4a07 100644 --- a/templates/src/objective-c/BoringSSL-GRPC.podspec.template +++ b/templates/src/objective-c/BoringSSL-GRPC.podspec.template @@ -137,7 +137,7 @@ # We don't need to inhibit all warnings; only -Wno-shorten-64-to-32. But Cocoapods' linter doesn't # want that for some reason. - s.compiler_flags = '-DOPENSSL_NO_ASM', '-GCC_WARN_INHIBIT_ALL_WARNINGS', '-w', '-DBORINGSSL_PREFIX=GRPC' + s.compiler_flags = '-DOPENSSL_NO_ASM', '-w', '-DBORINGSSL_PREFIX=GRPC' s.requires_arc = false # Like many other C libraries, BoringSSL has its public headers under `include//` and its diff --git a/test/core/bad_client/bad_client.cc b/test/core/bad_client/bad_client.cc index c3e1bf920a791..012dd6abddb6b 100644 --- a/test/core/bad_client/bad_client.cc +++ b/test/core/bad_client/bad_client.cc @@ -227,7 +227,7 @@ void grpc_run_bad_client_test( grpc_core::CoreConfiguration::Get() .channel_args_preconditioning() .PreconditionChannelArgs(server_args.ToC().get()), - sfd.server, false); + grpc_core::OrphanablePtr(sfd.server), false); server_setup_transport(&a, transport); grpc_chttp2_transport_start_reading(transport, nullptr, nullptr, nullptr, nullptr); diff --git a/test/core/bad_connection/close_fd_test.cc b/test/core/bad_connection/close_fd_test.cc index 823cecf78de9b..a2afa61fd899b 100644 --- a/test/core/bad_connection/close_fd_test.cc +++ b/test/core/bad_connection/close_fd_test.cc @@ -36,6 +36,7 @@ #include "src/core/channelz/channelz.h" #include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/iomgr/endpoint.h" #include "src/core/lib/iomgr/error.h" @@ -121,8 +122,9 @@ static void client_setup_transport(grpc_core::Transport* transport) { static void init_client() { grpc_core::ExecCtx exec_ctx; grpc_core::Transport* transport; - transport = grpc_create_chttp2_transport(grpc_core::ChannelArgs(), - g_ctx.ep->client, true); + transport = grpc_create_chttp2_transport( + grpc_core::ChannelArgs(), + grpc_core::OrphanablePtr(g_ctx.ep->client), true); client_setup_transport(transport); CHECK(g_ctx.client); grpc_chttp2_transport_start_reading(transport, nullptr, nullptr, nullptr, @@ -136,8 +138,9 @@ static void init_server() { g_ctx.server = grpc_server_create(nullptr, nullptr); grpc_server_register_completion_queue(g_ctx.server, g_ctx.cq, nullptr); grpc_server_start(g_ctx.server); - transport = grpc_create_chttp2_transport(grpc_core::ChannelArgs(), - g_ctx.ep->server, false); + transport = grpc_create_chttp2_transport( + grpc_core::ChannelArgs(), + grpc_core::OrphanablePtr(g_ctx.ep->server), false); server_setup_transport(transport); grpc_chttp2_transport_start_reading(transport, nullptr, nullptr, nullptr, nullptr); diff --git a/test/core/channel/channel_stack_builder_test.cc b/test/core/channel/channel_stack_builder_test.cc index 2c7dff9526e11..82bda202df411 100644 --- a/test/core/channel/channel_stack_builder_test.cc +++ b/test/core/channel/channel_stack_builder_test.cc @@ -19,6 +19,7 @@ #include "src/core/lib/channel/channel_stack_builder.h" #include +#include #include #include "absl/status/status.h" @@ -53,19 +54,24 @@ void CallDestroyFunc(grpc_call_element* /*elem*/, const grpc_call_final_info* /*final_info*/, grpc_closure* /*ignored*/) {} -const grpc_channel_filter* FilterNamed(const char* name) { +const grpc_channel_filter* FilterNamed(absl::string_view name) { static auto* filters = new std::map; auto it = filters->find(name); if (it != filters->end()) return it->second; + static auto* name_factories = + new std::vector>(); + name_factories->emplace_back(std::make_unique(name)); + auto unique_type_name = name_factories->back()->Create(); return filters - ->emplace(name, - new grpc_channel_filter{ - grpc_call_next_op, grpc_channel_next_op, 0, CallInitFunc, - grpc_call_stack_ignore_set_pollset_or_pollset_set, - CallDestroyFunc, 0, ChannelInitFunc, - [](grpc_channel_stack*, grpc_channel_element*) {}, - ChannelDestroyFunc, grpc_channel_next_get_info, name}) + ->emplace( + name, + new grpc_channel_filter{ + grpc_call_next_op, grpc_channel_next_op, 0, CallInitFunc, + grpc_call_stack_ignore_set_pollset_or_pollset_set, + CallDestroyFunc, 0, ChannelInitFunc, + [](grpc_channel_stack*, grpc_channel_element*) {}, + ChannelDestroyFunc, grpc_channel_next_get_info, unique_type_name}) .first->second; } diff --git a/test/core/channel/channel_stack_test.cc b/test/core/channel/channel_stack_test.cc index bc94f262aaf05..10d4ee8e5fbd7 100644 --- a/test/core/channel/channel_stack_test.cc +++ b/test/core/channel/channel_stack_test.cc @@ -92,7 +92,7 @@ TEST(ChannelStackTest, CreateChannelStack) { grpc_channel_stack_no_post_init, channel_destroy_func, grpc_channel_next_get_info, - "some_test_filter"}; + GRPC_UNIQUE_TYPE_NAME_HERE("some_test_filter")}; const grpc_channel_filter* filters = &filter; grpc_channel_stack* channel_stack; grpc_call_stack* call_stack; diff --git a/test/core/channel/minimal_stack_is_minimal_test.cc b/test/core/channel/minimal_stack_is_minimal_test.cc index ebf0f66c82c76..cf1a7b0833959 100644 --- a/test/core/channel/minimal_stack_is_minimal_test.cc +++ b/test/core/channel/minimal_stack_is_minimal_test.cc @@ -101,9 +101,7 @@ std::vector MakeStack(const char* transport_name, std::vector parts; for (const auto& entry : *builder.mutable_stack()) { - const char* name = entry->name; - if (name == nullptr) continue; - parts.push_back(name); + parts.push_back(std::string(entry->name.name())); } return parts; diff --git a/test/core/end2end/fixtures/sockpair_fixture.h b/test/core/end2end/fixtures/sockpair_fixture.h index 93714b96e05f5..354f052a76993 100644 --- a/test/core/end2end/fixtures/sockpair_fixture.h +++ b/test/core/end2end/fixtures/sockpair_fixture.h @@ -79,11 +79,12 @@ class SockpairFixture : public CoreTestFixture { auto server_channel_args = CoreConfiguration::Get() .channel_args_preconditioning() .PreconditionChannelArgs(args.ToC().get()); - auto* server_endpoint = std::exchange(ep_.server, nullptr); + OrphanablePtr server_endpoint( + std::exchange(ep_.server, nullptr)); EXPECT_NE(server_endpoint, nullptr); + grpc_endpoint_add_to_pollset(server_endpoint.get(), grpc_cq_pollset(cq)); transport = grpc_create_chttp2_transport(server_channel_args, - server_endpoint, false); - grpc_endpoint_add_to_pollset(server_endpoint, grpc_cq_pollset(cq)); + std::move(server_endpoint), false); Server* core_server = Server::FromC(server); grpc_error_handle error = core_server->SetupTransport( transport, nullptr, core_server->channel_args(), nullptr); @@ -106,9 +107,11 @@ class SockpairFixture : public CoreTestFixture { .ToC() .get()); Transport* transport; - auto* client_endpoint = std::exchange(ep_.client, nullptr); + OrphanablePtr client_endpoint( + std::exchange(ep_.client, nullptr)); EXPECT_NE(client_endpoint, nullptr); - transport = grpc_create_chttp2_transport(args, client_endpoint, true); + transport = + grpc_create_chttp2_transport(args, std::move(client_endpoint), true); auto channel = ChannelCreate("socketpair-target", args, GRPC_CLIENT_DIRECT_CHANNEL, transport); grpc_channel* client; diff --git a/test/core/end2end/fuzzers/client_fuzzer.cc b/test/core/end2end/fuzzers/client_fuzzer.cc index 3e7ca5089ba31..4eef246458acc 100644 --- a/test/core/end2end/fuzzers/client_fuzzer.cc +++ b/test/core/end2end/fuzzers/client_fuzzer.cc @@ -29,6 +29,7 @@ #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/experiments/config.h" #include "src/core/lib/gprpp/env.h" +#include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/iomgr/endpoint.h" #include "src/core/lib/iomgr/exec_ctx.h" @@ -65,8 +66,8 @@ class ClientFuzzer final : public BasicFuzzer { .channel_args_preconditioning() .PreconditionChannelArgs(nullptr) .SetIfUnset(GRPC_ARG_DEFAULT_AUTHORITY, "test-authority"); - Transport* transport = - grpc_create_chttp2_transport(args, mock_endpoint_, true); + Transport* transport = grpc_create_chttp2_transport( + args, OrphanablePtr(mock_endpoint_), true); channel_ = ChannelCreate("test-target", args, GRPC_CLIENT_DIRECT_CHANNEL, transport) ->release() diff --git a/test/core/end2end/tests/filter_causes_close.cc b/test/core/end2end/tests/filter_causes_close.cc index 0e96e1213d769..8a77719e4017b 100644 --- a/test/core/end2end/tests/filter_causes_close.cc +++ b/test/core/end2end/tests/filter_causes_close.cc @@ -52,6 +52,7 @@ namespace { class TestFilter : public ImplementChannelFilter { public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "filter_causes_close"; } static absl::StatusOr> Create( const ChannelArgs&, ChannelFilter::Args) { return std::make_unique(); @@ -81,8 +82,7 @@ const NoInterceptor TestFilter::Call::OnServerToClientMessage; const NoInterceptor TestFilter::Call::OnFinalize; const grpc_channel_filter TestFilter::kFilter = - MakePromiseBasedFilter( - "filter_causes_close"); + MakePromiseBasedFilter(); CORE_END2END_TEST(CoreEnd2endTest, FilterCausesClose) { CoreConfiguration::RegisterBuilder([](CoreConfiguration::Builder* builder) { diff --git a/test/core/end2end/tests/filter_init_fails.cc b/test/core/end2end/tests/filter_init_fails.cc index 9d2b272a2369e..c2b08486fee1f 100644 --- a/test/core/end2end/tests/filter_init_fails.cc +++ b/test/core/end2end/tests/filter_init_fails.cc @@ -85,7 +85,7 @@ const grpc_channel_filter test_filter = { // last one. // Filter ordering code falls back to lexical ordering in the absense of // other dependencies, so name this appropriately. - "zzzzzz_filter_init_fails"}; + GRPC_UNIQUE_TYPE_NAME_HERE("zzzzzz_filter_init_fails")}; void RegisterFilter(grpc_channel_stack_type type) { CoreConfiguration::RegisterBuilder( diff --git a/test/core/end2end/tests/max_connection_idle.cc b/test/core/end2end/tests/max_connection_idle.cc index 466e72ff5b535..ab9371485ab3a 100644 --- a/test/core/end2end/tests/max_connection_idle.cc +++ b/test/core/end2end/tests/max_connection_idle.cc @@ -90,7 +90,9 @@ CORE_END2END_TEST(RetryHttp2Test, MaxConnectionIdle) { .Set(GRPC_ARG_INITIAL_RECONNECT_BACKOFF_MS, Duration::Seconds(1).millis()) .Set(GRPC_ARG_MAX_RECONNECT_BACKOFF_MS, Duration::Seconds(1).millis()) - .Set(GRPC_ARG_MIN_RECONNECT_BACKOFF_MS, Duration::Seconds(5).millis()) + .Set(GRPC_ARG_MIN_RECONNECT_BACKOFF_MS, + g_is_fuzzing_core_e2e_tests ? Duration::Minutes(5).millis() + : Duration::Seconds(5).millis()) // Avoid transparent retries for this test. .Set(GRPC_ARG_ENABLE_RETRIES, false)); InitServer( diff --git a/test/core/end2end/tests/retry_cancel_with_multiple_send_batches.cc b/test/core/end2end/tests/retry_cancel_with_multiple_send_batches.cc index 1ee77a489fcdd..a5b932a7ea0ec 100644 --- a/test/core/end2end/tests/retry_cancel_with_multiple_send_batches.cc +++ b/test/core/end2end/tests/retry_cancel_with_multiple_send_batches.cc @@ -172,7 +172,7 @@ grpc_channel_filter FailSendOpsFilter::kFilterVtable = { grpc_channel_stack_no_post_init, Destroy, grpc_channel_next_get_info, - "FailSendOpsFilter", + GRPC_UNIQUE_TYPE_NAME_HERE("FailSendOpsFilter"), }; void RegisterFilter() { diff --git a/test/core/end2end/tests/retry_recv_message_replay.cc b/test/core/end2end/tests/retry_recv_message_replay.cc index 902b52800ddf4..c3057f88161a4 100644 --- a/test/core/end2end/tests/retry_recv_message_replay.cc +++ b/test/core/end2end/tests/retry_recv_message_replay.cc @@ -32,6 +32,7 @@ #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/gprpp/status_helper.h" #include "src/core/lib/gprpp/time.h" +#include "src/core/lib/gprpp/unique_type_name.h" #include "src/core/lib/iomgr/call_combiner.h" #include "src/core/lib/iomgr/closure.h" #include "src/core/lib/iomgr/error.h" @@ -118,7 +119,7 @@ grpc_channel_filter FailFirstSendOpFilter::kFilterVtable = { grpc_channel_stack_no_post_init, Destroy, grpc_channel_next_get_info, - "FailFirstSendOpFilter", + GRPC_UNIQUE_TYPE_NAME_HERE("FailFirstSendOpFilter"), }; // Tests the fix for a bug found in real-world code where recv_message diff --git a/test/core/end2end/tests/retry_recv_trailing_metadata_error.cc b/test/core/end2end/tests/retry_recv_trailing_metadata_error.cc index ec3f3b8ede76a..cdbd9b7544925 100644 --- a/test/core/end2end/tests/retry_recv_trailing_metadata_error.cc +++ b/test/core/end2end/tests/retry_recv_trailing_metadata_error.cc @@ -113,7 +113,7 @@ grpc_channel_filter InjectStatusFilter::kFilterVtable = { grpc_channel_stack_no_post_init, Destroy, grpc_channel_next_get_info, - "InjectStatusFilter", + GRPC_UNIQUE_TYPE_NAME_HERE("InjectStatusFilter"), }; // Tests that we honor the error passed to recv_trailing_metadata_ready diff --git a/test/core/end2end/tests/retry_send_op_fails.cc b/test/core/end2end/tests/retry_send_op_fails.cc index 840525ef57fbd..825745ab96ac7 100644 --- a/test/core/end2end/tests/retry_send_op_fails.cc +++ b/test/core/end2end/tests/retry_send_op_fails.cc @@ -119,7 +119,7 @@ grpc_channel_filter FailFirstCallFilter::kFilterVtable = { grpc_channel_stack_no_post_init, Destroy, grpc_channel_next_get_info, - "FailFirstCallFilter", + GRPC_UNIQUE_TYPE_NAME_HERE("FailFirstCallFilter"), }; // Tests failure on a send op batch: diff --git a/test/core/end2end/tests/retry_transparent_goaway.cc b/test/core/end2end/tests/retry_transparent_goaway.cc index ef3c9561c5850..be68169cc26a5 100644 --- a/test/core/end2end/tests/retry_transparent_goaway.cc +++ b/test/core/end2end/tests/retry_transparent_goaway.cc @@ -29,6 +29,7 @@ #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/gprpp/status_helper.h" #include "src/core/lib/gprpp/time.h" +#include "src/core/lib/gprpp/unique_type_name.h" #include "src/core/lib/iomgr/call_combiner.h" #include "src/core/lib/iomgr/closure.h" #include "src/core/lib/iomgr/error.h" @@ -125,7 +126,7 @@ grpc_channel_filter FailFirstCallFilter::kFilterVtable = { grpc_channel_stack_no_post_init, Destroy, grpc_channel_next_get_info, - "FailFirstCallFilter", + GRPC_UNIQUE_TYPE_NAME_HERE("FailFirstCallFilter"), }; // Tests transparent retries when the call was never sent out on the wire. diff --git a/test/core/end2end/tests/retry_transparent_not_sent_on_wire.cc b/test/core/end2end/tests/retry_transparent_not_sent_on_wire.cc index 735be66f6e98d..b55ee605c4cb3 100644 --- a/test/core/end2end/tests/retry_transparent_not_sent_on_wire.cc +++ b/test/core/end2end/tests/retry_transparent_not_sent_on_wire.cc @@ -31,6 +31,7 @@ #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/gprpp/status_helper.h" #include "src/core/lib/gprpp/time.h" +#include "src/core/lib/gprpp/unique_type_name.h" #include "src/core/lib/iomgr/call_combiner.h" #include "src/core/lib/iomgr/closure.h" #include "src/core/lib/iomgr/error.h" @@ -124,7 +125,7 @@ grpc_channel_filter FailFirstTenCallsFilter::kFilterVtable = { grpc_channel_stack_no_post_init, Destroy, grpc_channel_next_get_info, - "FailFirstTenCallsFilter", + GRPC_UNIQUE_TYPE_NAME_HERE("FailFirstTenCallsFilter"), }; // Tests transparent retries when the call was never sent out on the wire. diff --git a/test/core/gprpp/unique_type_name_test.cc b/test/core/gprpp/unique_type_name_test.cc index bce147edd321c..83737c8c6a2c6 100644 --- a/test/core/gprpp/unique_type_name_test.cc +++ b/test/core/gprpp/unique_type_name_test.cc @@ -93,6 +93,26 @@ TEST(UniqueTypeNameTest, CanUseAsMapKey) { ::testing::Pair(bar.type(), 2))); } +struct Filter1 { + static absl::string_view TypeName() { return "Filter1"; } +}; + +struct Filter2 { + static absl::string_view TypeName() { return "Filter2"; } +}; + +TEST(UniqueTypeNameTest, UniqueTypeNameFor) { + EXPECT_EQ(UniqueTypeNameFor(), UniqueTypeNameFor()); + EXPECT_NE(UniqueTypeNameFor(), UniqueTypeNameFor()); +} + +TEST(UniqueTypeNameTest, UniqueTypeNameHere) { + auto name1 = GRPC_UNIQUE_TYPE_NAME_HERE("name"); + auto name2 = GRPC_UNIQUE_TYPE_NAME_HERE("name"); + EXPECT_EQ(name1.name(), name2.name()); + EXPECT_NE(name1, name2); +} + } // namespace } // namespace grpc_core diff --git a/test/core/handshake/readahead_handshaker_server_ssl.cc b/test/core/handshake/readahead_handshaker_server_ssl.cc index c5331e1abb40d..b15edfc32c681 100644 --- a/test/core/handshake/readahead_handshaker_server_ssl.cc +++ b/test/core/handshake/readahead_handshaker_server_ssl.cc @@ -18,6 +18,8 @@ #include +#include "absl/base/thread_annotations.h" +#include "absl/strings/string_view.h" #include "gtest/gtest.h" #include @@ -28,6 +30,7 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/gprpp/sync.h" #include "src/core/lib/iomgr/closure.h" #include "src/core/lib/iomgr/endpoint.h" #include "src/core/lib/iomgr/error.h" @@ -49,15 +52,52 @@ namespace grpc_core { class ReadAheadHandshaker : public Handshaker { public: - ~ReadAheadHandshaker() override {} - const char* name() const override { return "read_ahead"; } - void Shutdown(grpc_error_handle /*why*/) override {} - void DoHandshake(grpc_tcp_server_acceptor* /*acceptor*/, - grpc_closure* on_handshake_done, - HandshakerArgs* args) override { - grpc_endpoint_read(args->endpoint, args->read_buffer, on_handshake_done, - /*urgent=*/false, /*min_progress_size=*/1); + absl::string_view name() const override { return "read_ahead"; } + + void DoHandshake( + HandshakerArgs* args, + absl::AnyInvocable on_handshake_done) override { + MutexLock lock(&mu_); + args_ = args; + on_handshake_done_ = std::move(on_handshake_done); + Ref().release(); // Held by callback. + GRPC_CLOSURE_INIT(&on_read_done_, OnReadDone, this, nullptr); + grpc_endpoint_read(args->endpoint.get(), args->read_buffer.c_slice_buffer(), + &on_read_done_, /*urgent=*/false, + /*min_progress_size=*/1); + } + + void Shutdown(absl::Status /*error*/) override { + MutexLock lock(&mu_); + if (on_handshake_done_ != nullptr) args_->endpoint.reset(); } + + private: + static void OnReadDone(void* arg, grpc_error_handle error) { + auto* self = static_cast(arg); + // Need an async hop here, because grpc_endpoint_read() may invoke + // the callback synchronously, leading to deadlock. + // TODO(roth): This async hop will no longer be necessary once we + // switch to the EventEngine endpoint API. + self->args_->event_engine->Run( + [self = RefCountedPtr(self), + error = std::move(error)]() mutable { + absl::AnyInvocable on_handshake_done; + { + MutexLock lock(&self->mu_); + on_handshake_done = std::move(self->on_handshake_done_); + } + on_handshake_done(std::move(error)); + }); + } + + grpc_closure on_read_done_; + + Mutex mu_; + // Mutex guards args_->endpoint but not the rest of the struct. + HandshakerArgs* args_ = nullptr; + absl::AnyInvocable on_handshake_done_ + ABSL_GUARDED_BY(&mu_); }; class ReadAheadHandshakerFactory : public HandshakerFactory { diff --git a/test/core/security/secure_endpoint_test.cc b/test/core/security/secure_endpoint_test.cc index baf90302f6922..b09e04c4b19eb 100644 --- a/test/core/security/secure_endpoint_test.cc +++ b/test/core/security/secure_endpoint_test.cc @@ -161,9 +161,11 @@ static grpc_endpoint_test_fixture secure_endpoint_create_fixture_tcp_socketpair( } if (leftover_nslices == 0) { - f.client_ep = grpc_secure_endpoint_create(fake_read_protector, - fake_read_zero_copy_protector, - tcp.client, nullptr, &args, 0); + f.client_ep = grpc_secure_endpoint_create( + fake_read_protector, fake_read_zero_copy_protector, + grpc_core::OrphanablePtr(tcp.client), + nullptr, &args, 0) + .release(); } else { unsigned i; tsi_result result; @@ -206,15 +208,19 @@ static grpc_endpoint_test_fixture secure_endpoint_create_fixture_tcp_socketpair( reinterpret_cast(encrypted_buffer), total_buffer_size - buffer_size); f.client_ep = grpc_secure_endpoint_create( - fake_read_protector, fake_read_zero_copy_protector, tcp.client, - &encrypted_leftover, &args, 1); + fake_read_protector, fake_read_zero_copy_protector, + grpc_core::OrphanablePtr(tcp.client), + &encrypted_leftover, &args, 1) + .release(); grpc_slice_unref(encrypted_leftover); gpr_free(encrypted_buffer); } - f.server_ep = grpc_secure_endpoint_create(fake_write_protector, - fake_write_zero_copy_protector, - tcp.server, nullptr, &args, 0); + f.server_ep = grpc_secure_endpoint_create( + fake_write_protector, fake_write_zero_copy_protector, + grpc_core::OrphanablePtr(tcp.server), + nullptr, &args, 0) + .release(); grpc_resource_quota_unref( static_cast(a[1].value.pointer.p)); return f; diff --git a/test/core/security/ssl_server_fuzzer.cc b/test/core/security/ssl_server_fuzzer.cc index 62a9de9d69e21..e0671ab4a8f59 100644 --- a/test/core/security/ssl_server_fuzzer.cc +++ b/test/core/security/ssl_server_fuzzer.cc @@ -15,7 +15,9 @@ // limitations under the License. // // + #include "absl/log/check.h" +#include "absl/synchronization/notification.h" #include #include @@ -36,6 +38,7 @@ #define SERVER_CERT_PATH "src/core/tsi/test_creds/server1.pem" #define SERVER_KEY_PATH "src/core/tsi/test_creds/server1.key" +using grpc_core::HandshakerArgs; using grpc_event_engine::experimental::EventEngine; using grpc_event_engine::experimental::GetDefaultEventEngine; @@ -44,20 +47,6 @@ bool squelch = true; // Turning this on will fail the leak check. bool leak_check = false; -struct handshake_state { - grpc_core::Notification done_signal; -}; - -static void on_handshake_done(void* arg, grpc_error_handle error) { - grpc_core::HandshakerArgs* args = - static_cast(arg); - struct handshake_state* state = - static_cast(args->user_data); - // The fuzzer should not pass the handshake. - CHECK(!error.ok()); - state->done_signal.Notify(); -} - extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { if (squelch) { grpc_disable_all_absl_logs(); @@ -91,21 +80,26 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { grpc_core::Timestamp deadline = grpc_core::Duration::Seconds(1) + grpc_core::Timestamp::Now(); - struct handshake_state state; auto handshake_mgr = grpc_core::MakeRefCounted(); auto channel_args = grpc_core::ChannelArgs().SetObject(std::move(engine)); sc->add_handshakers(channel_args, nullptr, handshake_mgr.get()); - handshake_mgr->DoHandshake(mock_endpoint, channel_args, deadline, - nullptr /* acceptor */, on_handshake_done, - &state); + absl::Notification handshake_completed; + handshake_mgr->DoHandshake( + grpc_core::OrphanablePtr(mock_endpoint), channel_args, + deadline, nullptr /* acceptor */, + [&](absl::StatusOr result) { + // The fuzzer should not pass the handshake. + CHECK(!result.ok()); + handshake_completed.Notify(); + }); grpc_core::ExecCtx::Get()->Flush(); // If the given string happens to be part of the correct client hello, the // server will wait for more data. Explicitly fail the server by shutting // down the handshake manager. - if (!state.done_signal.WaitForNotificationWithTimeout(absl::Seconds(3))) { + if (!handshake_completed.WaitForNotificationWithTimeout(absl::Seconds(3))) { handshake_mgr->Shutdown( absl::DeadlineExceededError("handshake did not fail as expected")); } diff --git a/test/core/server/xds_channel_stack_modifier_test.cc b/test/core/server/xds_channel_stack_modifier_test.cc index 4c1e9f0919dcc..16f0e6e4e8194 100644 --- a/test/core/server/xds_channel_stack_modifier_test.cc +++ b/test/core/server/xds_channel_stack_modifier_test.cc @@ -70,8 +70,8 @@ TEST(XdsChannelStackModifierTest, ChannelArgsCompare) { grpc_shutdown(); } -constexpr char kTestFilter1[] = "test_filter_1"; -constexpr char kTestFilter2[] = "test_filter_2"; +const UniqueTypeName kTestFilter1 = GRPC_UNIQUE_TYPE_NAME_HERE("test_filter_1"); +const UniqueTypeName kTestFilter2 = GRPC_UNIQUE_TYPE_NAME_HERE("test_filter_2"); namespace { class FakeTransport final : public Transport { @@ -117,11 +117,12 @@ TEST(XdsChannelStackModifierTest, XdsHttpFiltersInsertion) { } std::vector filters; for (const auto& entry : *builder.mutable_stack()) { - filters.push_back(entry->name); + filters.push_back(std::string(entry->name.name())); } filters.resize(3); - EXPECT_EQ(filters, - std::vector({"server", kTestFilter1, kTestFilter2})); + EXPECT_EQ(filters, std::vector( + {"server", std::string(kTestFilter1.name()), + std::string(kTestFilter2.name())})); grpc_shutdown(); } diff --git a/test/core/surface/channel_init_test.cc b/test/core/surface/channel_init_test.cc index b0b84323d4751..39a8781f00bd3 100644 --- a/test/core/surface/channel_init_test.cc +++ b/test/core/surface/channel_init_test.cc @@ -37,10 +37,15 @@ const grpc_channel_filter* FilterNamed(const char* name) { new std::map; auto it = filters->find(name); if (it != filters->end()) return it->second; + static auto* name_factories = + new std::vector>(); + name_factories->emplace_back(std::make_unique(name)); + auto unique_type_name = name_factories->back()->Create(); return filters - ->emplace(name, new grpc_channel_filter{nullptr, nullptr, 0, nullptr, - nullptr, nullptr, 0, nullptr, - nullptr, nullptr, nullptr, name}) + ->emplace(name, + new grpc_channel_filter{nullptr, nullptr, 0, nullptr, nullptr, + nullptr, 0, nullptr, nullptr, nullptr, + nullptr, unique_type_name}) .first->second; } @@ -51,7 +56,7 @@ std::vector GetFilterNames(const ChannelInit& init, if (!init.CreateStack(&b)) return {}; std::vector names; for (auto f : b.stack()) { - names.push_back(f->name); + names.push_back(std::string(f->name.name())); } EXPECT_NE(names, std::vector()); return names; @@ -239,8 +244,9 @@ class TestFilter1 { }; const grpc_channel_filter TestFilter1::kFilter = { - nullptr, nullptr, 0, nullptr, nullptr, nullptr, - 0, nullptr, nullptr, nullptr, nullptr, "test_filter1"}; + nullptr, nullptr, 0, nullptr, + nullptr, nullptr, 0, nullptr, + nullptr, nullptr, nullptr, GRPC_UNIQUE_TYPE_NAME_HERE("test_filter1")}; const NoInterceptor TestFilter1::Call::OnClientInitialMetadata; const NoInterceptor TestFilter1::Call::OnServerInitialMetadata; const NoInterceptor TestFilter1::Call::OnServerTrailingMetadata; diff --git a/test/core/surface/secure_channel_create_test.cc b/test/core/surface/secure_channel_create_test.cc index 89f21615e98e6..baa1e8d05d13a 100644 --- a/test/core/surface/secure_channel_create_test.cc +++ b/test/core/surface/secure_channel_create_test.cc @@ -37,7 +37,7 @@ void test_unknown_scheme_target(void) { grpc_channel* chan = grpc_channel_create("blah://blah", creds, nullptr); grpc_channel_element* elem = grpc_channel_stack_element( grpc_core::Channel::FromC(chan)->channel_stack(), 0); - ASSERT_STREQ(elem->filter->name, "lame-client"); + ASSERT_EQ(elem->filter->name.name(), "lame-client"); grpc_core::ExecCtx exec_ctx; grpc_core::Channel::FromC(chan)->Unref(); creds->Unref(); @@ -51,7 +51,7 @@ void test_security_connector_already_in_arg(void) { grpc_channel* chan = grpc_channel_create(nullptr, nullptr, &args); grpc_channel_element* elem = grpc_channel_stack_element( grpc_core::Channel::FromC(chan)->channel_stack(), 0); - ASSERT_STREQ(elem->filter->name, "lame-client"); + ASSERT_EQ(elem->filter->name.name(), "lame-client"); grpc_core::ExecCtx exec_ctx; grpc_core::Channel::FromC(chan)->Unref(); } @@ -60,7 +60,7 @@ void test_null_creds(void) { grpc_channel* chan = grpc_channel_create(nullptr, nullptr, nullptr); grpc_channel_element* elem = grpc_channel_stack_element( grpc_core::Channel::FromC(chan)->channel_stack(), 0); - ASSERT_STREQ(elem->filter->name, "lame-client"); + ASSERT_EQ(elem->filter->name.name(), "lame-client"); grpc_core::ExecCtx exec_ctx; grpc_core::Channel::FromC(chan)->Unref(); } diff --git a/test/core/test_util/fake_stats_plugin.cc b/test/core/test_util/fake_stats_plugin.cc index 59546f9ce454f..3ce9431cd6662 100644 --- a/test/core/test_util/fake_stats_plugin.cc +++ b/test/core/test_util/fake_stats_plugin.cc @@ -24,6 +24,8 @@ class FakeStatsClientFilter : public ChannelFilter { public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "fake_stats_client"; } + explicit FakeStatsClientFilter( FakeClientCallTracerFactory* fake_client_call_tracer_factory); @@ -38,8 +40,7 @@ class FakeStatsClientFilter : public ChannelFilter { }; const grpc_channel_filter FakeStatsClientFilter::kFilter = - MakePromiseBasedFilter( - "fake_stats_client"); + MakePromiseBasedFilter(); absl::StatusOr> FakeStatsClientFilter::Create(const ChannelArgs& args, diff --git a/test/core/transport/chttp2/graceful_shutdown_test.cc b/test/core/transport/chttp2/graceful_shutdown_test.cc index c9040edc79872..f4e4da7268f9c 100644 --- a/test/core/transport/chttp2/graceful_shutdown_test.cc +++ b/test/core/transport/chttp2/graceful_shutdown_test.cc @@ -51,6 +51,7 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/gprpp/crash.h" #include "src/core/lib/gprpp/notification.h" +#include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/sync.h" #include "src/core/lib/iomgr/closure.h" #include "src/core/lib/iomgr/endpoint.h" @@ -94,8 +95,9 @@ class GracefulShutdownTest : public ::testing::Test { grpc_server_register_completion_queue(server_, cq_, nullptr); grpc_server_start(server_); fds_ = grpc_iomgr_create_endpoint_pair("fixture", nullptr); - auto* transport = grpc_create_chttp2_transport(core_server->channel_args(), - fds_.server, false); + auto* transport = grpc_create_chttp2_transport( + core_server->channel_args(), OrphanablePtr(fds_.server), + false); grpc_endpoint_add_to_pollset(fds_.server, grpc_cq_pollset(cq_)); CHECK(core_server->SetupTransport(transport, nullptr, core_server->channel_args(), diff --git a/test/core/transport/chttp2/ping_configuration_test.cc b/test/core/transport/chttp2/ping_configuration_test.cc index 83acda3b5662e..143a334103bbe 100644 --- a/test/core/transport/chttp2/ping_configuration_test.cc +++ b/test/core/transport/chttp2/ping_configuration_test.cc @@ -54,8 +54,10 @@ class ConfigurationTest : public ::testing::Test { TEST_F(ConfigurationTest, ClientKeepaliveDefaults) { ExecCtx exec_ctx; - grpc_chttp2_transport* t = reinterpret_cast( - grpc_create_chttp2_transport(args_, mock_endpoint_, /*is_client=*/true)); + grpc_chttp2_transport* t = + reinterpret_cast(grpc_create_chttp2_transport( + args_, OrphanablePtr(mock_endpoint_), + /*is_client=*/true)); EXPECT_EQ(t->keepalive_time, Duration::Infinity()); EXPECT_EQ(t->keepalive_timeout, Duration::Infinity()); EXPECT_EQ(t->keepalive_permit_without_calls, false); @@ -69,8 +71,10 @@ TEST_F(ConfigurationTest, ClientKeepaliveExplicitArgs) { args_ = args_.Set(GRPC_ARG_KEEPALIVE_TIMEOUT_MS, 10000); args_ = args_.Set(GRPC_ARG_KEEPALIVE_PERMIT_WITHOUT_CALLS, true); args_ = args_.Set(GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA, 3); - grpc_chttp2_transport* t = reinterpret_cast( - grpc_create_chttp2_transport(args_, mock_endpoint_, /*is_client=*/true)); + grpc_chttp2_transport* t = + reinterpret_cast(grpc_create_chttp2_transport( + args_, OrphanablePtr(mock_endpoint_), + /*is_client=*/true)); EXPECT_EQ(t->keepalive_time, Duration::Seconds(20)); EXPECT_EQ(t->keepalive_timeout, Duration::Seconds(10)); EXPECT_EQ(t->keepalive_permit_without_calls, true); @@ -80,8 +84,10 @@ TEST_F(ConfigurationTest, ClientKeepaliveExplicitArgs) { TEST_F(ConfigurationTest, ServerKeepaliveDefaults) { ExecCtx exec_ctx; - grpc_chttp2_transport* t = reinterpret_cast( - grpc_create_chttp2_transport(args_, mock_endpoint_, /*is_client=*/false)); + grpc_chttp2_transport* t = + reinterpret_cast(grpc_create_chttp2_transport( + args_, OrphanablePtr(mock_endpoint_), + /*is_client=*/false)); EXPECT_EQ(t->keepalive_time, Duration::Hours(2)); EXPECT_EQ(t->keepalive_timeout, Duration::Seconds(20)); EXPECT_EQ(t->keepalive_permit_without_calls, false); @@ -102,8 +108,10 @@ TEST_F(ConfigurationTest, ServerKeepaliveExplicitArgs) { args_ = args_.Set(GRPC_ARG_HTTP2_MIN_RECV_PING_INTERVAL_WITHOUT_DATA_MS, 20000); args_ = args_.Set(GRPC_ARG_HTTP2_MAX_PING_STRIKES, 0); - grpc_chttp2_transport* t = reinterpret_cast( - grpc_create_chttp2_transport(args_, mock_endpoint_, /*is_client=*/false)); + grpc_chttp2_transport* t = + reinterpret_cast(grpc_create_chttp2_transport( + args_, OrphanablePtr(mock_endpoint_), + /*is_client=*/false)); EXPECT_EQ(t->keepalive_time, Duration::Seconds(20)); EXPECT_EQ(t->keepalive_timeout, Duration::Seconds(10)); EXPECT_EQ(t->keepalive_permit_without_calls, true); @@ -129,8 +137,10 @@ TEST_F(ConfigurationTest, ModifyClientDefaults) { grpc_chttp2_config_default_keepalive_args(args, /*is_client=*/true); // Note that we are using the original args_ object for creating the transport // which does not override the defaults. - grpc_chttp2_transport* t = reinterpret_cast( - grpc_create_chttp2_transport(args_, mock_endpoint_, /*is_client=*/true)); + grpc_chttp2_transport* t = + reinterpret_cast(grpc_create_chttp2_transport( + args_, OrphanablePtr(mock_endpoint_), + /*is_client=*/true)); EXPECT_EQ(t->keepalive_time, Duration::Seconds(20)); EXPECT_EQ(t->keepalive_timeout, Duration::Seconds(10)); EXPECT_EQ(t->keepalive_permit_without_calls, true); @@ -154,8 +164,10 @@ TEST_F(ConfigurationTest, ModifyServerDefaults) { grpc_chttp2_config_default_keepalive_args(args, /*is_client=*/false); // Note that we are using the original args_ object for creating the transport // which does not override the defaults. - grpc_chttp2_transport* t = reinterpret_cast( - grpc_create_chttp2_transport(args_, mock_endpoint_, /*is_client=*/false)); + grpc_chttp2_transport* t = + reinterpret_cast(grpc_create_chttp2_transport( + args_, OrphanablePtr(mock_endpoint_), + /*is_client=*/false)); EXPECT_EQ(t->keepalive_time, Duration::Seconds(20)); EXPECT_EQ(t->keepalive_timeout, Duration::Seconds(10)); EXPECT_EQ(t->keepalive_permit_without_calls, true); diff --git a/test/core/transport/chttp2/streams_not_seen_test.cc b/test/core/transport/chttp2/streams_not_seen_test.cc index f00ca984e51c5..98b78b3d6283e 100644 --- a/test/core/transport/chttp2/streams_not_seen_test.cc +++ b/test/core/transport/chttp2/streams_not_seen_test.cc @@ -215,7 +215,7 @@ grpc_channel_filter TrailingMetadataRecordingFilter::kFilterVtable = { // connected channel filter must be the last one. // Channel init code falls back to lexical ordering of filters if there are // otherwise no dependencies, so we leverage that. - "zzzzzz_trailing-metadata-recording-filter", + GRPC_UNIQUE_TYPE_NAME_HERE("zzzzzz_trailing-metadata-recording-filter"), }; bool TrailingMetadataRecordingFilter::trailing_metadata_available_; absl::optional diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc index 136087b8dd058..d492d1f0aa6a9 100644 --- a/test/cpp/end2end/client_lb_end2end_test.cc +++ b/test/cpp/end2end/client_lb_end2end_test.cc @@ -580,22 +580,23 @@ class ClientLbEnd2endTest : public ::testing::Test { } static std::string MakeConnectionFailureRegex(absl::string_view prefix) { - return absl::StrCat(prefix, - "; last error: (UNKNOWN|UNAVAILABLE): " - // IP address - "(ipv6:%5B::1%5D|ipv4:127.0.0.1):[0-9]+: " - // Prefixes added for context - "(Failed to connect to remote host: )?" - "(Timeout occurred: )?" - // Syscall - "((connect|recvmsg|getsockopt\\(SO\\_ERROR\\)): ?)?" - // strerror() output or other message - "(Connection refused" - "|Connection reset by peer" - "|Socket closed" - "|FD shutdown)" - // errno value - "( \\([0-9]+\\))?"); + return absl::StrCat( + prefix, + "; last error: (UNKNOWN|UNAVAILABLE): " + // IP address + "(ipv6:%5B::1%5D|ipv4:127.0.0.1):[0-9]+: " + // Prefixes added for context + "(Failed to connect to remote host: )?" + "(Timeout occurred: )?" + // Syscall + "((connect|sendmsg|recvmsg|getsockopt\\(SO\\_ERROR\\)): ?)?" + // strerror() output or other message + "(Connection refused" + "|Connection reset by peer" + "|Socket closed" + "|FD shutdown)" + // errno value + "( \\([0-9]+\\))?"); } const std::string server_host_; diff --git a/test/cpp/end2end/resource_quota_end2end_stress_test.cc b/test/cpp/end2end/resource_quota_end2end_stress_test.cc index 2ba4ea9c9a075..9960bdff93daa 100644 --- a/test/cpp/end2end/resource_quota_end2end_stress_test.cc +++ b/test/cpp/end2end/resource_quota_end2end_stress_test.cc @@ -126,7 +126,7 @@ class End2EndResourceQuotaUnaryTest : public ::testing::Test { Status status; auto stub = EchoTestService::NewStub( CreateChannel(server_address_, grpc::InsecureChannelCredentials())); - ctx.set_wait_for_ready(true); + ctx.set_wait_for_ready(false); EchoClientUnaryReactor reactor(&ctx, stub.get(), payload_, &status); reactor.Await(); } diff --git a/test/cpp/end2end/xds/xds_end2end_test_lib.cc b/test/cpp/end2end/xds/xds_end2end_test_lib.cc index 2acf69ff1a00a..9865a3a88dea8 100644 --- a/test/cpp/end2end/xds/xds_end2end_test_lib.cc +++ b/test/cpp/end2end/xds/xds_end2end_test_lib.cc @@ -836,22 +836,23 @@ void XdsEnd2endTest::SetProtoDuration( std::string XdsEnd2endTest::MakeConnectionFailureRegex( absl::string_view prefix) { - return absl::StrCat(prefix, - "(UNKNOWN|UNAVAILABLE): " - // IP address - "(ipv6:%5B::1%5D|ipv4:127.0.0.1):[0-9]+: " - // Prefixes added for context - "(Failed to connect to remote host: )?" - "(Timeout occurred: )?" - // Syscall - "((connect|recvmsg|getsockopt\\(SO\\_ERROR\\)): ?)?" - // strerror() output or other message - "(Connection refused" - "|Connection reset by peer" - "|Socket closed" - "|FD shutdown)" - // errno value - "( \\([0-9]+\\))?"); + return absl::StrCat( + prefix, + "(UNKNOWN|UNAVAILABLE): " + // IP address + "(ipv6:%5B::1%5D|ipv4:127.0.0.1):[0-9]+: " + // Prefixes added for context + "(Failed to connect to remote host: )?" + "(Timeout occurred: )?" + // Syscall + "((connect|sendmsg|recvmsg|getsockopt\\(SO\\_ERROR\\)): ?)?" + // strerror() output or other message + "(Connection refused" + "|Connection reset by peer" + "|Socket closed" + "|FD shutdown)" + // errno value + "( \\([0-9]+\\))?"); } grpc_core::PemKeyCertPairList XdsEnd2endTest::ReadTlsIdentityPair( diff --git a/test/cpp/ext/otel/otel_test_library.cc b/test/cpp/ext/otel/otel_test_library.cc index 043d8c2dbc9ce..e16ff6b9980de 100644 --- a/test/cpp/ext/otel/otel_test_library.cc +++ b/test/cpp/ext/otel/otel_test_library.cc @@ -50,6 +50,8 @@ class AddLabelsFilter : public grpc_core::ChannelFilter { public: static const grpc_channel_filter kFilter; + static absl::string_view TypeName() { return "add_service_labels_filter"; } + explicit AddLabelsFilter( std::map @@ -85,8 +87,7 @@ class AddLabelsFilter : public grpc_core::ChannelFilter { const grpc_channel_filter AddLabelsFilter::kFilter = grpc_core::MakePromiseBasedFilter( - "add_service_labels_filter"); + grpc_core::FilterEndpoint::kClient>(); OpenTelemetryPluginEnd2EndTest::MetricsCollectorThread::MetricsCollectorThread( OpenTelemetryPluginEnd2EndTest* test, grpc_core::Duration interval, diff --git a/test/cpp/microbenchmarks/fullstack_fixtures.h b/test/cpp/microbenchmarks/fullstack_fixtures.h index 30f00e8854373..664fa70a25aa2 100644 --- a/test/cpp/microbenchmarks/fullstack_fixtures.h +++ b/test/cpp/microbenchmarks/fullstack_fixtures.h @@ -181,7 +181,9 @@ class EndpointPairFixture : public BaseFixture { grpc_core::Server::FromC(server_->c_server()); grpc_core::ChannelArgs server_args = core_server->channel_args(); server_transport_ = grpc_create_chttp2_transport( - server_args, endpoints.server, false /* is_client */); + server_args, + grpc_core::OrphanablePtr(endpoints.server), + /*is_client=*/false); for (grpc_pollset* pollset : core_server->pollsets()) { grpc_endpoint_add_to_pollset(endpoints.server, pollset); } @@ -207,8 +209,9 @@ class EndpointPairFixture : public BaseFixture { .channel_args_preconditioning() .PreconditionChannelArgs(&tmp_args); } - client_transport_ = - grpc_create_chttp2_transport(c_args, endpoints.client, true); + client_transport_ = grpc_create_chttp2_transport( + c_args, grpc_core::OrphanablePtr(endpoints.client), + /*is_client=*/true); CHECK(client_transport_); grpc_channel* channel = grpc_core::ChannelCreate("target", c_args, GRPC_CLIENT_DIRECT_CHANNEL, diff --git a/test/cpp/naming/cancel_ares_query_test.cc b/test/cpp/naming/cancel_ares_query_test.cc index 03d787d0b690f..cf8d813f62303 100644 --- a/test/cpp/naming/cancel_ares_query_test.cc +++ b/test/cpp/naming/cancel_ares_query_test.cc @@ -42,6 +42,7 @@ #include "src/core/lib/event_engine/default_event_engine.h" #include "src/core/lib/experiments/experiments.h" #include "src/core/lib/gprpp/crash.h" +#include "src/core/lib/gprpp/notification.h" #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/thd.h" #include "src/core/lib/gprpp/work_serializer.h" @@ -118,6 +119,10 @@ void ArgsInit(ArgsStruct* args) { void DoNothing(void* /*arg*/, grpc_error_handle /*error*/) {} void ArgsFinish(ArgsStruct* args) { + grpc_core::Notification notification; + args->lock->Run([¬ification]() { notification.Notify(); }, DEBUG_LOCATION); + args->lock.reset(); + notification.WaitForNotification(); grpc_pollset_set_del_pollset(args->pollset_set, args->pollset); grpc_pollset_set_destroy(args->pollset_set); grpc_closure DoNothing_cb; diff --git a/test/cpp/performance/writes_per_rpc_test.cc b/test/cpp/performance/writes_per_rpc_test.cc index ea37648f7dc8b..54a1d6faadf9b 100644 --- a/test/cpp/performance/writes_per_rpc_test.cc +++ b/test/cpp/performance/writes_per_rpc_test.cc @@ -17,6 +17,7 @@ // #include +#include #include @@ -118,14 +119,14 @@ class InProcessCHTTP2 { { grpc_core::Server* core_server = grpc_core::Server::FromC(server_->c_server()); - grpc_endpoint* iomgr_server_endpoint = - grpc_event_engine_endpoint_create(std::move(listener_endpoint)); - grpc_core::Transport* transport = grpc_create_chttp2_transport( - core_server->channel_args(), iomgr_server_endpoint, - /*is_client=*/false); + grpc_core::OrphanablePtr iomgr_server_endpoint( + grpc_event_engine_endpoint_create(std::move(listener_endpoint))); for (grpc_pollset* pollset : core_server->pollsets()) { - grpc_endpoint_add_to_pollset(iomgr_server_endpoint, pollset); + grpc_endpoint_add_to_pollset(iomgr_server_endpoint.get(), pollset); } + grpc_core::Transport* transport = grpc_create_chttp2_transport( + core_server->channel_args(), std::move(iomgr_server_endpoint), + /*is_client=*/false); CHECK(GRPC_LOG_IF_ERROR( "SetupTransport", core_server->SetupTransport(transport, nullptr, @@ -143,9 +144,10 @@ class InProcessCHTTP2 { args = args.Set(GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH, INT_MAX) .Set(GRPC_ARG_MAX_SEND_MESSAGE_LENGTH, INT_MAX) .Set(GRPC_ARG_HTTP2_BDP_PROBE, 0); + grpc_core::OrphanablePtr endpoint( + grpc_event_engine_endpoint_create(std::move(client_endpoint))); grpc_core::Transport* transport = grpc_create_chttp2_transport( - args, grpc_event_engine_endpoint_create(std::move(client_endpoint)), - /*is_client=*/true); + args, std::move(endpoint), /*is_client=*/true); CHECK(transport); grpc_channel* channel = grpc_core::ChannelCreate("target", args, GRPC_CLIENT_DIRECT_CHANNEL, diff --git a/tools/distrib/check_namespace_qualification.py b/tools/distrib/check_namespace_qualification.py index 51ccb71aeb828..f08f73c66fc2d 100755 --- a/tools/distrib/check_namespace_qualification.py +++ b/tools/distrib/check_namespace_qualification.py @@ -77,6 +77,7 @@ def check(self, fpath, fix): "src/core/lib/gprpp/global_config_env.h", "src/core/lib/profiling/timers.h", "src/core/lib/gprpp/crash.h", + "src/core/lib/gprpp/unique_type_name.h", # The grpc_core::Server redundant namespace qualification is required for # older gcc versions. "src/core/ext/transport/chttp2/server/chttp2_server.h", diff --git a/tools/release/verify_python_release.py b/tools/release/verify_python_release.py index 56729fee737d2..8eb0d66207687 100644 --- a/tools/release/verify_python_release.py +++ b/tools/release/verify_python_release.py @@ -45,6 +45,7 @@ "grpcio-admin", "grpcio-csds", "grpcio-observability", + "grpcio-csm-observability", "xds-protos", ]