From 9b3f75d322297130ec15559ad48d6c29944e0314 Mon Sep 17 00:00:00 2001 From: Esun Kim Date: Mon, 20 Sep 2021 18:03:17 -0700 Subject: [PATCH] Added absl::Status support to error_utils (#27358) --- src/core/lib/iomgr/error.cc | 42 +++++++++++++++++++------ src/core/lib/iomgr/error.h | 2 +- src/core/lib/transport/error_utils.cc | 27 ++++++++++++++-- src/ruby/spec/client_server_spec.rb | 2 +- test/core/iomgr/error_test.cc | 10 +++--- test/core/transport/error_utils_test.cc | 6 ++-- test/cpp/end2end/end2end_test.cc | 2 +- test/cpp/microbenchmarks/bm_error.cc | 28 ++++++++++------- 8 files changed, 85 insertions(+), 34 deletions(-) diff --git a/src/core/lib/iomgr/error.cc b/src/core/lib/iomgr/error.cc index b4967c32b87..aec30b2498a 100644 --- a/src/core/lib/iomgr/error.cc +++ b/src/core/lib/iomgr/error.cc @@ -87,9 +87,9 @@ absl::Status grpc_wsa_error(const grpc_core::DebugLocation& location, int err, char* utf8_message = gpr_format_message(err); absl::Status s = StatusCreate(absl::StatusCode::kUnknown, "WSA Error", location, {}); - StatusSetInt(&s, grpc_core::StatusIntProperty::WSA_ERROR, err); - StatusSetStr(&s, grpc_core::StatusStrProperty::OS_ERROR, utf8_message); - StatusSetStr(&s, grpc_core::StatusStrProperty::SYSCALL, call_name); + StatusSetInt(&s, grpc_core::StatusIntProperty::kWsaError, err); + StatusSetStr(&s, grpc_core::StatusStrProperty::kOsError, utf8_message); + StatusSetStr(&s, grpc_core::StatusStrProperty::kSyscall, call_name); } #endif @@ -108,6 +108,22 @@ bool grpc_error_get_int(grpc_error_handle error, grpc_error_ints which, *p = *value; return true; } else { + // TODO(veblush): Remove this once absl::Status migration is done + if (which == GRPC_ERROR_INT_GRPC_STATUS) { + switch (error.code()) { + case absl::StatusCode::kOk: + *p = GRPC_STATUS_OK; + return true; + case absl::StatusCode::kResourceExhausted: + *p = GRPC_STATUS_RESOURCE_EXHAUSTED; + return true; + case absl::StatusCode::kCancelled: + *p = GRPC_STATUS_CANCELLED; + return true; + default: + break; + } + } return false; } } @@ -130,6 +146,12 @@ bool grpc_error_get_str(grpc_error_handle error, grpc_error_strs which, *s = grpc_slice_from_copied_buffer(value->c_str(), value->size()); return true; } else { + // TODO(veblush): Remove this once absl::Status migration is done + if (which == GRPC_ERROR_STR_DESCRIPTION && !error.message().empty()) { + *s = grpc_slice_from_copied_buffer(error.message().data(), + error.message().size()); + return true; + } return false; } } @@ -555,11 +577,11 @@ struct special_error_status_map { const special_error_status_map error_status_map[] = { {GRPC_STATUS_OK, "", 0}, // GRPC_ERROR_NONE {GRPC_STATUS_INVALID_ARGUMENT, "", 0}, // GRPC_ERROR_RESERVED_1 - {GRPC_STATUS_RESOURCE_EXHAUSTED, "Out of memory", - strlen("Out of memory")}, // GRPC_ERROR_OOM + {GRPC_STATUS_RESOURCE_EXHAUSTED, "RESOURCE_EXHAUSTED", + strlen("RESOURCE_EXHAUSTED")}, // GRPC_ERROR_OOM {GRPC_STATUS_INVALID_ARGUMENT, "", 0}, // GRPC_ERROR_RESERVED_2 - {GRPC_STATUS_CANCELLED, "Cancelled", - strlen("Cancelled")}, // GRPC_ERROR_CANCELLED + {GRPC_STATUS_CANCELLED, "CANCELLED", + strlen("CANCELLED")}, // GRPC_ERROR_CANCELLED }; bool grpc_error_get_int(grpc_error_handle err, grpc_error_ints which, @@ -631,9 +653,9 @@ grpc_error_handle grpc_error_add_child(grpc_error_handle src, } } -static const char* no_error_string = "\"No Error\""; -static const char* oom_error_string = "\"Out of memory\""; -static const char* cancelled_error_string = "\"Cancelled\""; +static const char* no_error_string = "\"OK\""; +static const char* oom_error_string = "\"RESOURCE_EXHAUSTED\""; +static const char* cancelled_error_string = "\"CANCELLED\""; struct kv_pair { char* key; diff --git a/src/core/lib/iomgr/error.h b/src/core/lib/iomgr/error.h index 29645b9e97b..1811fc71df8 100644 --- a/src/core/lib/iomgr/error.h +++ b/src/core/lib/iomgr/error.h @@ -156,7 +156,7 @@ void grpc_enable_error_creation(); #ifdef GRPC_ERROR_IS_ABSEIL_STATUS #define GRPC_ERROR_NONE absl::OkStatus() -#define GRPC_ERROR_OOM absl::Status(absl::ResourceExhaustedError) +#define GRPC_ERROR_OOM absl::Status(absl::ResourceExhaustedError("")) #define GRPC_ERROR_CANCELLED absl::CancelledError() #define GRPC_ERROR_REF(err) (err) diff --git a/src/core/lib/transport/error_utils.cc b/src/core/lib/transport/error_utils.cc index a826b910a61..d46b63af57d 100644 --- a/src/core/lib/transport/error_utils.cc +++ b/src/core/lib/transport/error_utils.cc @@ -22,6 +22,7 @@ #include +#include "src/core/lib/gprpp/status_helper.h" #include "src/core/lib/iomgr/error_internal.h" #include "src/core/lib/slice/slice_internal.h" #include "src/core/lib/transport/status_conversion.h" @@ -33,7 +34,14 @@ static grpc_error_handle recursively_find_error_with_field( if (grpc_error_get_int(error, which, &unused)) { return error; } - if (grpc_error_is_special(error)) return nullptr; +#ifdef GRPC_ERROR_IS_ABSEIL_STATUS + std::vector children = grpc_core::StatusGetChildren(error); + for (auto child : children) { + grpc_error_handle result = recursively_find_error_with_field(child, which); + if (result != GRPC_ERROR_NONE) return result; + } +#else + if (grpc_error_is_special(error)) return GRPC_ERROR_NONE; // Otherwise, search through its children. uint8_t slot = error->first_err; while (slot != UINT8_MAX) { @@ -44,7 +52,8 @@ static grpc_error_handle recursively_find_error_with_field( if (result) return result; slot = lerr->next; } - return nullptr; +#endif + return GRPC_ERROR_NONE; } void grpc_error_get_status(grpc_error_handle error, grpc_millis deadline, @@ -119,7 +128,12 @@ void grpc_error_get_status(grpc_error_handle error, grpc_millis deadline, if (slice != nullptr) { if (!grpc_error_get_str(found_error, GRPC_ERROR_STR_GRPC_MESSAGE, slice)) { if (!grpc_error_get_str(found_error, GRPC_ERROR_STR_DESCRIPTION, slice)) { +#ifdef GRPC_ERROR_IS_ABSEIL_STATUS + *slice = + grpc_slice_from_copied_string(grpc_error_std_string(error).c_str()); +#else *slice = grpc_slice_from_static_string("unknown error"); +#endif } } } @@ -153,6 +167,14 @@ bool grpc_error_has_clear_grpc_status(grpc_error_handle error) { if (grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, &unused)) { return true; } +#ifdef GRPC_ERROR_IS_ABSEIL_STATUS + std::vector children = grpc_core::StatusGetChildren(error); + for (auto child : children) { + if (grpc_error_has_clear_grpc_status(child)) { + return true; + } + } +#else uint8_t slot = error->first_err; while (slot != UINT8_MAX) { grpc_linked_error* lerr = @@ -162,5 +184,6 @@ bool grpc_error_has_clear_grpc_status(grpc_error_handle error) { } slot = lerr->next; } +#endif return false; } diff --git a/src/ruby/spec/client_server_spec.rb b/src/ruby/spec/client_server_spec.rb index afde5073a66..78b4a7f3052 100644 --- a/src/ruby/spec/client_server_spec.rb +++ b/src/ruby/spec/client_server_spec.rb @@ -311,7 +311,7 @@ shared_examples 'basic GRPC message delivery is OK' do it 'clients can cancel a call on the server' do expected_code = StatusCodes::CANCELLED - expected_details = 'Cancelled' + expected_details = 'CANCELLED' cancel_proc = proc { |call| call.cancel } client_cancel_test(cancel_proc, expected_code, expected_details) end diff --git a/test/core/iomgr/error_test.cc b/test/core/iomgr/error_test.cc index cb3e30e895c..dcc2aead6fc 100644 --- a/test/core/iomgr/error_test.cc +++ b/test/core/iomgr/error_test.cc @@ -28,7 +28,7 @@ static void test_set_get_int() { grpc_error_handle error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Test"); - GPR_ASSERT(error); + GPR_ASSERT(error != GRPC_ERROR_NONE); intptr_t i = 0; GPR_ASSERT(grpc_error_get_int(error, GRPC_ERROR_INT_FILE_LINE, &i)); GPR_ASSERT(i); // line set will never be 0 @@ -110,7 +110,7 @@ static void test_create_referencing() { GRPC_ERROR_STR_GRPC_MESSAGE, grpc_slice_from_static_string("message")); grpc_error_handle parent = GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING("Parent", &child, 1); - GPR_ASSERT(parent); + GPR_ASSERT(parent != GRPC_ERROR_NONE); GRPC_ERROR_UNREF(child); GRPC_ERROR_UNREF(parent); @@ -130,7 +130,7 @@ static void test_create_referencing_many() { grpc_error_handle parent = GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING("Parent", children, 3); - GPR_ASSERT(parent); + GPR_ASSERT(parent != GRPC_ERROR_NONE); for (size_t i = 0; i < 3; ++i) { GRPC_ERROR_UNREF(children[i]); @@ -188,6 +188,8 @@ static void test_os_error() { } static void test_overflow() { + // absl::Status doesn't have a limit so there is no overflow +#ifndef GRPC_ERROR_IS_ABSEIL_STATUS grpc_error_handle error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Overflow"); for (size_t i = 0; i < 150; ++i) { @@ -211,7 +213,7 @@ static void test_overflow() { GPR_ASSERT(i == 10); GRPC_ERROR_UNREF(error); - ; +#endif } int main(int argc, char** argv) { diff --git a/test/core/transport/error_utils_test.cc b/test/core/transport/error_utils_test.cc index d42a575009b..2c33e097642 100644 --- a/test/core/transport/error_utils_test.cc +++ b/test/core/transport/error_utils_test.cc @@ -41,7 +41,7 @@ TEST(ErrorUtilsTest, GrpcSpecialErrorNoneToAbslStatus) { // ---- Asymmetry of conversions of "Special" errors ---- TEST(ErrorUtilsTest, AbslStatusToGrpcErrorDoesNotReturnSpecialVariables) { grpc_error_handle error = - absl_status_to_grpc_error(absl::CancelledError("Cancelled")); + absl_status_to_grpc_error(absl::CancelledError("CANCELLED")); ASSERT_NE(error, GRPC_ERROR_CANCELLED); GRPC_ERROR_UNREF(error); } @@ -49,13 +49,13 @@ TEST(ErrorUtilsTest, AbslStatusToGrpcErrorDoesNotReturnSpecialVariables) { TEST(ErrorUtilsTest, GrpcSpecialErrorCancelledToAbslStatus) { absl::Status status = grpc_error_to_absl_status(GRPC_ERROR_CANCELLED); ASSERT_TRUE(absl::IsCancelled(status)); - ASSERT_EQ(status.message(), "Cancelled"); + ASSERT_EQ(status.message(), "CANCELLED"); } TEST(ErrorUtilsTest, GrpcSpecialErrorOOMToAbslStatus) { absl::Status status = grpc_error_to_absl_status(GRPC_ERROR_OOM); ASSERT_TRUE(absl::IsResourceExhausted(status)); - ASSERT_EQ(status.message(), "Out of memory"); + ASSERT_EQ(status.message(), "RESOURCE_EXHAUSTED"); } // ---- Ordinary statuses ---- diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index c4a287ee6c4..28e49f52e93 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -1630,7 +1630,7 @@ TEST_P(ProxyEnd2endTest, ClientCancelsRpc) { Status s = stub_->Echo(&context, request, &response); cancel_thread.join(); EXPECT_EQ(StatusCode::CANCELLED, s.error_code()); - EXPECT_EQ(s.error_message(), "Cancelled"); + EXPECT_EQ(s.error_message(), "CANCELLED"); } // Server cancels rpc after 1ms diff --git a/test/cpp/microbenchmarks/bm_error.cc b/test/cpp/microbenchmarks/bm_error.cc index 1559e9c1351..0492b1792e0 100644 --- a/test/cpp/microbenchmarks/bm_error.cc +++ b/test/cpp/microbenchmarks/bm_error.cc @@ -28,11 +28,15 @@ #include "test/cpp/microbenchmarks/helpers.h" #include "test/cpp/util/test_config.h" -class ErrorDeleter { +class ErrorHandleHolder { public: - void operator()(grpc_error_handle error) { GRPC_ERROR_UNREF(error); } + explicit ErrorHandleHolder(grpc_error_handle error) : error_(error) {} + ~ErrorHandleHolder() { GRPC_ERROR_UNREF(error_); } + const grpc_error_handle& get() const { return error_; } + + private: + grpc_error_handle error_; }; -typedef std::unique_ptr ErrorPtr; static void BM_ErrorCreateFromStatic(benchmark::State& state) { TrackCounters track_counters; @@ -132,7 +136,7 @@ BENCHMARK(BM_ErrorGetIntFromNoError); static void BM_ErrorGetMissingInt(benchmark::State& state) { TrackCounters track_counters; - ErrorPtr error(grpc_error_set_int( + ErrorHandleHolder error(grpc_error_set_int( GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error"), GRPC_ERROR_INT_INDEX, 1)); for (auto _ : state) { intptr_t value; @@ -144,7 +148,7 @@ BENCHMARK(BM_ErrorGetMissingInt); static void BM_ErrorGetPresentInt(benchmark::State& state) { TrackCounters track_counters; - ErrorPtr error(grpc_error_set_int( + ErrorHandleHolder error(grpc_error_set_int( GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error"), GRPC_ERROR_INT_OFFSET, 1)); for (auto _ : state) { intptr_t value; @@ -180,7 +184,7 @@ class SimpleError { private: const grpc_millis deadline_ = GRPC_MILLIS_INF_FUTURE; - ErrorPtr error_{GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error")}; + ErrorHandleHolder error_{GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error")}; }; class ErrorWithGrpcStatus { @@ -190,7 +194,7 @@ class ErrorWithGrpcStatus { private: const grpc_millis deadline_ = GRPC_MILLIS_INF_FUTURE; - ErrorPtr error_{grpc_error_set_int( + ErrorHandleHolder error_{grpc_error_set_int( GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error"), GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_UNIMPLEMENTED)}; }; @@ -202,7 +206,7 @@ class ErrorWithHttpError { private: const grpc_millis deadline_ = GRPC_MILLIS_INF_FUTURE; - ErrorPtr error_{grpc_error_set_int( + ErrorHandleHolder error_{grpc_error_set_int( GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error"), GRPC_ERROR_INT_HTTP2_ERROR, GRPC_HTTP2_COMPRESSION_ERROR)}; }; @@ -214,11 +218,11 @@ class ErrorWithNestedGrpcStatus { private: const grpc_millis deadline_ = GRPC_MILLIS_INF_FUTURE; - ErrorPtr nested_error_{grpc_error_set_int( + ErrorHandleHolder nested_error_{grpc_error_set_int( GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error"), GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_UNIMPLEMENTED)}; grpc_error_handle nested_errors_[1] = {nested_error_.get()}; - ErrorPtr error_{GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( + ErrorHandleHolder error_{GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( "Error", nested_errors_, 1)}; }; @@ -227,7 +231,7 @@ static void BM_ErrorStringOnNewError(benchmark::State& state) { TrackCounters track_counters; for (auto _ : state) { Fixture fixture; - grpc_error_string(fixture.error()); + grpc_error_std_string(fixture.error()); } track_counters.Finish(state); } @@ -237,7 +241,7 @@ static void BM_ErrorStringRepeatedly(benchmark::State& state) { TrackCounters track_counters; Fixture fixture; for (auto _ : state) { - grpc_error_string(fixture.error()); + grpc_error_std_string(fixture.error()); } track_counters.Finish(state); }