From 53540ae5d610820c0959d7788a42c097ce38927f Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 31 May 2024 13:23:09 -0700 Subject: [PATCH 1/7] [context] Move legacy tracing contexts to arena contexts (#36776) Closes #36776 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36776 from ctiller:ctx2 8be4cdcf43f00a371dfd4ce0fb01594ecd5483a8 PiperOrigin-RevId: 639133808 --- CMakeLists.txt | 42 +++++++ build_autogenerated.yaml | 110 ++++++++++++++++++ src/core/BUILD | 2 + .../client_channel/client_channel_filter.cc | 36 +++--- .../client_channel/client_channel_filter.h | 12 +- .../load_balanced_call_destination.cc | 4 +- .../message_compress/compression_filter.cc | 8 +- .../chttp2/transport/chttp2_transport.cc | 43 ++++--- .../ext/transport/chttp2/transport/internal.h | 7 +- .../ext/transport/chttp2/transport/parsing.cc | 9 +- .../ext/transport/chttp2/transport/writing.cc | 8 +- src/core/lib/channel/context.h | 23 ---- src/core/lib/promise/cancel_callback.h | 10 ++ src/core/lib/surface/call.cc | 27 +++-- src/core/lib/surface/call.h | 4 + src/core/server/server_call_tracer_filter.cc | 15 +-- src/core/telemetry/call_tracer.cc | 46 +++----- src/core/telemetry/call_tracer.h | 16 ++- src/core/telemetry/metrics.cc | 9 +- src/core/telemetry/metrics.h | 4 +- src/cpp/ext/filters/census/client_filter.cc | 26 ++--- .../grpcio/grpc/_cython/_cygrpc/grpc.pxi | 13 +-- .../_cython/_cygrpc/observability.pyx.pxi | 4 +- test/core/telemetry/call_tracer_test.cc | 47 +++----- test/core/test_util/fake_stats_plugin.cc | 6 +- test/cpp/ext/otel/otel_test_library.cc | 4 +- 26 files changed, 322 insertions(+), 213 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6595ee1d494..984cb875e22 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -9128,7 +9128,43 @@ endif() if(gRPC_BUILD_TESTS) add_executable(cancel_callback_test + src/core/ext/upb-gen/google/protobuf/any.upb_minitable.c + src/core/ext/upb-gen/google/rpc/status.upb_minitable.c + src/core/lib/debug/trace.cc + src/core/lib/experiments/config.cc + src/core/lib/experiments/experiments.cc + src/core/lib/gprpp/status_helper.cc + src/core/lib/gprpp/time.cc + src/core/lib/iomgr/closure.cc + src/core/lib/iomgr/combiner.cc + src/core/lib/iomgr/error.cc + src/core/lib/iomgr/exec_ctx.cc + src/core/lib/iomgr/executor.cc + src/core/lib/iomgr/iomgr_internal.cc + src/core/lib/promise/activity.cc + src/core/lib/promise/trace.cc + src/core/lib/resource_quota/arena.cc + src/core/lib/resource_quota/connection_quota.cc + src/core/lib/resource_quota/memory_quota.cc + src/core/lib/resource_quota/periodic_update.cc + src/core/lib/resource_quota/resource_quota.cc + src/core/lib/resource_quota/thread_quota.cc + src/core/lib/resource_quota/trace.cc + src/core/lib/slice/percent_encoding.cc + src/core/lib/slice/slice.cc + src/core/lib/slice/slice_refcount.cc + src/core/lib/slice/slice_string_helpers.cc test/core/promise/cancel_callback_test.cc + third_party/upb/upb/mini_descriptor/build_enum.c + third_party/upb/upb/mini_descriptor/decode.c + third_party/upb/upb/mini_descriptor/internal/base92.c + third_party/upb/upb/mini_descriptor/internal/encode.c + third_party/upb/upb/mini_descriptor/link.c + third_party/upb/upb/wire/decode.c + third_party/upb/upb/wire/encode.c + third_party/upb/upb/wire/eps_copy_input_stream.c + third_party/upb/upb/wire/internal/decode_fast.c + third_party/upb/upb/wire/reader.c ) if(WIN32 AND MSVC) if(BUILD_SHARED_LIBS) @@ -9161,7 +9197,13 @@ target_include_directories(cancel_callback_test target_link_libraries(cancel_callback_test ${_gRPC_ALLTARGETS_LIBRARIES} gtest + utf8_range_lib + upb_message_lib + absl::config + absl::function_ref + absl::hash absl::type_traits + absl::statusor gpr ) diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 26b1d9d6beb..6c1a2db5f15 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -6974,14 +6974,124 @@ targets: build: test language: c++ headers: + - src/core/ext/upb-gen/google/protobuf/any.upb.h + - src/core/ext/upb-gen/google/protobuf/any.upb_minitable.h + - src/core/ext/upb-gen/google/rpc/status.upb.h + - src/core/ext/upb-gen/google/rpc/status.upb_minitable.h + - src/core/lib/debug/trace.h + - src/core/lib/event_engine/event_engine_context.h + - src/core/lib/experiments/config.h + - src/core/lib/experiments/experiments.h + - src/core/lib/gprpp/atomic_utils.h + - src/core/lib/gprpp/bitset.h + - src/core/lib/gprpp/cpp_impl_of.h + - src/core/lib/gprpp/down_cast.h + - src/core/lib/gprpp/manual_constructor.h + - src/core/lib/gprpp/orphanable.h + - src/core/lib/gprpp/ref_counted.h + - src/core/lib/gprpp/ref_counted_ptr.h + - src/core/lib/gprpp/status_helper.h + - src/core/lib/gprpp/time.h + - src/core/lib/iomgr/closure.h + - src/core/lib/iomgr/combiner.h + - src/core/lib/iomgr/error.h + - src/core/lib/iomgr/exec_ctx.h + - src/core/lib/iomgr/executor.h + - src/core/lib/iomgr/iomgr_internal.h + - src/core/lib/promise/activity.h - src/core/lib/promise/cancel_callback.h + - src/core/lib/promise/context.h + - src/core/lib/promise/detail/basic_seq.h + - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h + - src/core/lib/promise/detail/seq_state.h + - src/core/lib/promise/detail/status.h + - src/core/lib/promise/exec_ctx_wakeup_scheduler.h + - src/core/lib/promise/loop.h + - src/core/lib/promise/map.h - src/core/lib/promise/poll.h + - src/core/lib/promise/race.h + - src/core/lib/promise/seq.h + - src/core/lib/promise/trace.h + - src/core/lib/resource_quota/arena.h + - src/core/lib/resource_quota/connection_quota.h + - src/core/lib/resource_quota/memory_quota.h + - src/core/lib/resource_quota/periodic_update.h + - src/core/lib/resource_quota/resource_quota.h + - src/core/lib/resource_quota/thread_quota.h + - src/core/lib/resource_quota/trace.h + - src/core/lib/slice/percent_encoding.h + - src/core/lib/slice/slice.h + - src/core/lib/slice/slice_internal.h + - src/core/lib/slice/slice_refcount.h + - src/core/lib/slice/slice_string_helpers.h + - src/core/util/spinlock.h + - third_party/upb/upb/generated_code_support.h + - third_party/upb/upb/mini_descriptor/build_enum.h + - third_party/upb/upb/mini_descriptor/decode.h + - third_party/upb/upb/mini_descriptor/internal/base92.h + - third_party/upb/upb/mini_descriptor/internal/decoder.h + - third_party/upb/upb/mini_descriptor/internal/encode.h + - third_party/upb/upb/mini_descriptor/internal/encode.hpp + - third_party/upb/upb/mini_descriptor/internal/modifiers.h + - third_party/upb/upb/mini_descriptor/internal/wire_constants.h + - third_party/upb/upb/mini_descriptor/link.h + - third_party/upb/upb/wire/decode.h + - third_party/upb/upb/wire/encode.h + - third_party/upb/upb/wire/eps_copy_input_stream.h + - third_party/upb/upb/wire/internal/constants.h + - third_party/upb/upb/wire/internal/decode_fast.h + - third_party/upb/upb/wire/internal/decoder.h + - third_party/upb/upb/wire/internal/reader.h + - third_party/upb/upb/wire/reader.h + - third_party/upb/upb/wire/types.h src: + - src/core/ext/upb-gen/google/protobuf/any.upb_minitable.c + - src/core/ext/upb-gen/google/rpc/status.upb_minitable.c + - src/core/lib/debug/trace.cc + - src/core/lib/experiments/config.cc + - src/core/lib/experiments/experiments.cc + - src/core/lib/gprpp/status_helper.cc + - src/core/lib/gprpp/time.cc + - src/core/lib/iomgr/closure.cc + - src/core/lib/iomgr/combiner.cc + - src/core/lib/iomgr/error.cc + - src/core/lib/iomgr/exec_ctx.cc + - src/core/lib/iomgr/executor.cc + - src/core/lib/iomgr/iomgr_internal.cc + - src/core/lib/promise/activity.cc + - src/core/lib/promise/trace.cc + - src/core/lib/resource_quota/arena.cc + - src/core/lib/resource_quota/connection_quota.cc + - src/core/lib/resource_quota/memory_quota.cc + - src/core/lib/resource_quota/periodic_update.cc + - src/core/lib/resource_quota/resource_quota.cc + - src/core/lib/resource_quota/thread_quota.cc + - src/core/lib/resource_quota/trace.cc + - src/core/lib/slice/percent_encoding.cc + - src/core/lib/slice/slice.cc + - src/core/lib/slice/slice_refcount.cc + - src/core/lib/slice/slice_string_helpers.cc - test/core/promise/cancel_callback_test.cc + - third_party/upb/upb/mini_descriptor/build_enum.c + - third_party/upb/upb/mini_descriptor/decode.c + - third_party/upb/upb/mini_descriptor/internal/base92.c + - third_party/upb/upb/mini_descriptor/internal/encode.c + - third_party/upb/upb/mini_descriptor/link.c + - third_party/upb/upb/wire/decode.c + - third_party/upb/upb/wire/encode.c + - third_party/upb/upb/wire/eps_copy_input_stream.c + - third_party/upb/upb/wire/internal/decode_fast.c + - third_party/upb/upb/wire/reader.c deps: - gtest + - utf8_range_lib + - upb_message_lib + - absl/base:config + - absl/functional:function_ref + - absl/hash:hash - absl/meta:type_traits + - absl/status:statusor - gpr uses_polling: false - name: cancel_in_a_vacuum_test diff --git a/src/core/BUILD b/src/core/BUILD index 9d23ed2f1a4..42f6fdba028 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -685,6 +685,8 @@ grpc_cc_library( "lib/promise/cancel_callback.h", ], deps = [ + "arena", + "context", "promise_like", "//:gpr_platform", ], diff --git a/src/core/client_channel/client_channel_filter.cc b/src/core/client_channel/client_channel_filter.cc index f0c58c1580f..5eceb724703 100644 --- a/src/core/client_channel/client_channel_filter.cc +++ b/src/core/client_channel/client_channel_filter.cc @@ -2133,8 +2133,7 @@ absl::optional ClientChannelFilter::CallData::CheckResolution( } // If the call was queued, add trace annotation. if (was_queued) { - auto* call_tracer = static_cast( - call_context()[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value); + auto* call_tracer = arena()->GetContext(); if (call_tracer != nullptr) { call_tracer->RecordAnnotation("Delayed name resolution complete."); } @@ -2574,7 +2573,7 @@ class ClientChannelFilter::LoadBalancedCall::LbCallState final public: explicit LbCallState(LoadBalancedCall* lb_call) : lb_call_(lb_call) {} - void* Alloc(size_t size) override { return lb_call_->arena()->Alloc(size); } + void* Alloc(size_t size) override { return lb_call_->arena_->Alloc(size); } // Internal API to allow first-party LB policies to access per-call // attributes set by the ConfigSelector. @@ -2696,7 +2695,7 @@ class ClientChannelFilter::LoadBalancedCall::BackendMetricAccessor final recv_trailing_metadata_ != nullptr) { if (const auto* md = recv_trailing_metadata_->get_pointer( EndpointLoadMetricsBinMetadata())) { - BackendMetricAllocator allocator(lb_call_->arena()); + BackendMetricAllocator allocator(lb_call_->arena_); lb_call_->backend_metric_data_ = ParseBackendMetricData(md->as_string_view(), &allocator); } @@ -2731,28 +2730,29 @@ class ClientChannelFilter::LoadBalancedCall::BackendMetricAccessor final namespace { -void CreateCallAttemptTracer(grpc_call_context_element* context, - bool is_transparent_retry) { - auto* call_tracer = static_cast( - context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value); +void CreateCallAttemptTracer(Arena* arena, bool is_transparent_retry) { + auto* call_tracer = DownCast( + arena->GetContext()); if (call_tracer == nullptr) return; auto* tracer = call_tracer->StartNewAttempt(is_transparent_retry); - context[GRPC_CONTEXT_CALL_TRACER].value = tracer; + arena->SetContext(tracer); } } // namespace ClientChannelFilter::LoadBalancedCall::LoadBalancedCall( ClientChannelFilter* chand, grpc_call_context_element* call_context, - absl::AnyInvocable on_commit, bool is_transparent_retry) + Arena* arena, absl::AnyInvocable on_commit, + bool is_transparent_retry) : InternallyRefCounted( GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_lb_call_trace) ? "LoadBalancedCall" : nullptr), chand_(chand), on_commit_(std::move(on_commit)), - call_context_(call_context) { - CreateCallAttemptTracer(call_context, is_transparent_retry); + call_context_(call_context), + arena_(arena) { + CreateCallAttemptTracer(arena, is_transparent_retry); if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_lb_call_trace)) { gpr_log(GPR_INFO, "chand=%p lb_call=%p: created", chand_, this); } @@ -3019,9 +3019,8 @@ ClientChannelFilter::FilterBasedLoadBalancedCall::FilterBasedLoadBalancedCall( ClientChannelFilter* chand, const grpc_call_element_args& args, grpc_polling_entity* pollent, grpc_closure* on_call_destruction_complete, absl::AnyInvocable on_commit, bool is_transparent_retry) - : LoadBalancedCall(chand, args.context, std::move(on_commit), + : LoadBalancedCall(chand, args.context, args.arena, std::move(on_commit), is_transparent_retry), - arena_(args.arena), owning_call_(args.call_stack), call_combiner_(args.call_combiner), pollent_(pollent), @@ -3464,7 +3463,7 @@ void ClientChannelFilter::FilterBasedLoadBalancedCall::CreateSubchannelCall() { SubchannelCall::Args call_args = { connected_subchannel()->Ref(), pollent_, path->Ref(), /*start_time=*/0, static_cast(call_context()[GRPC_CONTEXT_CALL].value)->deadline(), - arena_, + arena(), // TODO(roth): When we implement hedging support, we will probably // need to use a separate call context for each subchannel call. call_context(), call_combiner_}; @@ -3494,7 +3493,8 @@ ClientChannelFilter::PromiseBasedLoadBalancedCall::PromiseBasedLoadBalancedCall( ClientChannelFilter* chand, absl::AnyInvocable on_commit, bool is_transparent_retry) : LoadBalancedCall(chand, GetContext(), - std::move(on_commit), is_transparent_retry) {} + GetContext(), std::move(on_commit), + is_transparent_retry) {} ArenaPromise ClientChannelFilter::PromiseBasedLoadBalancedCall::MakeCallPromise( @@ -3610,10 +3610,6 @@ ClientChannelFilter::PromiseBasedLoadBalancedCall::MakeCallPromise( }); } -Arena* ClientChannelFilter::PromiseBasedLoadBalancedCall::arena() const { - return GetContext(); -} - grpc_metadata_batch* ClientChannelFilter::PromiseBasedLoadBalancedCall::send_initial_metadata() const { diff --git a/src/core/client_channel/client_channel_filter.h b/src/core/client_channel/client_channel_filter.h index 9233c846c6f..e1c5e9aee25 100644 --- a/src/core/client_channel/client_channel_filter.h +++ b/src/core/client_channel/client_channel_filter.h @@ -372,7 +372,7 @@ class ClientChannelFilter::LoadBalancedCall : public InternallyRefCounted { public: LoadBalancedCall(ClientChannelFilter* chand, - grpc_call_context_element* call_context, + grpc_call_context_element* call_context, Arena* arena, absl::AnyInvocable on_commit, bool is_transparent_retry); ~LoadBalancedCall() override; @@ -391,8 +391,8 @@ class ClientChannelFilter::LoadBalancedCall protected: ClientChannelFilter* chand() const { return chand_; } ClientCallTracer::CallAttemptTracer* call_attempt_tracer() const { - return static_cast( - call_context_[GRPC_CONTEXT_CALL_TRACER].value); + return DownCast( + arena_->GetContext()); } ConnectedSubchannel* connected_subchannel() const { return connected_subchannel_.get(); @@ -401,6 +401,7 @@ class ClientChannelFilter::LoadBalancedCall lb_subchannel_call_tracker() const { return lb_subchannel_call_tracker_.get(); } + Arena* arena() const { return arena_; } void Commit() { auto on_commit = std::move(on_commit_); @@ -433,7 +434,6 @@ class ClientChannelFilter::LoadBalancedCall class Metadata; class BackendMetricAccessor; - virtual Arena* arena() const = 0; virtual grpc_polling_entity* pollent() = 0; virtual grpc_metadata_batch* send_initial_metadata() const = 0; @@ -460,6 +460,7 @@ class ClientChannelFilter::LoadBalancedCall std::unique_ptr lb_subchannel_call_tracker_; grpc_call_context_element* const call_context_; + Arena* const arena_; }; class ClientChannelFilter::FilterBasedLoadBalancedCall final @@ -495,7 +496,6 @@ class ClientChannelFilter::FilterBasedLoadBalancedCall final using LoadBalancedCall::chand; using LoadBalancedCall::Commit; - Arena* arena() const override { return arena_; } grpc_polling_entity* pollent() override { return pollent_; } grpc_metadata_batch* send_initial_metadata() const override { return pending_batches_[0] @@ -550,7 +550,6 @@ class ClientChannelFilter::FilterBasedLoadBalancedCall final // TODO(roth): Instead of duplicating these fields in every filter // that uses any one of them, we should store them in the call // context. This will save per-call memory overhead. - Arena* arena_; grpc_call_stack* owning_call_; CallCombiner* call_combiner_; grpc_polling_entity* pollent_; @@ -598,7 +597,6 @@ class ClientChannelFilter::PromiseBasedLoadBalancedCall final CallArgs call_args, OrphanablePtr lb_call); private: - Arena* arena() const override; grpc_polling_entity* pollent() override { return &pollent_; } grpc_metadata_batch* send_initial_metadata() const override; diff --git a/src/core/client_channel/load_balanced_call_destination.cc b/src/core/client_channel/load_balanced_call_destination.cc index 4d702a5858c..002a778e142 100644 --- a/src/core/client_channel/load_balanced_call_destination.cc +++ b/src/core/client_channel/load_balanced_call_destination.cc @@ -121,9 +121,7 @@ class LbCallState : public ClientChannelLbCallState { } ClientCallTracer::CallAttemptTracer* GetCallAttemptTracer() const override { - auto* legacy_context = GetContext(); - return static_cast( - legacy_context[GRPC_CONTEXT_CALL_TRACER].value); + return GetContext(); } }; 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 43fa643bbb8..96272570aa4 100644 --- a/src/core/ext/filters/http/message_compress/compression_filter.cc +++ b/src/core/ext/filters/http/message_compress/compression_filter.cc @@ -119,9 +119,7 @@ MessageHandle ChannelCompression::CompressMessage( gpr_log(GPR_INFO, "CompressMessage: len=%" PRIdPTR " alg=%d flags=%d", message->payload()->Length(), algorithm, message->flags()); } - auto* call_context = GetContext(); - auto* call_tracer = static_cast( - call_context[GRPC_CONTEXT_CALL_TRACER].value); + auto* call_tracer = MaybeGetContext(); if (call_tracer != nullptr) { call_tracer->RecordSendMessage(*message->payload()); } @@ -178,9 +176,7 @@ absl::StatusOr ChannelCompression::DecompressMessage( message->payload()->Length(), args.max_recv_message_length.value_or(-1), args.algorithm); } - auto* call_context = GetContext(); - auto* call_tracer = static_cast( - call_context[GRPC_CONTEXT_CALL_TRACER].value); + auto* call_tracer = MaybeGetContext(); if (call_tracer != nullptr) { call_tracer->RecordReceivedMessage(*message->payload()); } diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 90cfdf3ad5e..a20e4c71c0d 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -227,14 +227,13 @@ namespace { using TaskHandle = ::grpc_event_engine::experimental::EventEngine::TaskHandle; -grpc_core::CallTracerInterface* CallTracerIfSampled(grpc_chttp2_stream* s) { - if (s->context == nullptr || !grpc_core::IsTraceRecordCallopsEnabled()) { +grpc_core::CallTracerAnnotationInterface* CallTracerIfSampled( + grpc_chttp2_stream* s) { + if (!grpc_core::IsTraceRecordCallopsEnabled()) { return nullptr; } - auto* call_tracer = static_cast( - static_cast( - s->context)[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] - .value); + auto* call_tracer = + s->arena->GetContext(); if (call_tracer == nullptr || !call_tracer->IsSampled()) { return nullptr; } @@ -243,13 +242,11 @@ grpc_core::CallTracerInterface* CallTracerIfSampled(grpc_chttp2_stream* s) { std::shared_ptr TcpTracerIfSampled( grpc_chttp2_stream* s) { - if (s->context == nullptr || !grpc_core::IsTraceRecordCallopsEnabled()) { + if (!grpc_core::IsTraceRecordCallopsEnabled()) { return nullptr; } - auto* call_attempt_tracer = static_cast( - static_cast( - s->context)[GRPC_CONTEXT_CALL_TRACER] - .value); + auto* call_attempt_tracer = + s->arena->GetContext(); if (call_attempt_tracer == nullptr || !call_attempt_tracer->IsSampled()) { return nullptr; } @@ -391,10 +388,10 @@ grpc_chttp2_transport::~grpc_chttp2_transport() { grpc_error_handle error = GRPC_ERROR_CREATE("Transport destroyed"); // ContextList::Execute follows semantics of a callback function and does not // take a ref on error - if (cl != nullptr) { - grpc_core::ForEachContextListEntryExecute(cl, nullptr, error); + if (context_list != nullptr) { + grpc_core::ForEachContextListEntryExecute(context_list, nullptr, error); } - cl = nullptr; + context_list = nullptr; grpc_slice_buffer_destroy(&read_buffer); grpc_chttp2_goaway_parser_destroy(&goaway_parser); @@ -617,7 +614,7 @@ grpc_chttp2_transport::grpc_chttp2_transport( &memory_owner), deframe_state(is_client ? GRPC_DTS_FH_0 : GRPC_DTS_CLIENT_PREFIX_0), is_client(is_client) { - cl = new grpc_core::ContextList(); + context_list = new grpc_core::ContextList(); CHECK(strlen(GRPC_CHTTP2_CLIENT_CONNECT_STRING) == GRPC_CHTTP2_CLIENT_CONNECT_STRLEN); @@ -784,7 +781,8 @@ void grpc_chttp2_stream_unref(grpc_chttp2_stream* s) { grpc_chttp2_stream::grpc_chttp2_stream(grpc_chttp2_transport* t, grpc_stream_refcount* refcount, - const void* server_data) + const void* server_data, + grpc_core::Arena* arena) : t(t->Ref()), refcount([refcount]() { // We reserve one 'active stream' that's dropped when the stream is @@ -798,6 +796,7 @@ grpc_chttp2_stream::grpc_chttp2_stream(grpc_chttp2_transport* t, #endif return refcount; }()), + arena(arena), flow_control(&t->flow_control) { t->streams_allocated.fetch_add(1, std::memory_order_relaxed); if (server_data) { @@ -855,8 +854,8 @@ grpc_chttp2_stream::~grpc_chttp2_stream() { void grpc_chttp2_transport::InitStream(grpc_stream* gs, grpc_stream_refcount* refcount, const void* server_data, - grpc_core::Arena*) { - new (gs) grpc_chttp2_stream(this, refcount, server_data); + grpc_core::Arena* arena) { + new (gs) grpc_chttp2_stream(this, refcount, server_data, arena); } static void destroy_stream_locked(void* sp, grpc_error_handle /*error*/) { @@ -1015,13 +1014,13 @@ static void write_action_begin_locked( } static void write_action(grpc_chttp2_transport* t) { - void* cl = t->cl; - if (!t->cl->empty()) { + void* cl = t->context_list; + if (!t->context_list->empty()) { // Transfer the ownership of the context list to the endpoint and create and // associate a new context list with the transport. // The old context list is stored in the cl local variable which is passed // to the endpoint. Its upto the endpoint to manage its lifetime. - t->cl = new grpc_core::ContextList(); + t->context_list = new grpc_core::ContextList(); } else { // t->cl is Empty. There is nothing to trace in this endpoint_write. set cl // to nullptr. @@ -1376,7 +1375,7 @@ static void perform_stream_op_locked(void* stream_op, } if (op->send_initial_metadata) { - if (s->call_tracer) { + if (s->call_tracer != nullptr) { s->call_tracer->RecordAnnotation( grpc_core::HttpAnnotation(grpc_core::HttpAnnotation::Type::kStart, gpr_now(GPR_CLOCK_REALTIME)) diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index ed596278686..ea68725210d 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -466,7 +466,7 @@ struct grpc_chttp2_transport final : public grpc_core::FilterStackTransport, grpc_chttp2_keepalive_state keepalive_state; // Soft limit on max header size. uint32_t max_header_list_size_soft_limit = 0; - grpc_core::ContextList* cl = nullptr; + grpc_core::ContextList* context_list = nullptr; grpc_core::RefCountedPtr channelz_socket; uint32_t num_messages_in_next_write = 0; /// The number of pending induced frames (SETTINGS_ACK, PINGS_ACK and @@ -545,12 +545,13 @@ typedef enum { struct grpc_chttp2_stream { grpc_chttp2_stream(grpc_chttp2_transport* t, grpc_stream_refcount* refcount, - const void* server_data); + const void* server_data, grpc_core::Arena* arena); ~grpc_chttp2_stream(); void* context = nullptr; const grpc_core::RefCountedPtr t; grpc_stream_refcount* refcount; + grpc_core::Arena* const arena; grpc_closure destroy_stream; grpc_closure* destroy_stream_arg; @@ -643,7 +644,7 @@ struct grpc_chttp2_stream { int64_t write_counter = 0; /// Only set when enabled. - grpc_core::CallTracerInterface* call_tracer = nullptr; + grpc_core::CallTracerAnnotationInterface* call_tracer = nullptr; /// Only set when enabled. std::shared_ptr tcp_tracer; diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc index 856d3bcb220..8e81482d24d 100644 --- a/src/core/ext/transport/chttp2/transport/parsing.cc +++ b/src/core/ext/transport/chttp2/transport/parsing.cc @@ -942,13 +942,8 @@ grpc_error_handle grpc_chttp2_header_parser_parse(void* hpack_parser, grpc_core::CallTracerAnnotationInterface* call_tracer = nullptr; if (s != nullptr) { s->stats.incoming.header_bytes += GRPC_SLICE_LENGTH(slice); - - if (s->context != nullptr) { - call_tracer = static_cast( - static_cast( - s->context)[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] - .value); - } + call_tracer = + s->arena->GetContext(); } grpc_error_handle error = parser->Parse( slice, is_last != 0, absl::BitGenRef(t->bitgen), call_tracer); diff --git a/src/core/ext/transport/chttp2/transport/writing.cc b/src/core/ext/transport/chttp2/transport/writing.cc index 0cc28270314..a40d7c54359 100644 --- a/src/core/ext/transport/chttp2/transport/writing.cc +++ b/src/core/ext/transport/chttp2/transport/writing.cc @@ -682,10 +682,10 @@ grpc_chttp2_begin_write_result grpc_chttp2_begin_write( grpc_core::GrpcHttp2GetCopyContextFn(); if (copy_context_fn != nullptr && grpc_core::GrpcHttp2GetWriteTimestampsCallback() != nullptr) { - t->cl->emplace_back(copy_context_fn(s->context), - outbuf_relative_start_pos, num_stream_bytes, - s->byte_counter, s->write_counter - 1, - s->tcp_tracer); + t->context_list->emplace_back(copy_context_fn(s->context), + outbuf_relative_start_pos, + num_stream_bytes, s->byte_counter, + s->write_counter - 1, s->tcp_tracer); } } outbuf_relative_start_pos += num_stream_bytes; diff --git a/src/core/lib/channel/context.h b/src/core/lib/channel/context.h index 4e3cdae4bfd..100f3927def 100644 --- a/src/core/lib/channel/context.h +++ b/src/core/lib/channel/context.h @@ -35,16 +35,6 @@ typedef enum { /// Value is a \a census_context. GRPC_CONTEXT_TRACING, - /// Value is a CallTracerAnnotationInterface. (ClientCallTracer object on the - /// client-side call, or ServerCallTracer on the server-side.) - GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE, - - /// Value is a CallTracerInterface (ServerCallTracer on the server-side, - /// CallAttemptTracer on a subchannel call.) - /// TODO(yashykt): Maybe come up with a better name. This will go away in the - /// future anyway, so not super important. - GRPC_CONTEXT_CALL_TRACER, - /// Holds a pointer to ServiceConfigCallData associated with this call. GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA, @@ -62,8 +52,6 @@ struct grpc_call_context_element { namespace grpc_core { class Call; -class CallTracerAnnotationInterface; -class CallTracerInterface; class ServiceConfigCallData; // Bind the legacy context array into the new style structure @@ -82,17 +70,6 @@ struct OldStyleContext { static constexpr grpc_context_index kIndex = GRPC_CONTEXT_CALL; }; -template <> -struct OldStyleContext { - static constexpr grpc_context_index kIndex = - GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE; -}; - -template <> -struct OldStyleContext { - static constexpr grpc_context_index kIndex = GRPC_CONTEXT_CALL_TRACER; -}; - template <> struct OldStyleContext { static constexpr grpc_context_index kIndex = diff --git a/src/core/lib/promise/cancel_callback.h b/src/core/lib/promise/cancel_callback.h index f4f002b8f37..92ac2019d4f 100644 --- a/src/core/lib/promise/cancel_callback.h +++ b/src/core/lib/promise/cancel_callback.h @@ -17,7 +17,9 @@ #include +#include "src/core/lib/promise/context.h" #include "src/core/lib/promise/detail/promise_like.h" +#include "src/core/lib/resource_quota/arena.h" namespace grpc_core { @@ -31,6 +33,7 @@ class Handler { Handler& operator=(const Handler&) = delete; ~Handler() { if (!done_) { + promise_detail::Context ctx(arena_.get()); fn_(); } } @@ -48,6 +51,13 @@ class Handler { private: Fn fn_; + // Since cancellation happens at destruction time we need to either capture + // context here (via the arena), or make sure that no promise is destructed + // without an Arena context on the stack. The latter is an eternal game of + // whackamole, so we're choosing the former for now. + // TODO(ctiller): re-evaluate at some point in the future. + RefCountedPtr arena_ = + HasContext() ? GetContext()->Ref() : nullptr; bool done_ = false; }; diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 5f0d4a99310..b793bba7df9 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -755,7 +755,7 @@ grpc_error_handle FilterStackCall::Create(grpc_call_create_args* args, GrpcRegisteredMethod(), reinterpret_cast(static_cast( args->registered_method))); channel_stack->stats_plugin_group->AddClientCallTracers( - Slice(CSliceRef(path)), args->registered_method, call->context_); + Slice(CSliceRef(path)), args->registered_method, call->GetArena()); } else { global_stats().IncrementServerCallsCreated(); call->final_op_.server.cancelled = nullptr; @@ -778,12 +778,11 @@ grpc_error_handle FilterStackCall::Create(grpc_call_create_args* args, // GRPC_CONTEXT_CALL_TRACER as a matter of convenience. In the future // promise-based world, we would just a single tracer object for each // stack (call, subchannel_call, server_call.) - call->ContextSet(GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE, - server_call_tracer, nullptr); - call->ContextSet(GRPC_CONTEXT_CALL_TRACER, server_call_tracer, nullptr); + arena->SetContext(server_call_tracer); + arena->SetContext(server_call_tracer); } } - channel_stack->stats_plugin_group->AddServerCallTracers(call->context_); + channel_stack->stats_plugin_group->AddServerCallTracers(arena.get()); } Call* parent = Call::FromC(args->parent); @@ -1230,8 +1229,7 @@ FilterStackCall::BatchControl* FilterStackCall::ReuseOrAllocateBatchControl( *pslot = bctl; } bctl->call_ = this; - bctl->call_tracer_ = static_cast( - ContextGet(GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE)); + bctl->call_tracer_ = arena()->GetContext(); bctl->op_.payload = &stream_op_payload_; return bctl; } @@ -2612,7 +2610,7 @@ class ClientPromiseBasedCall final : public PromiseBasedCall { } ScopedContext context(this); args->channel->channel_stack()->stats_plugin_group->AddClientCallTracers( - *args->path, args->registered_method, this->context()); + *args->path, args->registered_method, GetArena()); send_initial_metadata_ = Arena::MakePooled(); send_initial_metadata_->Set(HttpPathMetadata(), std::move(*args->path)); if (args->authority.has_value()) { @@ -3762,6 +3760,19 @@ void* grpc_call_context_get(grpc_call* call, grpc_context_index elem) { return grpc_core::Call::FromC(call)->ContextGet(elem); } +void grpc_call_tracer_set(grpc_call* call, + grpc_core::ClientCallTracer* tracer) { + grpc_core::Arena* arena = grpc_call_get_arena(call); + return arena->SetContext(tracer); +} + +void* grpc_call_tracer_get(grpc_call* call) { + grpc_core::Arena* arena = grpc_call_get_arena(call); + auto* call_tracer = + arena->GetContext(); + return call_tracer; +} + uint8_t grpc_call_is_client(grpc_call* call) { return grpc_core::Call::FromC(call)->is_client(); } diff --git a/src/core/lib/surface/call.h b/src/core/lib/surface/call.h index d464de2ec89..0d557629388 100644 --- a/src/core/lib/surface/call.h +++ b/src/core/lib/surface/call.h @@ -338,6 +338,10 @@ void grpc_call_context_set(grpc_call* call, grpc_context_index elem, // Get a context pointer. void* grpc_call_context_get(grpc_call* call, grpc_context_index elem); +void grpc_call_tracer_set(grpc_call* call, grpc_core::ClientCallTracer* tracer); + +void* grpc_call_tracer_get(grpc_call* call); + #define GRPC_CALL_LOG_BATCH(sev, ops, nops) \ do { \ if (GRPC_TRACE_FLAG_ENABLED(grpc_api_trace)) { \ diff --git a/src/core/server/server_call_tracer_filter.cc b/src/core/server/server_call_tracer_filter.cc index 7a63b4768b6..efe05fc1f0c 100644 --- a/src/core/server/server_call_tracer_filter.cc +++ b/src/core/server/server_call_tracer_filter.cc @@ -56,25 +56,25 @@ class ServerCallTracerFilter class Call { public: void OnClientInitialMetadata(ClientMetadata& client_initial_metadata) { - auto* call_tracer = CallTracer(); + auto* call_tracer = MaybeGetContext(); if (call_tracer == nullptr) return; call_tracer->RecordReceivedInitialMetadata(&client_initial_metadata); } void OnServerInitialMetadata(ServerMetadata& server_initial_metadata) { - auto* call_tracer = CallTracer(); + auto* call_tracer = MaybeGetContext(); if (call_tracer == nullptr) return; call_tracer->RecordSendInitialMetadata(&server_initial_metadata); } void OnFinalize(const grpc_call_final_info* final_info) { - auto* call_tracer = CallTracer(); + auto* call_tracer = MaybeGetContext(); if (call_tracer == nullptr) return; call_tracer->RecordEnd(final_info); } void OnServerTrailingMetadata(ServerMetadata& server_trailing_metadata) { - auto* call_tracer = CallTracer(); + auto* call_tracer = MaybeGetContext(); if (call_tracer == nullptr) return; call_tracer->RecordSendTrailingMetadata(&server_trailing_metadata); } @@ -82,13 +82,6 @@ class ServerCallTracerFilter static const NoInterceptor OnClientToServerMessage; static const NoInterceptor OnClientToServerHalfClose; static const NoInterceptor OnServerToClientMessage; - - private: - static ServerCallTracer* CallTracer() { - auto* call_context = GetContext(); - return static_cast( - call_context[GRPC_CONTEXT_CALL_TRACER].value); - } }; }; diff --git a/src/core/telemetry/call_tracer.cc b/src/core/telemetry/call_tracer.cc index 093664e85f6..4b70f220395 100644 --- a/src/core/telemetry/call_tracer.cc +++ b/src/core/telemetry/call_tracer.cc @@ -299,63 +299,53 @@ class DelegatingServerCallTracer : public ServerCallTracer { std::vector tracers_; }; -void AddClientCallTracerToContext(grpc_call_context_element* call_context, - ClientCallTracer* tracer) { - if (call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value == - nullptr) { +void AddClientCallTracerToContext(Arena* arena, ClientCallTracer* tracer) { + if (arena->GetContext() == nullptr) { // This is the first call tracer. Set it directly. - call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value = tracer; - call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].destroy = - nullptr; + arena->SetContext(tracer); } else { // There was already a call tracer present. - auto* orig_tracer = static_cast( - call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value); + auto* orig_tracer = DownCast( + arena->GetContext()); if (orig_tracer->IsDelegatingTracer()) { // We already created a delegating tracer. Just add the new tracer to the // list. - static_cast(orig_tracer)->AddTracer(tracer); + DownCast(orig_tracer)->AddTracer(tracer); } else { // Create a new delegating tracer and add the first tracer and the new // tracer to the list. auto* delegating_tracer = GetContext()->ManagedNew( orig_tracer); - call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value = - delegating_tracer; + arena->SetContext(delegating_tracer); delegating_tracer->AddTracer(tracer); } } } -void AddServerCallTracerToContext(grpc_call_context_element* call_context, - ServerCallTracer* tracer) { - DCHECK(call_context[GRPC_CONTEXT_CALL_TRACER].value == - call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value); - if (call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value == - nullptr) { +void AddServerCallTracerToContext(Arena* arena, ServerCallTracer* tracer) { + DCHECK_EQ(arena->GetContext(), + arena->GetContext()); + if (arena->GetContext() == nullptr) { // This is the first call tracer. Set it directly. - call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value = tracer; - call_context[GRPC_CONTEXT_CALL_TRACER].value = tracer; - call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].destroy = - nullptr; + arena->SetContext(tracer); + arena->SetContext(tracer); } else { // There was already a call tracer present. - auto* orig_tracer = static_cast( - call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value); + auto* orig_tracer = DownCast( + arena->GetContext()); if (orig_tracer->IsDelegatingTracer()) { // We already created a delegating tracer. Just add the new tracer to the // list. - static_cast(orig_tracer)->AddTracer(tracer); + DownCast(orig_tracer)->AddTracer(tracer); } else { // Create a new delegating tracer and add the first tracer and the new // tracer to the list. auto* delegating_tracer = GetContext()->ManagedNew( orig_tracer); - call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value = - delegating_tracer; - call_context[GRPC_CONTEXT_CALL_TRACER].value = delegating_tracer; + arena->SetContext(delegating_tracer); + arena->SetContext(delegating_tracer); delegating_tracer->AddTracer(tracer); } } diff --git a/src/core/telemetry/call_tracer.h b/src/core/telemetry/call_tracer.h index 967658fa36b..1053777d85d 100644 --- a/src/core/telemetry/call_tracer.h +++ b/src/core/telemetry/call_tracer.h @@ -214,13 +214,21 @@ class ServerCallTracerFactory { // Convenience functions to add call tracers to a call context. Allows setting // multiple call tracers to a single call. It is only valid to add client call // tracers before the client_channel filter sees the send_initial_metadata op. -void AddClientCallTracerToContext(grpc_call_context_element* call_context, - ClientCallTracer* tracer); +void AddClientCallTracerToContext(Arena* arena, ClientCallTracer* tracer); // TODO(yashykt): We want server call tracers to be registered through the // ServerCallTracerFactory, which has yet to be made into a list. -void AddServerCallTracerToContext(grpc_call_context_element* call_context, - ServerCallTracer* tracer); +void AddServerCallTracerToContext(Arena* arena, ServerCallTracer* tracer); + +template <> +struct ArenaContextType { + static void Destroy(CallTracerAnnotationInterface*) {} +}; + +template <> +struct ArenaContextType { + static void Destroy(CallTracerAnnotationInterface*) {} +}; template <> struct ContextSubclass { diff --git a/src/core/telemetry/metrics.cc b/src/core/telemetry/metrics.cc index b403a8649f2..db47f1f8608 100644 --- a/src/core/telemetry/metrics.cc +++ b/src/core/telemetry/metrics.cc @@ -100,23 +100,22 @@ RegisteredMetricCallback::~RegisteredMetricCallback() { } void GlobalStatsPluginRegistry::StatsPluginGroup::AddClientCallTracers( - const Slice& path, bool registered_method, - grpc_call_context_element* call_context) { + const Slice& path, bool registered_method, Arena* arena) { for (auto& state : plugins_state_) { auto* call_tracer = state.plugin->GetClientCallTracer( path, registered_method, state.scope_config); if (call_tracer != nullptr) { - AddClientCallTracerToContext(call_context, call_tracer); + AddClientCallTracerToContext(arena, call_tracer); } } } void GlobalStatsPluginRegistry::StatsPluginGroup::AddServerCallTracers( - grpc_call_context_element* call_context) { + Arena* arena) { for (auto& state : plugins_state_) { auto* call_tracer = state.plugin->GetServerCallTracer(state.scope_config); if (call_tracer != nullptr) { - AddServerCallTracerToContext(call_context, call_tracer); + AddServerCallTracerToContext(arena, call_tracer); } } } diff --git a/src/core/telemetry/metrics.h b/src/core/telemetry/metrics.h index 2fb92d4597e..8551b74bc62 100644 --- a/src/core/telemetry/metrics.h +++ b/src/core/telemetry/metrics.h @@ -464,10 +464,10 @@ class GlobalStatsPluginRegistry { // Adds all available client call tracers associated with the stats plugins // within the group to \a call_context. void AddClientCallTracers(const Slice& path, bool registered_method, - grpc_call_context_element* call_context); + Arena* arena); // Adds all available server call tracers associated with the stats plugins // within the group to \a call_context. - void AddServerCallTracers(grpc_call_context_element* call_context); + void AddServerCallTracers(Arena* arena); private: friend class RegisteredMetricCallback; diff --git a/src/cpp/ext/filters/census/client_filter.cc b/src/cpp/ext/filters/census/client_filter.cc index efb191da95d..4733040f152 100644 --- a/src/cpp/ext/filters/census/client_filter.cc +++ b/src/cpp/ext/filters/census/client_filter.cc @@ -100,17 +100,15 @@ OpenCensusClientFilter::MakeCallPromise( grpc_core::NextPromiseFactory next_promise_factory) { auto* path = call_args.client_initial_metadata->get_pointer( grpc_core::HttpPathMetadata()); - auto* call_context = grpc_core::GetContext(); - auto* tracer = - grpc_core::GetContext() - ->ManagedNew( - call_context, path != nullptr ? path->Ref() : grpc_core::Slice(), - grpc_core::GetContext(), - OpenCensusTracingEnabled() && tracing_enabled_); - DCHECK(call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value == - nullptr); - call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value = tracer; - call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].destroy = nullptr; + auto* arena = grpc_core::GetContext(); + auto* tracer = arena->ManagedNew( + grpc_core::GetContext(), + path != nullptr ? path->Ref() : grpc_core::Slice(), + grpc_core::GetContext(), + OpenCensusTracingEnabled() && tracing_enabled_); + DCHECK_EQ(arena->GetContext(), + nullptr); + grpc_core::SetContext(tracer); return next_promise_factory(std::move(call_args)); } @@ -424,9 +422,9 @@ class OpenCensusClientInterceptor : public grpc::experimental::Interceptor { grpc::experimental::InterceptorBatchMethods* methods) override { if (methods->QueryInterceptionHookPoint( grpc::experimental::InterceptionHookPoints::POST_RECV_STATUS)) { - auto* tracer = static_cast( - grpc_call_context_get(info_->client_context()->c_call(), - GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE)); + auto* tracer = grpc_core::DownCast( + grpc_call_get_arena(info_->client_context()->c_call()) + ->GetContext()); if (tracer != nullptr) { tracer->RecordApiLatency(absl::Now() - start_time_, static_cast( diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/grpc.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/grpc.pxi index 5f5afcfc944..e1467aeb565 100644 --- a/src/python/grpcio/grpc/_cython/_cygrpc/grpc.pxi +++ b/src/python/grpcio/grpc/_cython/_cygrpc/grpc.pxi @@ -63,6 +63,9 @@ cdef extern from "src/core/telemetry/call_tracer.h" namespace "grpc_core": cdef cppclass ClientCallTracer: pass + cdef cppclass CallTracerAnnotationInterface: + pass + cdef cppclass ServerCallTracer: string TraceId() nogil string SpanId() nogil @@ -72,14 +75,10 @@ cdef extern from "src/core/telemetry/call_tracer.h" namespace "grpc_core": @staticmethod void RegisterGlobal(ServerCallTracerFactory* factory) nogil -cdef extern from "src/core/lib/channel/context.h": - ctypedef enum grpc_context_index: - GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE - cdef extern from "src/core/lib/surface/call.h": - void grpc_call_context_set(grpc_call* call, grpc_context_index elem, - void* value, void (*destroy)(void* value)) nogil - void *grpc_call_context_get(grpc_call* call, grpc_context_index elem) nogil + void grpc_call_tracer_set(grpc_call* call, void* value) nogil + + void* grpc_call_tracer_get(grpc_call* call) nogil cdef extern from "grpc/support/alloc.h": diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/observability.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/observability.pyx.pxi index a29ccdd9376..5f62a0efe64 100644 --- a/src/python/grpcio/grpc/_cython/_cygrpc/observability.pyx.pxi +++ b/src/python/grpcio/grpc/_cython/_cygrpc/observability.pyx.pxi @@ -50,11 +50,11 @@ def maybe_save_server_trace_context(RequestCallEvent event) -> None: cdef void _set_call_tracer(grpc_call* call, void* capsule_ptr): cdef ClientCallTracer* call_tracer = capsule_ptr - grpc_call_context_set(call, GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE, call_tracer, NULL) + grpc_call_tracer_set(call, call_tracer) cdef void* _get_call_tracer(grpc_call* call): - cdef void* call_tracer = grpc_call_context_get(call, GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE) + cdef void* call_tracer = grpc_call_tracer_get(call) return call_tracer diff --git a/test/core/telemetry/call_tracer_test.cc b/test/core/telemetry/call_tracer_test.cc index 3adbc5d6204..c9af4d745b4 100644 --- a/test/core/telemetry/call_tracer_test.cc +++ b/test/core/telemetry/call_tracer_test.cc @@ -38,16 +38,13 @@ namespace { class CallTracerTest : public ::testing::Test { protected: RefCountedPtr arena_ = SimpleArenaAllocator()->MakeArena(); - grpc_call_context_element context_[GRPC_CONTEXT_COUNT] = {}; std::vector annotation_logger_; }; TEST_F(CallTracerTest, BasicClientCallTracer) { FakeClientCallTracer client_call_tracer(&annotation_logger_); - AddClientCallTracerToContext(context_, &client_call_tracer); - static_cast( - context_[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value) - ->RecordAnnotation("Test"); + AddClientCallTracerToContext(arena_.get(), &client_call_tracer); + arena_->GetContext()->RecordAnnotation("Test"); EXPECT_EQ(annotation_logger_, std::vector{"Test"}); } @@ -56,12 +53,10 @@ TEST_F(CallTracerTest, MultipleClientCallTracers) { FakeClientCallTracer client_call_tracer1(&annotation_logger_); FakeClientCallTracer client_call_tracer2(&annotation_logger_); FakeClientCallTracer client_call_tracer3(&annotation_logger_); - AddClientCallTracerToContext(context_, &client_call_tracer1); - AddClientCallTracerToContext(context_, &client_call_tracer2); - AddClientCallTracerToContext(context_, &client_call_tracer3); - static_cast( - context_[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value) - ->RecordAnnotation("Test"); + AddClientCallTracerToContext(arena_.get(), &client_call_tracer1); + AddClientCallTracerToContext(arena_.get(), &client_call_tracer2); + AddClientCallTracerToContext(arena_.get(), &client_call_tracer3); + arena_->GetContext()->RecordAnnotation("Test"); EXPECT_EQ(annotation_logger_, std::vector({"Test", "Test", "Test"})); } @@ -71,12 +66,12 @@ TEST_F(CallTracerTest, MultipleClientCallAttemptTracers) { FakeClientCallTracer client_call_tracer1(&annotation_logger_); FakeClientCallTracer client_call_tracer2(&annotation_logger_); FakeClientCallTracer client_call_tracer3(&annotation_logger_); - AddClientCallTracerToContext(context_, &client_call_tracer1); - AddClientCallTracerToContext(context_, &client_call_tracer2); - AddClientCallTracerToContext(context_, &client_call_tracer3); + AddClientCallTracerToContext(arena_.get(), &client_call_tracer1); + AddClientCallTracerToContext(arena_.get(), &client_call_tracer2); + AddClientCallTracerToContext(arena_.get(), &client_call_tracer3); auto* attempt_tracer = - static_cast( - context_[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value) + DownCast( + arena_->GetContext()) ->StartNewAttempt(true /* is_transparent_retry */); attempt_tracer->RecordAnnotation("Test"); EXPECT_EQ(annotation_logger_, @@ -86,13 +81,9 @@ TEST_F(CallTracerTest, MultipleClientCallAttemptTracers) { TEST_F(CallTracerTest, BasicServerCallTracerTest) { FakeServerCallTracer server_call_tracer(&annotation_logger_); - AddServerCallTracerToContext(context_, &server_call_tracer); - static_cast( - context_[GRPC_CONTEXT_CALL_TRACER].value) - ->RecordAnnotation("Test"); - static_cast( - context_[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value) - ->RecordAnnotation("Test"); + AddServerCallTracerToContext(arena_.get(), &server_call_tracer); + arena_->GetContext()->RecordAnnotation("Test"); + arena_->GetContext()->RecordAnnotation("Test"); EXPECT_EQ(annotation_logger_, std::vector({"Test", "Test"})); } @@ -101,12 +92,10 @@ TEST_F(CallTracerTest, MultipleServerCallTracers) { FakeServerCallTracer server_call_tracer1(&annotation_logger_); FakeServerCallTracer server_call_tracer2(&annotation_logger_); FakeServerCallTracer server_call_tracer3(&annotation_logger_); - AddServerCallTracerToContext(context_, &server_call_tracer1); - AddServerCallTracerToContext(context_, &server_call_tracer2); - AddServerCallTracerToContext(context_, &server_call_tracer3); - static_cast( - context_[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value) - ->RecordAnnotation("Test"); + AddServerCallTracerToContext(arena_.get(), &server_call_tracer1); + AddServerCallTracerToContext(arena_.get(), &server_call_tracer2); + AddServerCallTracerToContext(arena_.get(), &server_call_tracer3); + arena_->GetContext()->RecordAnnotation("Test"); EXPECT_EQ(annotation_logger_, std::vector({"Test", "Test", "Test"})); } diff --git a/test/core/test_util/fake_stats_plugin.cc b/test/core/test_util/fake_stats_plugin.cc index abc0a6a82ff..59546f9ce45 100644 --- a/test/core/test_util/fake_stats_plugin.cc +++ b/test/core/test_util/fake_stats_plugin.cc @@ -57,11 +57,7 @@ ArenaPromise FakeStatsClientFilter::MakeCallPromise( FakeClientCallTracer* client_call_tracer = fake_client_call_tracer_factory_->CreateFakeClientCallTracer(); if (client_call_tracer != nullptr) { - auto* call_context = GetContext(); - call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value = - client_call_tracer; - call_context[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].destroy = - nullptr; + SetContext(client_call_tracer); } return next_promise_factory(std::move(call_args)); } diff --git a/test/cpp/ext/otel/otel_test_library.cc b/test/cpp/ext/otel/otel_test_library.cc index 40153f13dda..338527903b4 100644 --- a/test/cpp/ext/otel/otel_test_library.cc +++ b/test/cpp/ext/otel/otel_test_library.cc @@ -68,9 +68,7 @@ class AddLabelsFilter : public grpc_core::ChannelFilter { grpc_core::CallArgs call_args, grpc_core::NextPromiseFactory next_promise_factory) override { using CallAttemptTracer = grpc_core::ClientCallTracer::CallAttemptTracer; - auto* call_context = grpc_core::GetContext(); - auto* call_tracer = static_cast( - call_context[GRPC_CONTEXT_CALL_TRACER].value); + auto* call_tracer = grpc_core::GetContext(); EXPECT_NE(call_tracer, nullptr); for (const auto& pair : labels_to_inject_) { call_tracer->SetOptionalLabel(pair.first, pair.second); From 34ac4ee5deff36acbe7e61737acda30851465b9e Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 31 May 2024 14:08:30 -0700 Subject: [PATCH 2/7] [grpc_error] remove unnecessary status attributes (#36523) The following attributes were completely unused: - kOffset - kIndex - kSize - kFilename - kKey - kValue The following attributes were added but never programmatically accessed, and I've moved them into the status messages themselves, which is another step toward #22883: - kErrorNo - kTsiCode - kWsaError - kHttpStatus - kOsError - kSyscall - kTargetAddress - kRawBytes - kTsiError Closes #36523 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36523 from markdroth:grpc_error_attribute_cleanup b289c399fed67322ac4f3de24b5cec5e81663886 PiperOrigin-RevId: 639147583 --- BUILD | 2 - CMakeLists.txt | 3 - Makefile | 1 - Package.swift | 2 - build_autogenerated.yaml | 6 -- config.m4 | 1 - config.w32 | 1 - gRPC-C++.podspec | 2 - gRPC-Core.podspec | 3 - grpc.gemspec | 2 - package.xml | 2 - src/core/BUILD | 1 + .../chttp2/transport/chttp2_transport.cc | 37 +++++------ .../handshaker/security/secure_endpoint.cc | 9 +-- .../security/security_handshaker.cc | 39 +++++------ src/core/handshaker/security/tsi_error.cc | 31 --------- src/core/handshaker/security/tsi_error.h | 30 --------- .../posix_engine/posix_endpoint.cc | 18 ++--- .../posix_engine/posix_endpoint.h | 2 +- .../event_engine/windows/windows_listener.cc | 8 +-- src/core/lib/gprpp/status_helper.cc | 41 ++++-------- src/core/lib/gprpp/status_helper.h | 34 +--------- src/core/lib/iomgr/endpoint_cfstream.cc | 16 ++--- src/core/lib/iomgr/error.cc | 28 ++++---- src/core/lib/iomgr/resolve_address_posix.cc | 17 ++--- .../lib/iomgr/socket_utils_common_posix.cc | 15 ++--- src/core/lib/iomgr/tcp_client_posix.cc | 11 +--- src/core/lib/iomgr/tcp_client_windows.cc | 6 +- src/core/lib/iomgr/tcp_posix.cc | 22 ++----- src/core/lib/iomgr/tcp_server_windows.cc | 11 +--- .../google_default_credentials.cc | 6 +- .../resolver/dns/c_ares/grpc_ares_wrapper.cc | 7 +- src/core/util/http_client/httpcli.cc | 7 +- src/python/grpcio/grpc_core_dependencies.py | 1 - test/core/gprpp/status_helper_test.cc | 32 ++++----- test/core/iomgr/error_test.cc | 66 +++++++------------ test/core/tsi/transport_security_test_lib.cc | 7 +- test/cpp/end2end/client_lb_end2end_test.cc | 16 +++-- test/cpp/end2end/xds/xds_end2end_test_lib.cc | 24 ++++--- tools/doxygen/Doxyfile.c++.internal | 2 - tools/doxygen/Doxyfile.core.internal | 2 - 41 files changed, 180 insertions(+), 391 deletions(-) delete mode 100644 src/core/handshaker/security/tsi_error.cc delete mode 100644 src/core/handshaker/security/tsi_error.h diff --git a/BUILD b/BUILD index f2caa63ed0d..2bc4c344fc7 100644 --- a/BUILD +++ b/BUILD @@ -2295,7 +2295,6 @@ grpc_cc_library( srcs = [ "//src/core:handshaker/security/secure_endpoint.cc", "//src/core:handshaker/security/security_handshaker.cc", - "//src/core:handshaker/security/tsi_error.cc", "//src/core:lib/security/context/security_context.cc", "//src/core:lib/security/credentials/call_creds_util.cc", "//src/core:lib/security/credentials/composite/composite_credentials.cc", @@ -2308,7 +2307,6 @@ grpc_cc_library( hdrs = [ "//src/core:handshaker/security/secure_endpoint.h", "//src/core:handshaker/security/security_handshaker.h", - "//src/core:handshaker/security/tsi_error.h", "//src/core:lib/security/context/security_context.h", "//src/core:lib/security/credentials/call_creds_util.h", "//src/core:lib/security/credentials/composite/composite_credentials.h", diff --git a/CMakeLists.txt b/CMakeLists.txt index 984cb875e22..88ea4bce529 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2233,7 +2233,6 @@ add_library(grpc src/core/handshaker/proxy_mapper_registry.cc src/core/handshaker/security/secure_endpoint.cc src/core/handshaker/security/security_handshaker.cc - src/core/handshaker/security/tsi_error.cc src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc src/core/lib/address_utils/parse_address.cc src/core/lib/address_utils/sockaddr_utils.cc @@ -3029,7 +3028,6 @@ add_library(grpc_unsecure src/core/handshaker/proxy_mapper_registry.cc src/core/handshaker/security/secure_endpoint.cc src/core/handshaker/security/security_handshaker.cc - src/core/handshaker/security/tsi_error.cc src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc src/core/lib/address_utils/parse_address.cc src/core/lib/address_utils/sockaddr_utils.cc @@ -5150,7 +5148,6 @@ add_library(grpc_authorization_provider src/core/handshaker/proxy_mapper_registry.cc src/core/handshaker/security/secure_endpoint.cc src/core/handshaker/security/security_handshaker.cc - src/core/handshaker/security/tsi_error.cc src/core/lib/address_utils/parse_address.cc src/core/lib/address_utils/sockaddr_utils.cc src/core/lib/backoff/backoff.cc diff --git a/Makefile b/Makefile index fc3701d0415..27a67eab1ef 100644 --- a/Makefile +++ b/Makefile @@ -1064,7 +1064,6 @@ LIBGRPC_SRC = \ src/core/handshaker/proxy_mapper_registry.cc \ src/core/handshaker/security/secure_endpoint.cc \ src/core/handshaker/security/security_handshaker.cc \ - src/core/handshaker/security/tsi_error.cc \ src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc \ src/core/lib/address_utils/parse_address.cc \ src/core/lib/address_utils/sockaddr_utils.cc \ diff --git a/Package.swift b/Package.swift index 6b411d73c85..f9aabb060b5 100644 --- a/Package.swift +++ b/Package.swift @@ -1082,8 +1082,6 @@ let package = Package( "src/core/handshaker/security/secure_endpoint.h", "src/core/handshaker/security/security_handshaker.cc", "src/core/handshaker/security/security_handshaker.h", - "src/core/handshaker/security/tsi_error.cc", - "src/core/handshaker/security/tsi_error.h", "src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc", "src/core/handshaker/tcp_connect/tcp_connect_handshaker.h", "src/core/lib/address_utils/parse_address.cc", diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 6c1a2db5f15..d10a96a9b1e 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -789,7 +789,6 @@ libs: - src/core/handshaker/proxy_mapper_registry.h - src/core/handshaker/security/secure_endpoint.h - src/core/handshaker/security/security_handshaker.h - - src/core/handshaker/security/tsi_error.h - src/core/handshaker/tcp_connect/tcp_connect_handshaker.h - src/core/lib/address_utils/parse_address.h - src/core/lib/address_utils/sockaddr_utils.h @@ -1649,7 +1648,6 @@ libs: - src/core/handshaker/proxy_mapper_registry.cc - src/core/handshaker/security/secure_endpoint.cc - src/core/handshaker/security/security_handshaker.cc - - src/core/handshaker/security/tsi_error.cc - src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc - src/core/lib/address_utils/parse_address.cc - src/core/lib/address_utils/sockaddr_utils.cc @@ -2334,7 +2332,6 @@ libs: - src/core/handshaker/proxy_mapper_registry.h - src/core/handshaker/security/secure_endpoint.h - src/core/handshaker/security/security_handshaker.h - - src/core/handshaker/security/tsi_error.h - src/core/handshaker/tcp_connect/tcp_connect_handshaker.h - src/core/lib/address_utils/parse_address.h - src/core/lib/address_utils/sockaddr_utils.h @@ -2809,7 +2806,6 @@ libs: - src/core/handshaker/proxy_mapper_registry.cc - src/core/handshaker/security/secure_endpoint.cc - src/core/handshaker/security/security_handshaker.cc - - src/core/handshaker/security/tsi_error.cc - src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc - src/core/lib/address_utils/parse_address.cc - src/core/lib/address_utils/sockaddr_utils.cc @@ -4431,7 +4427,6 @@ libs: - src/core/handshaker/proxy_mapper_registry.h - src/core/handshaker/security/secure_endpoint.h - src/core/handshaker/security/security_handshaker.h - - src/core/handshaker/security/tsi_error.h - src/core/lib/address_utils/parse_address.h - src/core/lib/address_utils/sockaddr_utils.h - src/core/lib/avl/avl.h @@ -4781,7 +4776,6 @@ libs: - src/core/handshaker/proxy_mapper_registry.cc - src/core/handshaker/security/secure_endpoint.cc - src/core/handshaker/security/security_handshaker.cc - - src/core/handshaker/security/tsi_error.cc - src/core/lib/address_utils/parse_address.cc - src/core/lib/address_utils/sockaddr_utils.cc - src/core/lib/backoff/backoff.cc diff --git a/config.m4 b/config.m4 index 695063579c5..deff264db93 100644 --- a/config.m4 +++ b/config.m4 @@ -439,7 +439,6 @@ if test "$PHP_GRPC" != "no"; then src/core/handshaker/proxy_mapper_registry.cc \ src/core/handshaker/security/secure_endpoint.cc \ src/core/handshaker/security/security_handshaker.cc \ - src/core/handshaker/security/tsi_error.cc \ src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc \ src/core/lib/address_utils/parse_address.cc \ src/core/lib/address_utils/sockaddr_utils.cc \ diff --git a/config.w32 b/config.w32 index 85a32dc6704..6a05a877cf8 100644 --- a/config.w32 +++ b/config.w32 @@ -404,7 +404,6 @@ if (PHP_GRPC != "no") { "src\\core\\handshaker\\proxy_mapper_registry.cc " + "src\\core\\handshaker\\security\\secure_endpoint.cc " + "src\\core\\handshaker\\security\\security_handshaker.cc " + - "src\\core\\handshaker\\security\\tsi_error.cc " + "src\\core\\handshaker\\tcp_connect\\tcp_connect_handshaker.cc " + "src\\core\\lib\\address_utils\\parse_address.cc " + "src\\core\\lib\\address_utils\\sockaddr_utils.cc " + diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 90a2f3849fc..75ed86a5b98 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -868,7 +868,6 @@ Pod::Spec.new do |s| 'src/core/handshaker/proxy_mapper_registry.h', 'src/core/handshaker/security/secure_endpoint.h', 'src/core/handshaker/security/security_handshaker.h', - 'src/core/handshaker/security/tsi_error.h', 'src/core/handshaker/tcp_connect/tcp_connect_handshaker.h', 'src/core/lib/address_utils/parse_address.h', 'src/core/lib/address_utils/sockaddr_utils.h', @@ -2143,7 +2142,6 @@ Pod::Spec.new do |s| 'src/core/handshaker/proxy_mapper_registry.h', 'src/core/handshaker/security/secure_endpoint.h', 'src/core/handshaker/security/security_handshaker.h', - 'src/core/handshaker/security/tsi_error.h', 'src/core/handshaker/tcp_connect/tcp_connect_handshaker.h', 'src/core/lib/address_utils/parse_address.h', 'src/core/lib/address_utils/sockaddr_utils.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index a8cc8fdda1f..8a7878202a4 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -1201,8 +1201,6 @@ Pod::Spec.new do |s| 'src/core/handshaker/security/secure_endpoint.h', 'src/core/handshaker/security/security_handshaker.cc', 'src/core/handshaker/security/security_handshaker.h', - 'src/core/handshaker/security/tsi_error.cc', - 'src/core/handshaker/security/tsi_error.h', 'src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc', 'src/core/handshaker/tcp_connect/tcp_connect_handshaker.h', 'src/core/lib/address_utils/parse_address.cc', @@ -2926,7 +2924,6 @@ Pod::Spec.new do |s| 'src/core/handshaker/proxy_mapper_registry.h', 'src/core/handshaker/security/secure_endpoint.h', 'src/core/handshaker/security/security_handshaker.h', - 'src/core/handshaker/security/tsi_error.h', 'src/core/handshaker/tcp_connect/tcp_connect_handshaker.h', 'src/core/lib/address_utils/parse_address.h', 'src/core/lib/address_utils/sockaddr_utils.h', diff --git a/grpc.gemspec b/grpc.gemspec index 5b919662d11..15b48f8cb0e 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -1088,8 +1088,6 @@ Gem::Specification.new do |s| s.files += %w( src/core/handshaker/security/secure_endpoint.h ) s.files += %w( src/core/handshaker/security/security_handshaker.cc ) s.files += %w( src/core/handshaker/security/security_handshaker.h ) - s.files += %w( src/core/handshaker/security/tsi_error.cc ) - s.files += %w( src/core/handshaker/security/tsi_error.h ) s.files += %w( src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc ) s.files += %w( src/core/handshaker/tcp_connect/tcp_connect_handshaker.h ) s.files += %w( src/core/lib/address_utils/parse_address.cc ) diff --git a/package.xml b/package.xml index d1778bf0607..ffe1342e92b 100644 --- a/package.xml +++ b/package.xml @@ -1070,8 +1070,6 @@ - - diff --git a/src/core/BUILD b/src/core/BUILD index 42f6fdba028..4b84aaac004 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -1655,6 +1655,7 @@ grpc_cc_library( "absl/log:check", "absl/log:log", "absl/status", + "absl/strings", "absl/strings:str_format", ], visibility = ["@grpc:alt_grpc_base_legacy"], diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index a20e4c71c0d..5de056f1b7b 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -1122,19 +1122,16 @@ void grpc_chttp2_add_incoming_goaway(grpc_chttp2_transport* t, uint32_t goaway_error, uint32_t last_stream_id, absl::string_view goaway_text) { - t->goaway_error = grpc_error_set_str( + t->goaway_error = grpc_error_set_int( grpc_error_set_int( - grpc_error_set_int( - grpc_core::StatusCreate( - absl::StatusCode::kUnavailable, - absl::StrFormat( - "GOAWAY received; Error code: %u; Debug Text: %s", - goaway_error, goaway_text), - DEBUG_LOCATION, {}), - grpc_core::StatusIntProperty::kHttp2Error, - static_cast(goaway_error)), - grpc_core::StatusIntProperty::kRpcStatus, GRPC_STATUS_UNAVAILABLE), - grpc_core::StatusStrProperty::kRawBytes, goaway_text); + grpc_core::StatusCreate( + absl::StatusCode::kUnavailable, + absl::StrFormat("GOAWAY received; Error code: %u; Debug Text: %s", + goaway_error, goaway_text), + DEBUG_LOCATION, {}), + grpc_core::StatusIntProperty::kHttp2Error, + static_cast(goaway_error)), + grpc_core::StatusIntProperty::kRpcStatus, GRPC_STATUS_UNAVAILABLE); GRPC_CHTTP2_IF_TRACING( gpr_log(GPR_INFO, "transport %p got goaway with last stream id %d", t, @@ -1292,12 +1289,10 @@ void grpc_chttp2_complete_closure_step(grpc_chttp2_transport* t, if (cl_err.ok()) { cl_err = GRPC_ERROR_CREATE(absl::StrCat( "Error in HTTP transport completing operation: ", desc, - " write_state=", write_state_name(t->write_state), " refs=", - closure->next_data.scratch / CLOSURE_BARRIER_FIRST_REF_BIT, " flags=", - closure->next_data.scratch % CLOSURE_BARRIER_FIRST_REF_BIT)); - cl_err = grpc_error_set_str(cl_err, - grpc_core::StatusStrProperty::kTargetAddress, - std::string(t->peer_string.as_string_view())); + " write_state=", write_state_name(t->write_state), + " refs=", closure->next_data.scratch / CLOSURE_BARRIER_FIRST_REF_BIT, + " flags=", closure->next_data.scratch % CLOSURE_BARRIER_FIRST_REF_BIT, + " peer_address=", t->peer_string.as_string_view())); } cl_err = grpc_error_add_child(cl_err, error); closure->error_data.error = grpc_core::internal::StatusAllocHeapPtr(cl_err); @@ -2612,9 +2607,9 @@ static grpc_error_handle try_http_parsing(grpc_chttp2_transport* t) { if (parse_error.ok() && (parse_error = grpc_http_parser_eof(&parser)) == absl::OkStatus()) { error = grpc_error_set_int( - grpc_error_set_int( - GRPC_ERROR_CREATE("Trying to connect an http1.x server"), - grpc_core::StatusIntProperty::kHttpStatus, response.status), + GRPC_ERROR_CREATE( + absl::StrCat("Trying to connect an http1.x server (HTTP status ", + response.status, ")")), grpc_core::StatusIntProperty::kRpcStatus, grpc_http2_status_to_grpc_status(response.status)); } diff --git a/src/core/handshaker/security/secure_endpoint.cc b/src/core/handshaker/security/secure_endpoint.cc index 922b111cab4..37955049106 100644 --- a/src/core/handshaker/security/secure_endpoint.cc +++ b/src/core/handshaker/security/secure_endpoint.cc @@ -40,7 +40,6 @@ #include #include -#include "src/core/handshaker/security/tsi_error.h" #include "src/core/lib/debug/trace.h" #include "src/core/lib/gprpp/debug_location.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" @@ -340,8 +339,9 @@ static void on_read(void* user_data, grpc_error_handle error) { if (result != TSI_OK) { grpc_slice_buffer_reset_and_unref(ep->read_buffer); - call_read_cb(ep, grpc_set_tsi_error_result( - GRPC_ERROR_CREATE("Unwrap failed"), result)); + call_read_cb( + ep, GRPC_ERROR_CREATE(absl::StrCat("Unwrap failed (", + tsi_result_to_string(result), ")"))); return; } @@ -484,7 +484,8 @@ static void endpoint_write(grpc_endpoint* secure_ep, grpc_slice_buffer* slices, grpc_slice_buffer_reset_and_unref(&ep->output_buffer); grpc_core::ExecCtx::Run( DEBUG_LOCATION, cb, - grpc_set_tsi_error_result(GRPC_ERROR_CREATE("Wrap failed"), result)); + GRPC_ERROR_CREATE( + absl::StrCat("Wrap failed (", tsi_result_to_string(result), ")"))); return; } diff --git a/src/core/handshaker/security/security_handshaker.cc b/src/core/handshaker/security/security_handshaker.cc index bf073a66561..e9da5697977 100644 --- a/src/core/handshaker/security/security_handshaker.cc +++ b/src/core/handshaker/security/security_handshaker.cc @@ -48,7 +48,6 @@ #include "src/core/handshaker/handshaker_factory.h" #include "src/core/handshaker/handshaker_registry.h" #include "src/core/handshaker/security/secure_endpoint.h" -#include "src/core/handshaker/security/tsi_error.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/gprpp/debug_location.h" @@ -260,10 +259,9 @@ void SecurityHandshaker::OnPeerCheckedInner(grpc_error_handle error) { tsi_result result = tsi_handshaker_result_get_unused_bytes( handshaker_result_, &unused_bytes, &unused_bytes_size); if (result != TSI_OK) { - HandshakeFailedLocked(grpc_set_tsi_error_result( - GRPC_ERROR_CREATE( - "TSI handshaker result does not provide unused bytes"), - result)); + HandshakeFailedLocked(GRPC_ERROR_CREATE( + absl::StrCat("TSI handshaker result does not provide unused bytes (", + tsi_result_to_string(result), ")"))); return; } // Check whether we need to wrap the endpoint. @@ -271,10 +269,10 @@ void SecurityHandshaker::OnPeerCheckedInner(grpc_error_handle error) { result = tsi_handshaker_result_get_frame_protector_type( handshaker_result_, &frame_protector_type); if (result != TSI_OK) { - HandshakeFailedLocked(grpc_set_tsi_error_result( - GRPC_ERROR_CREATE("TSI handshaker result does not implement " - "get_frame_protector_type"), - result)); + HandshakeFailedLocked(GRPC_ERROR_CREATE( + absl::StrCat("TSI handshaker result does not implement " + "get_frame_protector_type (", + tsi_result_to_string(result), ")"))); return; } tsi_zero_copy_grpc_protector* zero_copy_protector = nullptr; @@ -288,9 +286,9 @@ void SecurityHandshaker::OnPeerCheckedInner(grpc_error_handle error) { handshaker_result_, max_frame_size_ == 0 ? nullptr : &max_frame_size_, &zero_copy_protector); if (result != TSI_OK) { - HandshakeFailedLocked(grpc_set_tsi_error_result( - GRPC_ERROR_CREATE("Zero-copy frame protector creation failed"), - result)); + HandshakeFailedLocked(GRPC_ERROR_CREATE( + absl::StrCat("Zero-copy frame protector creation failed (", + tsi_result_to_string(result), ")"))); return; } break; @@ -300,8 +298,9 @@ void SecurityHandshaker::OnPeerCheckedInner(grpc_error_handle error) { handshaker_result_, max_frame_size_ == 0 ? nullptr : &max_frame_size_, &protector); if (result != TSI_OK) { - HandshakeFailedLocked(grpc_set_tsi_error_result( - GRPC_ERROR_CREATE("Frame protector creation failed"), result)); + HandshakeFailedLocked( + GRPC_ERROR_CREATE(absl::StrCat("Frame protector creation failed (", + tsi_result_to_string(result), ")"))); return; } break; @@ -356,8 +355,8 @@ grpc_error_handle SecurityHandshaker::CheckPeerLocked() { tsi_result result = tsi_handshaker_result_extract_peer(handshaker_result_, &peer); if (result != TSI_OK) { - return grpc_set_tsi_error_result( - GRPC_ERROR_CREATE("Peer extraction failed"), result); + 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_); @@ -398,11 +397,9 @@ grpc_error_handle SecurityHandshaker::OnHandshakeNextDoneLocked( if (security_connector != nullptr) { connector_type = security_connector->type().name(); } - return grpc_set_tsi_error_result( - GRPC_ERROR_CREATE(absl::StrCat( - connector_type, " handshake failed", - (tsi_handshake_error_.empty() ? "" : ": "), tsi_handshake_error_)), - result); + return GRPC_ERROR_CREATE(absl::StrCat( + connector_type, " handshake failed (", tsi_result_to_string(result), + ")", (tsi_handshake_error_.empty() ? "" : ": "), tsi_handshake_error_)); } // Update handshaker result. if (handshaker_result != nullptr) { diff --git a/src/core/handshaker/security/tsi_error.cc b/src/core/handshaker/security/tsi_error.cc deleted file mode 100644 index 06b816df6ee..00000000000 --- a/src/core/handshaker/security/tsi_error.cc +++ /dev/null @@ -1,31 +0,0 @@ -// -// -// Copyright 2015 gRPC authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// -// - -#include "src/core/handshaker/security/tsi_error.h" - -#include - -#include "src/core/lib/gprpp/status_helper.h" - -grpc_error_handle grpc_set_tsi_error_result(grpc_error_handle error, - tsi_result result) { - return grpc_error_set_int( - grpc_error_set_str(error, grpc_core::StatusStrProperty::kTsiError, - tsi_result_to_string(result)), - grpc_core::StatusIntProperty::kTsiCode, result); -} diff --git a/src/core/handshaker/security/tsi_error.h b/src/core/handshaker/security/tsi_error.h deleted file mode 100644 index f1a57dc78ab..00000000000 --- a/src/core/handshaker/security/tsi_error.h +++ /dev/null @@ -1,30 +0,0 @@ -// -// -// Copyright 2015 gRPC authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// -// - -#ifndef GRPC_SRC_CORE_HANDSHAKER_SECURITY_TSI_ERROR_H -#define GRPC_SRC_CORE_HANDSHAKER_SECURITY_TSI_ERROR_H - -#include - -#include "src/core/lib/iomgr/error.h" -#include "src/core/tsi/transport_security_interface.h" - -grpc_error_handle grpc_set_tsi_error_result(grpc_error_handle error, - tsi_result result); - -#endif // GRPC_SRC_CORE_HANDSHAKER_SECURITY_TSI_ERROR_H diff --git a/src/core/lib/event_engine/posix_engine/posix_endpoint.cc b/src/core/lib/event_engine/posix_engine/posix_endpoint.cc index 18d7dfb4623..2ddb38d49d1 100644 --- a/src/core/lib/event_engine/posix_engine/posix_endpoint.cc +++ b/src/core/lib/event_engine/posix_engine/posix_endpoint.cc @@ -212,14 +212,9 @@ bool CmsgIsZeroCopy(const cmsghdr& cmsg) { } #endif // GRPC_LINUX_ERRQUEUE -absl::Status PosixOSError(int error_no, const char* call_name) { - absl::Status s = absl::UnknownError(grpc_core::StrError(error_no)); - grpc_core::StatusSetInt(&s, grpc_core::StatusIntProperty::kErrorNo, error_no); - grpc_core::StatusSetStr(&s, grpc_core::StatusStrProperty::kOsError, - grpc_core::StrError(error_no)); - grpc_core::StatusSetStr(&s, grpc_core::StatusStrProperty::kSyscall, - call_name); - return s; +absl::Status PosixOSError(int error_no, absl::string_view call_name) { + return absl::UnknownError(absl::StrCat( + call_name, ": ", grpc_core::StrError(error_no), " (", error_no, ")")); } } // namespace @@ -282,12 +277,7 @@ void PosixEndpointImpl::FinishEstimate() { bytes_read_this_round_ = 0; } -absl::Status PosixEndpointImpl::TcpAnnotateError(absl::Status src_error) { - auto peer_string = ResolvedAddressToNormalizedString(peer_address_); - - grpc_core::StatusSetStr(&src_error, - grpc_core::StatusStrProperty::kTargetAddress, - peer_string.ok() ? *peer_string : ""); +absl::Status PosixEndpointImpl::TcpAnnotateError(absl::Status src_error) const { grpc_core::StatusSetInt(&src_error, grpc_core::StatusIntProperty::kFd, handle_->WrappedFd()); grpc_core::StatusSetInt(&src_error, grpc_core::StatusIntProperty::kRpcStatus, diff --git a/src/core/lib/event_engine/posix_engine/posix_endpoint.h b/src/core/lib/event_engine/posix_engine/posix_endpoint.h index 9a0ca6b8db4..3e919857d28 100644 --- a/src/core/lib/event_engine/posix_engine/posix_endpoint.h +++ b/src/core/lib/event_engine/posix_engine/posix_endpoint.h @@ -525,7 +525,7 @@ class PosixEndpointImpl : public grpc_core::RefCounted { bool WriteWithTimestamps(struct msghdr* msg, size_t sending_length, ssize_t* sent_length, int* saved_errno, int additional_flags); - absl::Status TcpAnnotateError(absl::Status src_error); + absl::Status TcpAnnotateError(absl::Status src_error) const; #ifdef GRPC_LINUX_ERRQUEUE bool ProcessErrors(); // Reads a cmsg to process zerocopy control messages. diff --git a/src/core/lib/event_engine/windows/windows_listener.cc b/src/core/lib/event_engine/windows/windows_listener.cc index 848258aca96..fd0f375c869 100644 --- a/src/core/lib/event_engine/windows/windows_listener.cc +++ b/src/core/lib/event_engine/windows/windows_listener.cc @@ -267,13 +267,9 @@ WindowsEventEngineListener::SinglePortSocketListener::PrepareListenerSocket( SOCKET sock, const EventEngine::ResolvedAddress& addr) { auto fail = [&](absl::Status error) -> absl::Status { CHECK(!error.ok()); - auto addr_uri = ResolvedAddressToURI(addr); error = grpc_error_set_int( - grpc_error_set_str( - GRPC_ERROR_CREATE_REFERENCING("Failed to prepare server socket", - &error, 1), - grpc_core::StatusStrProperty::kTargetAddress, - addr_uri.ok() ? *addr_uri : addr_uri.status().ToString()), + GRPC_ERROR_CREATE_REFERENCING("Failed to prepare server socket", &error, + 1), grpc_core::StatusIntProperty::kFd, static_cast(sock)); if (sock != INVALID_SOCKET) closesocket(sock); return error; diff --git a/src/core/lib/gprpp/status_helper.cc b/src/core/lib/gprpp/status_helper.cc index fe76efb87a3..40db6dc0cfc 100644 --- a/src/core/lib/gprpp/status_helper.cc +++ b/src/core/lib/gprpp/status_helper.cc @@ -60,30 +60,16 @@ const absl::string_view kChildrenPropertyUrl = TYPE_URL(TYPE_CHILDREN_TAG); const char* GetStatusIntPropertyUrl(StatusIntProperty key) { switch (key) { - case StatusIntProperty::kErrorNo: - return TYPE_URL(TYPE_INT_TAG "errno"); case StatusIntProperty::kFileLine: return TYPE_URL(TYPE_INT_TAG "file_line"); case StatusIntProperty::kStreamId: return TYPE_URL(TYPE_INT_TAG "stream_id"); case StatusIntProperty::kRpcStatus: return TYPE_URL(TYPE_INT_TAG "grpc_status"); - case StatusIntProperty::kOffset: - return TYPE_URL(TYPE_INT_TAG "offset"); - case StatusIntProperty::kIndex: - return TYPE_URL(TYPE_INT_TAG "index"); - case StatusIntProperty::kSize: - return TYPE_URL(TYPE_INT_TAG "size"); case StatusIntProperty::kHttp2Error: return TYPE_URL(TYPE_INT_TAG "http2_error"); - case StatusIntProperty::kTsiCode: - return TYPE_URL(TYPE_INT_TAG "tsi_code"); - case StatusIntProperty::kWsaError: - return TYPE_URL(TYPE_INT_TAG "wsa_error"); case StatusIntProperty::kFd: return TYPE_URL(TYPE_INT_TAG "fd"); - case StatusIntProperty::kHttpStatus: - return TYPE_URL(TYPE_INT_TAG "http_status"); case StatusIntProperty::kOccurredDuringWrite: return TYPE_URL(TYPE_INT_TAG "occurred_during_write"); case StatusIntProperty::ChannelConnectivityState: @@ -100,24 +86,8 @@ const char* GetStatusStrPropertyUrl(StatusStrProperty key) { return TYPE_URL(TYPE_STR_TAG "description"); case StatusStrProperty::kFile: return TYPE_URL(TYPE_STR_TAG "file"); - case StatusStrProperty::kOsError: - return TYPE_URL(TYPE_STR_TAG "os_error"); - case StatusStrProperty::kSyscall: - return TYPE_URL(TYPE_STR_TAG "syscall"); - case StatusStrProperty::kTargetAddress: - return TYPE_URL(TYPE_STR_TAG "target_address"); case StatusStrProperty::kGrpcMessage: return TYPE_URL(TYPE_STR_TAG "grpc_message"); - case StatusStrProperty::kRawBytes: - return TYPE_URL(TYPE_STR_TAG "raw_bytes"); - case StatusStrProperty::kTsiError: - return TYPE_URL(TYPE_STR_TAG "tsi_error"); - case StatusStrProperty::kFilename: - return TYPE_URL(TYPE_STR_TAG "filename"); - case StatusStrProperty::kKey: - return TYPE_URL(TYPE_STR_TAG "key"); - case StatusStrProperty::kValue: - return TYPE_URL(TYPE_STR_TAG "value"); } GPR_UNREACHABLE_CODE(return "unknown"); } @@ -348,6 +318,17 @@ std::string StatusToString(const absl::Status& status) { : absl::StrCat(head, " {", absl::StrJoin(kvs, ", "), "}"); } +absl::Status AddMessagePrefix(absl::string_view prefix, absl::Status status) { + absl::Status new_status(status.code(), + absl::StrCat(prefix, ": ", status.message())); + // TODO(roth): Remove this once we elimiate all status attributes. + status.ForEachPayload( + [&](absl::string_view type_url, const absl::Cord& payload) { + new_status.SetPayload(type_url, payload); + }); + return new_status; +} + namespace internal { google_rpc_Status* StatusToProto(const absl::Status& status, upb_Arena* arena) { diff --git a/src/core/lib/gprpp/status_helper.h b/src/core/lib/gprpp/status_helper.h index 1f001948796..3f561d21b9b 100644 --- a/src/core/lib/gprpp/status_helper.h +++ b/src/core/lib/gprpp/status_helper.h @@ -48,8 +48,6 @@ namespace grpc_core { /// This enum should have the same value of grpc_error_ints enum class StatusIntProperty { - /// 'errno' from the operating system - kErrorNo, /// __LINE__ from the call site creating the error kFileLine, /// stream identifier: for errors that are associated with an individual @@ -58,23 +56,10 @@ enum class StatusIntProperty { /// grpc status code representing this error // TODO(veblush): Remove this after grpc_error is replaced with absl::Status kRpcStatus, - /// offset into some binary blob (usually represented by - /// RAW_BYTES) where the error occurred - kOffset, - /// context sensitive index associated with the error - kIndex, - /// context sensitive size associated with the error - kSize, /// http2 error code associated with the error (see the HTTP2 RFC) kHttp2Error, - /// TSI status code associated with the error - kTsiCode, - /// WSAGetLastError() reported when this error occurred - kWsaError, /// File descriptor associated with this error kFd, - /// HTTP status (i.e. 404) - kHttpStatus, /// chttp2: did the error occur while a write was in progress kOccurredDuringWrite, /// channel connectivity state associated with the error @@ -89,24 +74,8 @@ enum class StatusStrProperty { kDescription, /// source file in which this error occurred kFile, - /// operating system description of this error - kOsError, - /// syscall that generated this error - kSyscall, /// peer that we were trying to communicate when this error occurred - kTargetAddress, - /// grpc status message associated with this error kGrpcMessage, - /// hex dump (or similar) with the data that generated this error - kRawBytes, - /// tsi error string associated with this error - kTsiError, - /// filename that we were trying to read/write when this error occurred - kFilename, - /// key associated with the error - kKey, - /// value associated with the error - kValue, }; /// This enum should have the same value of grpc_error_times @@ -158,6 +127,9 @@ GRPC_MUST_USE_RESULT std::vector StatusGetChildren( /// CANCELLATION:SampleMessage {errno:'2021', line:'54', children:[ABORTED]} GRPC_MUST_USE_RESULT std::string StatusToString(const absl::Status& status); +/// Adds prefix to the message of status. +absl::Status AddMessagePrefix(absl::string_view prefix, absl::Status status); + namespace internal { /// Builds a upb message, google_rpc_Status from a status diff --git a/src/core/lib/iomgr/endpoint_cfstream.cc b/src/core/lib/iomgr/endpoint_cfstream.cc index 631f310a2b4..86d1e6e0ea5 100644 --- a/src/core/lib/iomgr/endpoint_cfstream.cc +++ b/src/core/lib/iomgr/endpoint_cfstream.cc @@ -106,12 +106,9 @@ static void CFStreamUnref(CFStreamEndpoint* ep) { static void CFStreamRef(CFStreamEndpoint* ep) { gpr_ref(&ep->refcount); } #endif -static grpc_error_handle CFStreamAnnotateError(grpc_error_handle src_error, - CFStreamEndpoint* ep) { - return grpc_error_set_str( - grpc_error_set_int(src_error, grpc_core::StatusIntProperty::kRpcStatus, - GRPC_STATUS_UNAVAILABLE), - grpc_core::StatusStrProperty::kTargetAddress, ep->peer_string); +static grpc_error_handle CFStreamAnnotateError(grpc_error_handle src_error) { + return grpc_error_set_int(src_error, grpc_core::StatusIntProperty::kRpcStatus, + GRPC_STATUS_UNAVAILABLE); } static void CallReadCb(CFStreamEndpoint* ep, grpc_error_handle error) { @@ -170,7 +167,7 @@ static void ReadAction(void* arg, grpc_error_handle error) { CFErrorRef stream_error = CFReadStreamCopyError(ep->read_stream); if (stream_error != nullptr) { error = CFStreamAnnotateError( - GRPC_ERROR_CREATE_FROM_CFERROR(stream_error, "Read error"), ep); + GRPC_ERROR_CREATE_FROM_CFERROR(stream_error, "Read error")); CFRelease(stream_error); } else { error = GRPC_ERROR_CREATE("Read error"); @@ -179,8 +176,7 @@ static void ReadAction(void* arg, grpc_error_handle error) { EP_UNREF(ep, "read"); } else if (read_size == 0) { grpc_slice_buffer_reset_and_unref(ep->read_slices); - CallReadCb(ep, - CFStreamAnnotateError(GRPC_ERROR_CREATE("Socket closed"), ep)); + CallReadCb(ep, CFStreamAnnotateError(GRPC_ERROR_CREATE("Socket closed"))); EP_UNREF(ep, "read"); } else { if (read_size < static_cast(len)) { @@ -209,7 +205,7 @@ static void WriteAction(void* arg, grpc_error_handle error) { CFErrorRef stream_error = CFWriteStreamCopyError(ep->write_stream); if (stream_error != nullptr) { error = CFStreamAnnotateError( - GRPC_ERROR_CREATE_FROM_CFERROR(stream_error, "write failed."), ep); + GRPC_ERROR_CREATE_FROM_CFERROR(stream_error, "Write failed")); CFRelease(stream_error); } else { error = GRPC_ERROR_CREATE("write failed."); diff --git a/src/core/lib/iomgr/error.cc b/src/core/lib/iomgr/error.cc index b070a764fb3..b0d72a503fe 100644 --- a/src/core/lib/iomgr/error.cc +++ b/src/core/lib/iomgr/error.cc @@ -21,6 +21,7 @@ #include #include "absl/log/check.h" +#include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" #include @@ -58,15 +59,10 @@ absl::Status grpc_status_create(absl::StatusCode code, absl::string_view msg, absl::Status grpc_os_error(const grpc_core::DebugLocation& location, int err, const char* call_name) { - auto err_string = grpc_core::StrError(err); - absl::Status s = - StatusCreate(absl::StatusCode::kUnknown, err_string, location, {}); - grpc_core::StatusSetInt(&s, grpc_core::StatusIntProperty::kErrorNo, err); - grpc_core::StatusSetStr(&s, grpc_core::StatusStrProperty::kOsError, - err_string); - grpc_core::StatusSetStr(&s, grpc_core::StatusStrProperty::kSyscall, - call_name); - return s; + return StatusCreate( + absl::StatusCode::kUnknown, + absl::StrCat(call_name, ": ", grpc_core::StrError(err), " (", err, ")"), + location, {}); } #ifdef GPR_WINDOWS @@ -104,15 +100,15 @@ std::string WSAErrorToShortDescription(int err) { absl::Status grpc_wsa_error(const grpc_core::DebugLocation& location, int err, absl::string_view call_name) { char* utf8_message = gpr_format_message(err); - absl::Status s = StatusCreate(absl::StatusCode::kUnavailable, - WSAErrorToShortDescription(err), location, {}); - StatusSetInt(&s, grpc_core::StatusIntProperty::kWsaError, err); - StatusSetInt(&s, grpc_core::StatusIntProperty::kRpcStatus, + absl::Status status = StatusCreate( + absl::StatusCode::kUnavailable, + absl::StrCat(call_name, ": ", WSAErrorToShortDescription(err), " (", + utf8_message, " -- ", err, ")"), + location, {}); + StatusSetInt(&status, grpc_core::StatusIntProperty::kRpcStatus, GRPC_STATUS_UNAVAILABLE); - StatusSetStr(&s, grpc_core::StatusStrProperty::kOsError, utf8_message); - StatusSetStr(&s, grpc_core::StatusStrProperty::kSyscall, call_name); gpr_free(utf8_message); - return s; + return status; } #endif diff --git a/src/core/lib/iomgr/resolve_address_posix.cc b/src/core/lib/iomgr/resolve_address_posix.cc index c9f2967b898..8cad6517c1a 100644 --- a/src/core/lib/iomgr/resolve_address_posix.cc +++ b/src/core/lib/iomgr/resolve_address_posix.cc @@ -106,14 +106,13 @@ NativeDNSResolver::LookupHostnameBlocking(absl::string_view name, // parse name, splitting it into host and port parts SplitHostPort(name, &host, &port); if (host.empty()) { - err = grpc_error_set_str(GRPC_ERROR_CREATE("unparseable host:port"), - StatusStrProperty::kTargetAddress, name); + err = + GRPC_ERROR_CREATE(absl::StrCat("unparseable host:port \"", name, "\"")); goto done; } if (port.empty()) { if (default_port.empty()) { - err = grpc_error_set_str(GRPC_ERROR_CREATE("no port in name"), - StatusStrProperty::kTargetAddress, name); + err = GRPC_ERROR_CREATE(absl::StrCat("no port in name \"", name, "\"")); goto done; } port = std::string(default_port); @@ -139,14 +138,8 @@ NativeDNSResolver::LookupHostnameBlocking(absl::string_view name, } } if (s != 0) { - err = grpc_error_set_str( - grpc_error_set_str( - grpc_error_set_str( - grpc_error_set_int(GRPC_ERROR_CREATE(gai_strerror(s)), - StatusIntProperty::kErrorNo, s), - StatusStrProperty::kOsError, gai_strerror(s)), - StatusStrProperty::kSyscall, "getaddrinfo"), - StatusStrProperty::kTargetAddress, name); + err = absl::UnknownError(absl::StrCat( + "getaddrinfo(\"", name, "\"): ", gai_strerror(s), " (", s, ")")); goto done; } // Success path: fill in addrs diff --git a/src/core/lib/iomgr/socket_utils_common_posix.cc b/src/core/lib/iomgr/socket_utils_common_posix.cc index 7ebafa6897b..e2d1ffa249f 100644 --- a/src/core/lib/iomgr/socket_utils_common_posix.cc +++ b/src/core/lib/iomgr/socket_utils_common_posix.cc @@ -45,6 +45,7 @@ #include "absl/log/check.h" #include "absl/log/log.h" +#include "absl/strings/str_cat.h" #include #include @@ -464,15 +465,9 @@ int grpc_ipv6_loopback_available(void) { return g_ipv6_loopback_available; } -static grpc_error_handle error_for_fd(int fd, - const grpc_resolved_address* addr) { +static grpc_error_handle error_for_fd(int fd) { if (fd >= 0) return absl::OkStatus(); - auto addr_str = grpc_sockaddr_to_string(addr, false); - grpc_error_handle err = grpc_error_set_str( - GRPC_OS_ERROR(errno, "socket"), - grpc_core::StatusStrProperty::kTargetAddress, - addr_str.ok() ? addr_str.value() : addr_str.status().ToString()); - return err; + return GRPC_OS_ERROR(errno, "socket"); } grpc_error_handle grpc_create_dualstack_socket( @@ -523,7 +518,7 @@ grpc_error_handle grpc_create_dualstack_socket_using_factory( // If this isn't an IPv4 address, then return whatever we've got. if (!grpc_sockaddr_is_v4mapped(resolved_addr, nullptr)) { *dsmode = GRPC_DSMODE_IPV6; - return error_for_fd(*newfd, resolved_addr); + return error_for_fd(*newfd); } // Fall back to AF_INET. if (*newfd >= 0) { @@ -533,7 +528,7 @@ grpc_error_handle grpc_create_dualstack_socket_using_factory( } *dsmode = family == AF_INET ? GRPC_DSMODE_IPV4 : GRPC_DSMODE_NONE; *newfd = create_socket(factory, family, type, protocol); - return error_for_fd(*newfd, resolved_addr); + return error_for_fd(*newfd); } #endif diff --git a/src/core/lib/iomgr/tcp_client_posix.cc b/src/core/lib/iomgr/tcp_client_posix.cc index 8fb50f6754e..32f7d56e229 100644 --- a/src/core/lib/iomgr/tcp_client_posix.cc +++ b/src/core/lib/iomgr/tcp_client_posix.cc @@ -199,8 +199,7 @@ static void on_writable(void* acp, grpc_error_handle error) { gpr_mu_lock(&ac->mu); if (!error.ok()) { - error = grpc_error_set_str(error, grpc_core::StatusStrProperty::kOsError, - "Timeout occurred"); + error = grpc_core::AddMessagePrefix("Timeout occurred", error); goto finish; } @@ -281,8 +280,6 @@ finish: absl::StrCat("Failed to connect to remote host: ", str); error = grpc_error_set_str( error, grpc_core::StatusStrProperty::kDescription, description); - error = grpc_error_set_str( - error, grpc_core::StatusStrProperty::kTargetAddress, addr_str); } if (done) { // This is safe even outside the lock, because "done", the sentinel, is @@ -347,7 +344,7 @@ int64_t grpc_tcp_client_create_from_prepared_fd( return 0; } - std::string name = absl::StrCat("tcp-client:", addr_uri.value()); + std::string name = absl::StrCat("tcp-client:", *addr_uri); grpc_fd* fdobj = grpc_fd_create(fd, name.c_str(), true); int64_t connection_id = 0; if (connect_errno == EWOULDBLOCK || connect_errno == EINPROGRESS) { @@ -358,7 +355,7 @@ int64_t grpc_tcp_client_create_from_prepared_fd( if (err >= 0) { // Connection already succeded. Return 0 to discourage any cancellation // attempts. - *ep = grpc_tcp_client_create_from_fd(fdobj, options, addr_uri.value()); + *ep = grpc_tcp_client_create_from_fd(fdobj, options, *addr_uri); grpc_core::ExecCtx::Run(DEBUG_LOCATION, closure, absl::OkStatus()); return 0; } @@ -366,8 +363,6 @@ int64_t grpc_tcp_client_create_from_prepared_fd( // Connection already failed. Return 0 to discourage any cancellation // attempts. grpc_error_handle error = GRPC_OS_ERROR(connect_errno, "connect"); - error = grpc_error_set_str( - error, grpc_core::StatusStrProperty::kTargetAddress, addr_uri.value()); grpc_fd_orphan(fdobj, nullptr, nullptr, "tcp_client_connect_error"); grpc_core::ExecCtx::Run(DEBUG_LOCATION, closure, error); return 0; diff --git a/src/core/lib/iomgr/tcp_client_windows.cc b/src/core/lib/iomgr/tcp_client_windows.cc index a0589770aac..bedf1cca5b1 100644 --- a/src/core/lib/iomgr/tcp_client_windows.cc +++ b/src/core/lib/iomgr/tcp_client_windows.cc @@ -246,10 +246,8 @@ static int64_t tcp_connect(grpc_closure* on_done, grpc_endpoint** endpoint, failure: CHECK(!error.ok()); - grpc_error_handle final_error = grpc_error_set_str( - GRPC_ERROR_CREATE_REFERENCING("Failed to connect", &error, 1), - grpc_core::StatusStrProperty::kTargetAddress, - addr_uri.ok() ? *addr_uri : addr_uri.status().ToString()); + grpc_error_handle final_error = + GRPC_ERROR_CREATE_REFERENCING("Failed to connect", &error, 1); if (socket != NULL) { grpc_winsocket_destroy(socket); } else if (sock != INVALID_SOCKET) { diff --git a/src/core/lib/iomgr/tcp_posix.cc b/src/core/lib/iomgr/tcp_posix.cc index 6992bcf687c..92394e734e9 100644 --- a/src/core/lib/iomgr/tcp_posix.cc +++ b/src/core/lib/iomgr/tcp_posix.cc @@ -762,14 +762,11 @@ static void finish_estimate(grpc_tcp* tcp) { static grpc_error_handle tcp_annotate_error(grpc_error_handle src_error, grpc_tcp* tcp) { - return grpc_error_set_str( - grpc_error_set_int( - grpc_error_set_int(src_error, grpc_core::StatusIntProperty::kFd, - tcp->fd), - // All tcp errors are marked with UNAVAILABLE so that application may - // choose to retry. - grpc_core::StatusIntProperty::kRpcStatus, GRPC_STATUS_UNAVAILABLE), - grpc_core::StatusStrProperty::kTargetAddress, tcp->peer_string); + return grpc_error_set_int( + grpc_error_set_int(src_error, grpc_core::StatusIntProperty::kFd, tcp->fd), + // All tcp errors are marked with UNAVAILABLE so that application may + // choose to retry. + grpc_core::StatusIntProperty::kRpcStatus, GRPC_STATUS_UNAVAILABLE); } static void tcp_handle_read(void* arg /* grpc_tcp */, grpc_error_handle error); @@ -1668,10 +1665,6 @@ static bool do_tcp_flush_zerocopy(grpc_tcp* tcp, TcpZerocopySendRecord* record, if (saved_errno == EAGAIN || saved_errno == ENOBUFS) { record->UnwindIfThrottled(unwind_slice_idx, unwind_byte_idx); return false; - } else if (saved_errno == EPIPE) { - *error = tcp_annotate_error(GRPC_OS_ERROR(saved_errno, "sendmsg"), tcp); - tcp_shutdown_buffer_list(tcp); - return true; } else { *error = tcp_annotate_error(GRPC_OS_ERROR(saved_errno, "sendmsg"), tcp); tcp_shutdown_buffer_list(tcp); @@ -1782,11 +1775,6 @@ static bool tcp_flush(grpc_tcp* tcp, grpc_error_handle* error) { grpc_slice_buffer_remove_first(tcp->outgoing_buffer); } return false; - } else if (saved_errno == EPIPE) { - *error = tcp_annotate_error(GRPC_OS_ERROR(saved_errno, "sendmsg"), tcp); - grpc_slice_buffer_reset_and_unref(tcp->outgoing_buffer); - tcp_shutdown_buffer_list(tcp); - return true; } else { *error = tcp_annotate_error(GRPC_OS_ERROR(saved_errno, "sendmsg"), tcp); grpc_slice_buffer_reset_and_unref(tcp->outgoing_buffer); diff --git a/src/core/lib/iomgr/tcp_server_windows.cc b/src/core/lib/iomgr/tcp_server_windows.cc index b2a0ab25be8..f1ee5be6437 100644 --- a/src/core/lib/iomgr/tcp_server_windows.cc +++ b/src/core/lib/iomgr/tcp_server_windows.cc @@ -295,14 +295,9 @@ static grpc_error_handle prepare_socket(SOCKET sock, failure: CHECK(!error.ok()); - auto addr_uri = grpc_sockaddr_to_uri(addr); - error = grpc_error_set_int( - grpc_error_set_str( - GRPC_ERROR_CREATE_REFERENCING("Failed to prepare server socket", - &error, 1), - grpc_core::StatusStrProperty::kTargetAddress, - addr_uri.ok() ? *addr_uri : addr_uri.status().ToString()), - grpc_core::StatusIntProperty::kFd, (intptr_t)sock); + error = grpc_error_set_int(GRPC_ERROR_CREATE_REFERENCING( + "Failed to prepare server socket", &error, 1), + grpc_core::StatusIntProperty::kFd, (intptr_t)sock); if (sock != INVALID_SOCKET) closesocket(sock); return error; } diff --git a/src/core/lib/security/credentials/google_default/google_default_credentials.cc b/src/core/lib/security/credentials/google_default/google_default_credentials.cc index 3e6b843b972..bebf35e4710 100644 --- a/src/core/lib/security/credentials/google_default/google_default_credentials.cc +++ b/src/core/lib/security/credentials/google_default/google_default_credentials.cc @@ -27,6 +27,7 @@ #include "absl/log/log.h" #include "absl/status/statusor.h" #include "absl/strings/match.h" +#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" @@ -283,9 +284,8 @@ static grpc_error_handle create_default_creds_from_path( json = std::move(*json_or); } if (json.type() != Json::Type::kObject) { - error = grpc_error_set_str(GRPC_ERROR_CREATE("Failed to parse JSON"), - grpc_core::StatusStrProperty::kRawBytes, - creds_data->as_string_view()); + error = GRPC_ERROR_CREATE(absl::StrCat("Failed to parse JSON \"", + creds_data->as_string_view(), "\"")); goto end; } diff --git a/src/core/resolver/dns/c_ares/grpc_ares_wrapper.cc b/src/core/resolver/dns/c_ares/grpc_ares_wrapper.cc index 7994a1900a5..3cf577cb625 100644 --- a/src/core/resolver/dns/c_ares/grpc_ares_wrapper.cc +++ b/src/core/resolver/dns/c_ares/grpc_ares_wrapper.cc @@ -884,14 +884,11 @@ grpc_error_handle grpc_dns_lookup_ares_continued( grpc_core::SplitHostPort(name, host, port); if (host->empty()) { error = - grpc_error_set_str(GRPC_ERROR_CREATE("unparseable host:port"), - grpc_core::StatusStrProperty::kTargetAddress, name); + GRPC_ERROR_CREATE(absl::StrCat("unparseable host:port \"", name, "\"")); return error; } else if (check_port && port->empty()) { if (default_port == nullptr || strlen(default_port) == 0) { - error = grpc_error_set_str(GRPC_ERROR_CREATE("no port in name"), - grpc_core::StatusStrProperty::kTargetAddress, - name); + error = GRPC_ERROR_CREATE(absl::StrCat("no port in name \"", name, "\"")); return error; } *port = default_port; diff --git a/src/core/util/http_client/httpcli.cc b/src/core/util/http_client/httpcli.cc index 2771787dd00..715777b1a4a 100644 --- a/src/core/util/http_client/httpcli.cc +++ b/src/core/util/http_client/httpcli.cc @@ -244,11 +244,8 @@ void HttpRequest::AppendError(grpc_error_handle error) { } const grpc_resolved_address* addr = &addresses_[next_address_ - 1]; auto addr_text = grpc_sockaddr_to_uri(addr); - overall_error_ = grpc_error_add_child( - overall_error_, - grpc_error_set_str( - error, StatusStrProperty::kTargetAddress, - addr_text.ok() ? addr_text.value() : addr_text.status().ToString())); + if (addr_text.ok()) error = AddMessagePrefix(*addr_text, std::move(error)); + overall_error_ = grpc_error_add_child(overall_error_, std::move(error)); } void HttpRequest::OnReadInternal(grpc_error_handle error) { diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index 3ed50fe703b..f8d44cc6a01 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -413,7 +413,6 @@ CORE_SOURCE_FILES = [ 'src/core/handshaker/proxy_mapper_registry.cc', 'src/core/handshaker/security/secure_endpoint.cc', 'src/core/handshaker/security/security_handshaker.cc', - 'src/core/handshaker/security/tsi_error.cc', 'src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc', 'src/core/lib/address_utils/parse_address.cc', 'src/core/lib/address_utils/sockaddr_utils.cc', diff --git a/test/core/gprpp/status_helper_test.cc b/test/core/gprpp/status_helper_test.cc index fd967742a2d..51f9a571cff 100644 --- a/test/core/gprpp/status_helper_test.cc +++ b/test/core/gprpp/status_helper_test.cc @@ -47,26 +47,26 @@ TEST(StatusUtilTest, CreateStatus) { TEST(StatusUtilTest, SetAndGetInt) { absl::Status s = absl::CancelledError(); - StatusSetInt(&s, StatusIntProperty::kErrorNo, 2021); - EXPECT_EQ(2021, StatusGetInt(s, StatusIntProperty::kErrorNo)); + StatusSetInt(&s, StatusIntProperty::kStreamId, 2021); + EXPECT_EQ(2021, StatusGetInt(s, StatusIntProperty::kStreamId)); } TEST(StatusUtilTest, GetIntNotExistent) { absl::Status s = absl::CancelledError(); EXPECT_EQ(absl::optional(), - StatusGetInt(s, StatusIntProperty::kErrorNo)); + StatusGetInt(s, StatusIntProperty::kStreamId)); } TEST(StatusUtilTest, SetAndGetStr) { absl::Status s = absl::CancelledError(); - StatusSetStr(&s, StatusStrProperty::kOsError, "value"); - EXPECT_EQ("value", StatusGetStr(s, StatusStrProperty::kOsError)); + StatusSetStr(&s, StatusStrProperty::kFile, "value"); + EXPECT_EQ("value", StatusGetStr(s, StatusStrProperty::kFile)); } TEST(StatusUtilTest, GetStrNotExistent) { absl::Status s = absl::CancelledError(); EXPECT_EQ(absl::optional(), - StatusGetStr(s, StatusStrProperty::kOsError)); + StatusGetStr(s, StatusStrProperty::kFile)); } TEST(StatusUtilTest, SetAndGetTime) { @@ -96,8 +96,8 @@ TEST(StatusUtilTest, AddAndGetChildren) { TEST(StatusUtilTest, ToAndFromProto) { absl::Status s = absl::CancelledError("Message"); - StatusSetInt(&s, StatusIntProperty::kErrorNo, 2021); - StatusSetStr(&s, StatusStrProperty::kOsError, "value"); + StatusSetInt(&s, StatusIntProperty::kStreamId, 2021); + StatusSetStr(&s, StatusStrProperty::kFile, "value"); upb::Arena arena; google_rpc_Status* msg = internal::StatusToProto(s, arena.ptr()); size_t len; @@ -109,8 +109,8 @@ TEST(StatusUtilTest, ToAndFromProto) { TEST(StatusUtilTest, ToAndFromProtoWithNonUTF8Characters) { absl::Status s = absl::CancelledError("_\xAB\xCD\xEF_"); - StatusSetInt(&s, StatusIntProperty::kErrorNo, 2021); - StatusSetStr(&s, StatusStrProperty::kOsError, "!\xFF\xCC\xAA!"); + StatusSetInt(&s, StatusIntProperty::kStreamId, 2021); + StatusSetStr(&s, StatusStrProperty::kFile, "!\xFF\xCC\xAA!"); upb::Arena arena; google_rpc_Status* msg = internal::StatusToProto(s, arena.ptr()); size_t len; @@ -134,9 +134,9 @@ TEST(StatusUtilTest, CancelledErrorToString) { TEST(StatusUtilTest, ErrorWithIntPropertyToString) { absl::Status s = absl::CancelledError("Message"); - StatusSetInt(&s, StatusIntProperty::kErrorNo, 2021); + StatusSetInt(&s, StatusIntProperty::kStreamId, 2021); std::string t = StatusToString(s); - EXPECT_EQ("CANCELLED:Message {errno:2021}", t); + EXPECT_EQ("CANCELLED:Message {stream_id:2021}", t); } TEST(StatusUtilTest, ErrorWithStrPropertyToString) { @@ -158,16 +158,16 @@ TEST(StatusUtilTest, ErrorWithTimePropertyToString) { TEST(StatusUtilTest, ComplexErrorWithChildrenToString) { absl::Status s = absl::CancelledError("Message"); - StatusSetInt(&s, StatusIntProperty::kErrorNo, 2021); + StatusSetInt(&s, StatusIntProperty::kStreamId, 2021); absl::Status s1 = absl::AbortedError("Message1"); StatusAddChild(&s, s1); absl::Status s2 = absl::AlreadyExistsError("Message2"); - StatusSetStr(&s2, StatusStrProperty::kOsError, "value"); + StatusSetStr(&s2, StatusStrProperty::kFile, "value"); StatusAddChild(&s, s2); std::string t = StatusToString(s); EXPECT_EQ( - "CANCELLED:Message {errno:2021, children:[" - "ABORTED:Message1, ALREADY_EXISTS:Message2 {os_error:\"value\"}]}", + "CANCELLED:Message {stream_id:2021, children:[" + "ABORTED:Message1, ALREADY_EXISTS:Message2 {file:\"value\"}]}", t); } diff --git a/test/core/iomgr/error_test.cc b/test/core/iomgr/error_test.cc index 165158b06b9..86650ac9d45 100644 --- a/test/core/iomgr/error_test.cc +++ b/test/core/iomgr/error_test.cc @@ -23,11 +23,13 @@ #include #include "absl/log/log.h" +#include "absl/strings/str_cat.h" #include #include #include "src/core/lib/gprpp/crash.h" +#include "src/core/lib/gprpp/strerror.h" #include "test/core/test_util/test_config.h" TEST(ErrorTest, SetGetInt) { @@ -41,16 +43,9 @@ TEST(ErrorTest, SetGetInt) { EXPECT_TRUE(i); // line set will never be 0 #endif EXPECT_TRUE( - !grpc_error_get_int(error, grpc_core::StatusIntProperty::kErrorNo, &i)); - EXPECT_TRUE( - !grpc_error_get_int(error, grpc_core::StatusIntProperty::kSize, &i)); - - intptr_t errnumber = 314; - error = grpc_error_set_int(error, grpc_core::StatusIntProperty::kErrorNo, - errnumber); - EXPECT_TRUE( - grpc_error_get_int(error, grpc_core::StatusIntProperty::kErrorNo, &i)); - EXPECT_EQ(i, errnumber); + !grpc_error_get_int(error, grpc_core::StatusIntProperty::kStreamId, &i)); + EXPECT_TRUE(!grpc_error_get_int( + error, grpc_core::StatusIntProperty::kHttp2Error, &i)); intptr_t http = 2; error = grpc_error_set_int(error, grpc_core::StatusIntProperty::kHttp2Error, @@ -64,10 +59,6 @@ TEST(ErrorTest, SetGetStr) { grpc_error_handle error = GRPC_ERROR_CREATE("Test"); std::string str; - EXPECT_TRUE( - !grpc_error_get_str(error, grpc_core::StatusStrProperty::kSyscall, &str)); - EXPECT_TRUE(!grpc_error_get_str( - error, grpc_core::StatusStrProperty::kTsiError, &str)); #ifndef NDEBUG // grpc_core::StatusStrProperty::kFile is for debug only EXPECT_TRUE( @@ -90,28 +81,24 @@ TEST(ErrorTest, SetGetStr) { TEST(ErrorTest, CopyAndUnRef) { // error1 has one ref - grpc_error_handle error1 = - grpc_error_set_str(GRPC_ERROR_CREATE("Test"), - grpc_core::StatusStrProperty::kGrpcMessage, "message"); - std::string str; - EXPECT_TRUE(grpc_error_get_str( - error1, grpc_core::StatusStrProperty::kGrpcMessage, &str)); - EXPECT_EQ(str, "message"); + grpc_error_handle error1 = grpc_error_set_int( + GRPC_ERROR_CREATE("Test"), grpc_core::StatusIntProperty::kStreamId, 1); + intptr_t i; + EXPECT_TRUE( + grpc_error_get_int(error1, grpc_core::StatusIntProperty::kStreamId, &i)); + EXPECT_EQ(i, 1); // this gives error3 a ref to the new error, and decrements error1 to one ref - grpc_error_handle error3 = grpc_error_set_str( - error1, grpc_core::StatusStrProperty::kSyscall, "syscall"); + grpc_error_handle error3 = + grpc_error_set_int(error1, grpc_core::StatusIntProperty::kHttp2Error, 2); EXPECT_NE(error3, error1); // should not be the same because of extra ref - EXPECT_TRUE(grpc_error_get_str( - error3, grpc_core::StatusStrProperty::kGrpcMessage, &str)); - EXPECT_EQ(str, "message"); + EXPECT_TRUE(grpc_error_get_int( + error3, grpc_core::StatusIntProperty::kHttp2Error, &i)); + EXPECT_EQ(i, 2); - // error 1 should not have a syscall but 3 should - EXPECT_TRUE(!grpc_error_get_str( - error1, grpc_core::StatusStrProperty::kSyscall, &str)); - EXPECT_TRUE( - grpc_error_get_str(error3, grpc_core::StatusStrProperty::kSyscall, &str)); - EXPECT_EQ(str, "syscall"); + // error 1 should not have kHttp2Error + EXPECT_FALSE(grpc_error_get_int( + error1, grpc_core::StatusIntProperty::kHttp2Error, &i)); } TEST(ErrorTest, CreateReferencing) { @@ -146,7 +133,8 @@ TEST(ErrorTest, PrintErrorString) { grpc_error_handle error = grpc_error_set_int( GRPC_ERROR_CREATE("Error"), grpc_core::StatusIntProperty::kRpcStatus, GRPC_STATUS_UNIMPLEMENTED); - error = grpc_error_set_int(error, grpc_core::StatusIntProperty::kSize, 666); + error = + grpc_error_set_int(error, grpc_core::StatusIntProperty::kHttp2Error, 666); error = grpc_error_set_str(error, grpc_core::StatusStrProperty::kGrpcMessage, "message"); // VLOG(2) << grpc_core::StatusToString(error); @@ -176,16 +164,8 @@ TEST(ErrorTest, TestOsError) { int fake_errno = 5; const char* syscall = "syscall name"; grpc_error_handle error = GRPC_OS_ERROR(fake_errno, syscall); - - intptr_t i = 0; - EXPECT_TRUE( - grpc_error_get_int(error, grpc_core::StatusIntProperty::kErrorNo, &i)); - EXPECT_EQ(i, fake_errno); - - std::string str; - EXPECT_TRUE( - grpc_error_get_str(error, grpc_core::StatusStrProperty::kSyscall, &str)); - EXPECT_EQ(str, syscall); + EXPECT_EQ(error.message(), + absl::StrCat("syscall name: ", grpc_core::StrError(5), " (5)")); } int main(int argc, char** argv) { diff --git a/test/core/tsi/transport_security_test_lib.cc b/test/core/tsi/transport_security_test_lib.cc index 946c09f6e2e..bf0d7415d9a 100644 --- a/test/core/tsi/transport_security_test_lib.cc +++ b/test/core/tsi/transport_security_test_lib.cc @@ -32,14 +32,15 @@ #include #include "absl/log/check.h" +#include "absl/strings/str_cat.h" #include #include #include -#include "src/core/handshaker/security/tsi_error.h" #include "src/core/lib/gprpp/crash.h" #include "src/core/lib/gprpp/memory.h" +#include "src/core/lib/iomgr/error.h" static void notification_signal(tsi_test_fixture* fixture) { gpr_mu_lock(&fixture->mu); @@ -318,8 +319,8 @@ grpc_error_handle on_handshake_next_done( } if (result != TSI_OK) { notification_signal(fixture); - return grpc_set_tsi_error_result(GRPC_ERROR_CREATE("Handshake failed"), - result); + return GRPC_ERROR_CREATE( + absl::StrCat("Handshake failed (", tsi_result_to_string(result), ")")); } // Update handshaker result. if (handshaker_result != nullptr) { diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc index 97cf1f61555..cc0a6bbccfa 100644 --- a/test/cpp/end2end/client_lb_end2end_test.cc +++ b/test/cpp/end2end/client_lb_end2end_test.cc @@ -582,12 +582,20 @@ 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: )?" - "(Connection refused|Connection reset by peer|" - "recvmsg:Connection reset by peer|" - "getsockopt\\(SO\\_ERROR\\): Connection reset by peer|" - "Socket closed|FD shutdown)"); + "(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]+\\))?"); } const std::string server_host_; diff --git a/test/cpp/end2end/xds/xds_end2end_test_lib.cc b/test/cpp/end2end/xds/xds_end2end_test_lib.cc index f9e884650fe..e46a16bdf19 100644 --- a/test/cpp/end2end/xds/xds_end2end_test_lib.cc +++ b/test/cpp/end2end/xds/xds_end2end_test_lib.cc @@ -836,14 +836,22 @@ void XdsEnd2endTest::SetProtoDuration( std::string XdsEnd2endTest::MakeConnectionFailureRegex( absl::string_view prefix) { - return absl::StrCat( - prefix, - "(UNKNOWN|UNAVAILABLE): (ipv6:%5B::1%5D|ipv4:127.0.0.1):[0-9]+: " - "(Failed to connect to remote host: )?" - "(Connection refused|Connection reset by peer|" - "recvmsg:Connection reset by peer|" - "getsockopt\\(SO\\_ERROR\\): Connection reset by peer|" - "Socket closed|FD shutdown)"); + 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]+\\))?"); } grpc_core::PemKeyCertPairList XdsEnd2endTest::ReadTlsIdentityPair( diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index c3428fdd3a1..55526bff491 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -2087,8 +2087,6 @@ src/core/handshaker/security/secure_endpoint.cc \ src/core/handshaker/security/secure_endpoint.h \ src/core/handshaker/security/security_handshaker.cc \ src/core/handshaker/security/security_handshaker.h \ -src/core/handshaker/security/tsi_error.cc \ -src/core/handshaker/security/tsi_error.h \ src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc \ src/core/handshaker/tcp_connect/tcp_connect_handshaker.h \ src/core/lib/address_utils/parse_address.cc \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 1439a0c8d46..da2bfa5b6fd 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -1857,8 +1857,6 @@ src/core/handshaker/security/secure_endpoint.cc \ src/core/handshaker/security/secure_endpoint.h \ src/core/handshaker/security/security_handshaker.cc \ src/core/handshaker/security/security_handshaker.h \ -src/core/handshaker/security/tsi_error.cc \ -src/core/handshaker/security/tsi_error.h \ src/core/handshaker/tcp_connect/tcp_connect_handshaker.cc \ src/core/handshaker/tcp_connect/tcp_connect_handshaker.h \ src/core/lib/README.md \ From ca4c025091f5ca8e72225492baf7db66174e793c Mon Sep 17 00:00:00 2001 From: Esun Kim Date: Fri, 31 May 2024 14:53:46 -0700 Subject: [PATCH 3/7] [Test] Silence gcc 7 (#36752) Regarding https://github.com/grpc/grpc/issues/36751, this PR is to Silence gcc 7. This change is expected to get rolled back once gcc 7 is able to build gRPC. --- tools/bazelify_tests/test/portability_tests.bzl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/bazelify_tests/test/portability_tests.bzl b/tools/bazelify_tests/test/portability_tests.bzl index e3328126cbf..006d1f48b45 100644 --- a/tools/bazelify_tests/test/portability_tests.bzl +++ b/tools/bazelify_tests/test/portability_tests.bzl @@ -53,7 +53,9 @@ def generate_run_tests_portability_tests(name): # C and C++ under different compilers for language in ["c", "c++"]: compiler_configs = [ - ["gcc_7", "", "tools/dockerfile/test/cxx_gcc_7_x64.current_version"], + # TODO(https://github.com/grpc/grpc/issues/36751): Replace gcc_8 with gcc_7 once it's fixed. + # ["gcc_7", "", "tools/dockerfile/test/cxx_gcc_7_x64.current_version"], + ["gcc_8", "", "tools/dockerfile/test/cxx_gcc_8_x64.current_version"], ["gcc_12", "--cmake_configure_extra_args=-DCMAKE_CXX_STANDARD=20", "tools/dockerfile/test/cxx_gcc_12_x64.current_version"], ["gcc10.2_openssl102", "--cmake_configure_extra_args=-DgRPC_SSL_PROVIDER=package", "tools/dockerfile/test/cxx_debian11_openssl102_x64.current_version"], ["gcc10.2_openssl111", "--cmake_configure_extra_args=-DgRPC_SSL_PROVIDER=package", "tools/dockerfile/test/cxx_debian11_openssl111_x64.current_version"], From 108f70b4bb277d527c1fc4a3a2d02589e285ae8c Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 31 May 2024 15:00:03 -0700 Subject: [PATCH 4/7] [context] Move tracing context to arena based context (#36782) Closes #36782 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36782 from ctiller:ctx8 ea7da93756f73d1fd9956fc10b161f395fa87091 PiperOrigin-RevId: 639162692 --- BUILD | 2 +- src/core/ext/filters/census/grpc_context.cc | 6 ++---- .../ext/transport/chttp2/transport/chttp2_transport.h | 2 +- src/core/ext/transport/chttp2/transport/writing.cc | 2 +- src/core/lib/channel/context.h | 3 --- src/core/lib/surface/call.cc | 4 ++-- src/core/lib/surface/call.h | 5 +++++ src/cpp/ext/filters/census/client_filter.cc | 4 ++-- src/cpp/ext/filters/census/server_call_tracer.cc | 8 ++++---- 9 files changed, 18 insertions(+), 18 deletions(-) diff --git a/BUILD b/BUILD index 2bc4c344fc7..612c1cfae46 100644 --- a/BUILD +++ b/BUILD @@ -1341,7 +1341,7 @@ grpc_cc_library( "grpc_base", "grpc_public_hdrs", "grpc_trace", - "legacy_context", + "//src/core:arena", ], ) diff --git a/src/core/ext/filters/census/grpc_context.cc b/src/core/ext/filters/census/grpc_context.cc index a26ccf05898..e2affd09a4b 100644 --- a/src/core/ext/filters/census/grpc_context.cc +++ b/src/core/ext/filters/census/grpc_context.cc @@ -21,7 +21,6 @@ #include #include -#include "src/core/lib/channel/context.h" #include "src/core/lib/debug/trace.h" #include "src/core/lib/surface/api_trace.h" #include "src/core/lib/surface/call.h" @@ -30,12 +29,11 @@ void grpc_census_call_set_context(grpc_call* call, census_context* context) { GRPC_API_TRACE("grpc_census_call_set_context(call=%p, census_context=%p)", 2, (call, context)); if (context != nullptr) { - grpc_call_context_set(call, GRPC_CONTEXT_TRACING, context, nullptr); + grpc_call_get_arena(call)->SetContext(context); } } census_context* grpc_census_call_get_context(grpc_call* call) { GRPC_API_TRACE("grpc_census_call_get_context(call=%p)", 1, (call)); - return static_cast( - grpc_call_context_get(call, GRPC_CONTEXT_TRACING)); + return grpc_call_get_arena(call)->GetContext(); } diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.h b/src/core/ext/transport/chttp2/transport/chttp2_transport.h index a98381b489d..884fe64efa9 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.h +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.h @@ -83,7 +83,7 @@ void TestOnlyGlobalHttp2TransportDisableTransientFailureStateNotification( typedef void (*WriteTimestampsCallback)(void*, Timestamps*, grpc_error_handle error); -typedef void* (*CopyContextFn)(void*); +typedef void* (*CopyContextFn)(grpc_core::Arena*); void GrpcHttp2SetWriteTimestampsCallback(WriteTimestampsCallback fn); void GrpcHttp2SetCopyContextFn(CopyContextFn fn); diff --git a/src/core/ext/transport/chttp2/transport/writing.cc b/src/core/ext/transport/chttp2/transport/writing.cc index a40d7c54359..f15c391b831 100644 --- a/src/core/ext/transport/chttp2/transport/writing.cc +++ b/src/core/ext/transport/chttp2/transport/writing.cc @@ -682,7 +682,7 @@ grpc_chttp2_begin_write_result grpc_chttp2_begin_write( grpc_core::GrpcHttp2GetCopyContextFn(); if (copy_context_fn != nullptr && grpc_core::GrpcHttp2GetWriteTimestampsCallback() != nullptr) { - t->context_list->emplace_back(copy_context_fn(s->context), + t->context_list->emplace_back(copy_context_fn(s->arena), outbuf_relative_start_pos, num_stream_bytes, s->byte_counter, s->write_counter - 1, s->tcp_tracer); diff --git a/src/core/lib/channel/context.h b/src/core/lib/channel/context.h index 100f3927def..632c6b5ce12 100644 --- a/src/core/lib/channel/context.h +++ b/src/core/lib/channel/context.h @@ -32,9 +32,6 @@ typedef enum { /// grpc_call* associated with this context. GRPC_CONTEXT_CALL = 0, - /// Value is a \a census_context. - GRPC_CONTEXT_TRACING, - /// Holds a pointer to ServiceConfigCallData associated with this call. GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA, diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index b793bba7df9..8c8bc45d385 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -161,8 +161,8 @@ absl::Status Call::InitParent(Call* parent, uint32_t propagation_mask) { "Census tracing propagation requested without Census context " "propagation"); } - ContextSet(GRPC_CONTEXT_TRACING, parent->ContextGet(GRPC_CONTEXT_TRACING), - nullptr); + arena()->SetContext( + parent->arena()->GetContext()); } else if (propagation_mask & GRPC_PROPAGATE_CENSUS_STATS_CONTEXT) { return absl::UnknownError( "Census context propagation requested without Census tracing " diff --git a/src/core/lib/surface/call.h b/src/core/lib/surface/call.h index 0d557629388..917881f00ed 100644 --- a/src/core/lib/surface/call.h +++ b/src/core/lib/surface/call.h @@ -78,6 +78,11 @@ typedef struct grpc_call_create_args { namespace grpc_core { +template <> +struct ArenaContextType { + static void Destroy(census_context*) {} +}; + class Call : public CppImplOf, public grpc_event_engine::experimental::EventEngine:: Closure /* for deadlines */ { diff --git a/src/cpp/ext/filters/census/client_filter.cc b/src/cpp/ext/filters/census/client_filter.cc index 4733040f152..52465fdaf30 100644 --- a/src/cpp/ext/filters/census/client_filter.cc +++ b/src/cpp/ext/filters/census/client_filter.cc @@ -312,8 +312,8 @@ OpenCensusCallTracer::OpenCensusCallTracer( method_(GetMethod(path_)), arena_(arena), tracing_enabled_(tracing_enabled) { - auto* parent_context = reinterpret_cast( - call_context_[GRPC_CONTEXT_TRACING].value); + auto* parent_context = + reinterpret_cast(arena->GetContext()); GenerateClientContext(tracing_enabled_ ? absl::StrCat("Sent.", method_) : "", &context_, (parent_context == nullptr) ? nullptr : parent_context); diff --git a/src/cpp/ext/filters/census/server_call_tracer.cc b/src/cpp/ext/filters/census/server_call_tracer.cc index e0852065320..24c6e80fb98 100644 --- a/src/cpp/ext/filters/census/server_call_tracer.cc +++ b/src/cpp/ext/filters/census/server_call_tracer.cc @@ -40,16 +40,16 @@ #include "opencensus/trace/span_id.h" #include "opencensus/trace/trace_id.h" +#include #include #include -#include "src/core/lib/channel/channel_stack.h" -#include "src/core/lib/channel/context.h" #include "src/core/lib/iomgr/error.h" #include "src/core/lib/promise/context.h" #include "src/core/lib/resource_quota/arena.h" #include "src/core/lib/slice/slice.h" #include "src/core/lib/slice/slice_buffer.h" +#include "src/core/lib/surface/call.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/telemetry/call_tracer.h" #include "src/core/telemetry/tcp_tracer.h" @@ -206,8 +206,8 @@ void OpenCensusServerCallTracer::RecordReceivedInitialMetadata( tracing_enabled ? sml.tracing_slice.as_string_view() : "", absl::StrCat("Recv.", method_), &context_); if (tracing_enabled) { - auto* call_context = grpc_core::GetContext(); - call_context[GRPC_CONTEXT_TRACING].value = &context_; + grpc_core::SetContext( + reinterpret_cast(&context_)); } if (OpenCensusStatsEnabled()) { std::vector> tags = From a1ff6531fbfb5bf6cc681a57136c970768728691 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 31 May 2024 15:36:42 -0700 Subject: [PATCH 5/7] [context] Move `Call` context to arena context (#36775) Closes #36775 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36775 from ctiller:ctx1 3ca1d76da10a4076b37e63bb50e5789a076e254d PiperOrigin-RevId: 639173227 --- src/core/client_channel/client_channel_filter.cc | 11 +++-------- src/core/lib/channel/context.h | 9 --------- src/core/lib/surface/call.cc | 7 +++---- src/core/lib/surface/call.h | 5 +++++ 4 files changed, 11 insertions(+), 21 deletions(-) diff --git a/src/core/client_channel/client_channel_filter.cc b/src/core/client_channel/client_channel_filter.cc index 5eceb724703..9f935dc1b59 100644 --- a/src/core/client_channel/client_channel_filter.cc +++ b/src/core/client_channel/client_channel_filter.cc @@ -269,8 +269,7 @@ class ClientChannelFilter::FilterBasedCallData final void ResetDeadline(Duration timeout) override { const Timestamp per_method_deadline = Timestamp::FromCycleCounterRoundUp(call_start_time_) + timeout; - static_cast(call_context_[GRPC_CONTEXT_CALL].value) - ->UpdateDeadline(per_method_deadline); + arena_->GetContext()->UpdateDeadline(per_method_deadline); } void CreateDynamicCall(); @@ -3320,10 +3319,7 @@ void ClientChannelFilter::FilterBasedLoadBalancedCall:: grpc_status_code code; std::string message; grpc_error_get_status( - error, - static_cast(self->call_context()[GRPC_CONTEXT_CALL].value) - ->deadline(), - &code, &message, + error, self->arena()->GetContext()->deadline(), &code, &message, /*http_error=*/nullptr, /*error_string=*/nullptr); status = absl::Status(static_cast(code), message); } else { @@ -3462,8 +3458,7 @@ void ClientChannelFilter::FilterBasedLoadBalancedCall::CreateSubchannelCall() { CHECK_NE(path, nullptr); SubchannelCall::Args call_args = { connected_subchannel()->Ref(), pollent_, path->Ref(), /*start_time=*/0, - static_cast(call_context()[GRPC_CONTEXT_CALL].value)->deadline(), - arena(), + arena()->GetContext()->deadline(), arena(), // TODO(roth): When we implement hedging support, we will probably // need to use a separate call context for each subchannel call. call_context(), call_combiner_}; diff --git a/src/core/lib/channel/context.h b/src/core/lib/channel/context.h index 632c6b5ce12..6432efb03a6 100644 --- a/src/core/lib/channel/context.h +++ b/src/core/lib/channel/context.h @@ -29,9 +29,6 @@ /// This enum represents the indexes into the array, where each index /// contains a different type of value. typedef enum { - /// grpc_call* associated with this context. - GRPC_CONTEXT_CALL = 0, - /// Holds a pointer to ServiceConfigCallData associated with this call. GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA, @@ -48,7 +45,6 @@ struct grpc_call_context_element { }; namespace grpc_core { -class Call; class ServiceConfigCallData; // Bind the legacy context array into the new style structure @@ -62,11 +58,6 @@ namespace promise_detail { template struct OldStyleContext; -template <> -struct OldStyleContext { - static constexpr grpc_context_index kIndex = GRPC_CONTEXT_CALL; -}; - template <> struct OldStyleContext { static constexpr grpc_context_index kIndex = diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 8c8bc45d385..00f424bf0af 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -602,7 +602,7 @@ class FilterStackCall final : public ChannelBasedCall { args.send_deadline, args.channel->Ref()), cq_(args.cq), stream_op_payload_(context_) { - context_[GRPC_CONTEXT_CALL].value = this; + GetArena()->SetContext(this); } static void ReleaseCall(void* call, grpc_error_handle); @@ -1917,7 +1917,7 @@ class BasicPromiseBasedCall : public ChannelBasedCall, public Party { if (args.cq != nullptr) { GRPC_CQ_INTERNAL_REF(args.cq, "bind"); } - context_[GRPC_CONTEXT_CALL].value = this; + GetArena()->SetContext(this); } ~BasicPromiseBasedCall() override { @@ -3173,8 +3173,7 @@ class ServerCall final : public Call, public DualRefCounted { client_initial_metadata_stored_(std::move(client_initial_metadata)), cq_(cq), server_(server) { - call_handler_.legacy_context()[GRPC_CONTEXT_CALL].value = - static_cast(this); + call_handler_.arena()->SetContext(this); global_stats().IncrementServerCallsCreated(); } diff --git a/src/core/lib/surface/call.h b/src/core/lib/surface/call.h index 917881f00ed..12c214c68f1 100644 --- a/src/core/lib/surface/call.h +++ b/src/core/lib/surface/call.h @@ -246,6 +246,11 @@ class Call : public CppImplOf, gpr_cycle_counter start_time_ = gpr_get_cycle_counter(); }; +template <> +struct ArenaContextType { + static void Destroy(Call*) {} +}; + class BasicPromiseBasedCall; class ServerPromiseBasedCall; From 0c372338923da49cab6aea6cea4420e3960603df Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 31 May 2024 16:16:58 -0700 Subject: [PATCH 6/7] [sanity] fix (#36791) Closes #36791 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36791 from ctiller:san 692d74bffa52c0cea8006aa91bf5c7a89c75b69a PiperOrigin-RevId: 639184328 --- src/core/ext/transport/chttp2/transport/chttp2_transport.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.h b/src/core/ext/transport/chttp2/transport/chttp2_transport.h index 884fe64efa9..81efd055e6a 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.h +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.h @@ -83,7 +83,7 @@ void TestOnlyGlobalHttp2TransportDisableTransientFailureStateNotification( typedef void (*WriteTimestampsCallback)(void*, Timestamps*, grpc_error_handle error); -typedef void* (*CopyContextFn)(grpc_core::Arena*); +typedef void* (*CopyContextFn)(Arena*); void GrpcHttp2SetWriteTimestampsCallback(WriteTimestampsCallback fn); void GrpcHttp2SetCopyContextFn(CopyContextFn fn); From d26a878e83af91e7b6d828af65936d7e7babb5db Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 31 May 2024 16:20:48 -0700 Subject: [PATCH 7/7] [context] Move backend metric provider to arena based context (#36781) Closes #36781 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36781 from ctiller:ctx7 06f44a31d198a62b22cfe2fa2e92544f342a8d71 PiperOrigin-RevId: 639185505 --- src/core/BUILD | 1 + .../ext/filters/backend_metrics/backend_metric_filter.cc | 6 ++---- .../ext/filters/backend_metrics/backend_metric_provider.h | 7 +++++++ src/core/lib/channel/context.h | 4 ---- src/cpp/server/server_context.cc | 3 +-- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/core/BUILD b/src/core/BUILD index 4b84aaac004..354b3d9fcab 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -5032,6 +5032,7 @@ grpc_cc_library( "ext/filters/backend_metrics/backend_metric_provider.h", ], language = "c++", + deps = ["arena"], ) grpc_cc_library( 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 cdc78e0523a..2664c271a22 100644 --- a/src/core/ext/filters/backend_metrics/backend_metric_filter.cc +++ b/src/core/ext/filters/backend_metrics/backend_metric_filter.cc @@ -130,16 +130,14 @@ BackendMetricFilter::Create(const ChannelArgs&, ChannelFilter::Args) { void BackendMetricFilter::Call::OnServerTrailingMetadata(ServerMetadata& md) { if (md.get(GrpcCallWasCancelled()).value_or(false)) return; - auto* ctx = &GetContext< - grpc_call_context_element>()[GRPC_CONTEXT_BACKEND_METRIC_PROVIDER]; + auto* ctx = MaybeGetContext(); if (ctx == nullptr) { if (GRPC_TRACE_FLAG_ENABLED(grpc_backend_metric_filter_trace)) { gpr_log(GPR_INFO, "[%p] No BackendMetricProvider.", this); } return; } - absl::optional serialized = MaybeSerializeBackendMetrics( - reinterpret_cast(ctx->value)); + absl::optional serialized = MaybeSerializeBackendMetrics(ctx); if (serialized.has_value() && !serialized->empty()) { if (GRPC_TRACE_FLAG_ENABLED(grpc_backend_metric_filter_trace)) { gpr_log(GPR_INFO, "[%p] Backend metrics serialized. size: %" PRIuPTR, diff --git a/src/core/ext/filters/backend_metrics/backend_metric_provider.h b/src/core/ext/filters/backend_metrics/backend_metric_provider.h index 42ecc909529..c1ae776f83b 100644 --- a/src/core/ext/filters/backend_metrics/backend_metric_provider.h +++ b/src/core/ext/filters/backend_metrics/backend_metric_provider.h @@ -15,6 +15,8 @@ #ifndef GRPC_SRC_CORE_EXT_FILTERS_BACKEND_METRICS_BACKEND_METRIC_PROVIDER_H #define GRPC_SRC_CORE_EXT_FILTERS_BACKEND_METRICS_BACKEND_METRIC_PROVIDER_H +#include "src/core/lib/resource_quota/arena.h" + namespace grpc_core { struct BackendMetricData; @@ -24,6 +26,11 @@ class BackendMetricProvider { virtual BackendMetricData GetBackendMetricData() = 0; }; +template <> +struct ArenaContextType { + static void Destroy(BackendMetricProvider*) {} +}; + } // namespace grpc_core #endif // GRPC_SRC_CORE_EXT_FILTERS_BACKEND_METRICS_BACKEND_METRIC_PROVIDER_H diff --git a/src/core/lib/channel/context.h b/src/core/lib/channel/context.h index 6432efb03a6..f31f6afb938 100644 --- a/src/core/lib/channel/context.h +++ b/src/core/lib/channel/context.h @@ -32,10 +32,6 @@ typedef enum { /// Holds a pointer to ServiceConfigCallData associated with this call. GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA, - /// Holds a pointer to BackendMetricProvider associated with this call on - /// the server. - GRPC_CONTEXT_BACKEND_METRIC_PROVIDER, - GRPC_CONTEXT_COUNT } grpc_context_index; diff --git a/src/cpp/server/server_context.cc b/src/cpp/server/server_context.cc index 69d2f55237f..00fa2769281 100644 --- a/src/cpp/server/server_context.cc +++ b/src/cpp/server/server_context.cc @@ -410,8 +410,7 @@ void ServerContextBase::CreateCallMetricRecorder( auto* backend_metric_state = arena->New(server_metric_recorder); call_metric_recorder_ = backend_metric_state; - grpc_call_context_set(call_.call, GRPC_CONTEXT_BACKEND_METRIC_PROVIDER, - backend_metric_state, nullptr); + arena->SetContext(backend_metric_state); } grpc::string_ref ServerContextBase::ExperimentalGetAuthority() const {