From 34ac4ee5deff36acbe7e61737acda30851465b9e Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 31 May 2024 14:08:30 -0700 Subject: [PATCH] [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 \