From 933676c56c24b9b4f494e2b68d2488d05987bdd8 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 21 Sep 2021 09:48:57 -0700 Subject: [PATCH] Revert "Added absl::Status support to error_utils (#27358)" (#27418) This reverts commit 9b3f75d322297130ec15559ad48d6c29944e0314. --- 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, 34 insertions(+), 85 deletions(-) diff --git a/src/core/lib/iomgr/error.cc b/src/core/lib/iomgr/error.cc index 8282e153f96..f7c9b6c9c41 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::kWsaError, err); - StatusSetStr(&s, grpc_core::StatusStrProperty::kOsError, utf8_message); - StatusSetStr(&s, grpc_core::StatusStrProperty::kSyscall, call_name); + 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); } #endif @@ -108,22 +108,6 @@ 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; } } @@ -146,12 +130,6 @@ 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; } } @@ -577,11 +555,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, "RESOURCE_EXHAUSTED", - strlen("RESOURCE_EXHAUSTED")}, // GRPC_ERROR_OOM + {GRPC_STATUS_RESOURCE_EXHAUSTED, "Out of memory", + strlen("Out of memory")}, // 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, @@ -653,9 +631,9 @@ grpc_error_handle grpc_error_add_child(grpc_error_handle src, } } -static const char* no_error_string = "\"OK\""; -static const char* oom_error_string = "\"RESOURCE_EXHAUSTED\""; -static const char* cancelled_error_string = "\"CANCELLED\""; +static const char* no_error_string = "\"No Error\""; +static const char* oom_error_string = "\"Out of memory\""; +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 2a1d50a923d..efd0042a390 100644 --- a/src/core/lib/iomgr/error.h +++ b/src/core/lib/iomgr/error.h @@ -157,7 +157,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 d46b63af57d..a826b910a61 100644 --- a/src/core/lib/transport/error_utils.cc +++ b/src/core/lib/transport/error_utils.cc @@ -22,7 +22,6 @@ #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" @@ -34,14 +33,7 @@ static grpc_error_handle recursively_find_error_with_field( if (grpc_error_get_int(error, which, &unused)) { return error; } -#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; + if (grpc_error_is_special(error)) return nullptr; // Otherwise, search through its children. uint8_t slot = error->first_err; while (slot != UINT8_MAX) { @@ -52,8 +44,7 @@ static grpc_error_handle recursively_find_error_with_field( if (result) return result; slot = lerr->next; } -#endif - return GRPC_ERROR_NONE; + return nullptr; } void grpc_error_get_status(grpc_error_handle error, grpc_millis deadline, @@ -128,12 +119,7 @@ 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 } } } @@ -167,14 +153,6 @@ 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 = @@ -184,6 +162,5 @@ 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 78b4a7f3052..afde5073a66 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 dcc2aead6fc..cb3e30e895c 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 != GRPC_ERROR_NONE); + GPR_ASSERT(error); 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 != GRPC_ERROR_NONE); + GPR_ASSERT(parent); 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 != GRPC_ERROR_NONE); + GPR_ASSERT(parent); for (size_t i = 0; i < 3; ++i) { GRPC_ERROR_UNREF(children[i]); @@ -188,8 +188,6 @@ 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) { @@ -213,7 +211,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 2c33e097642..d42a575009b 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(), "RESOURCE_EXHAUSTED"); + ASSERT_EQ(status.message(), "Out of memory"); } // ---- Ordinary statuses ---- diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index 28e49f52e93..c4a287ee6c4 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 0492b1792e0..1559e9c1351 100644 --- a/test/cpp/microbenchmarks/bm_error.cc +++ b/test/cpp/microbenchmarks/bm_error.cc @@ -28,15 +28,11 @@ #include "test/cpp/microbenchmarks/helpers.h" #include "test/cpp/util/test_config.h" -class ErrorHandleHolder { +class ErrorDeleter { public: - 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_; + void operator()(grpc_error_handle error) { GRPC_ERROR_UNREF(error); } }; +typedef std::unique_ptr ErrorPtr; static void BM_ErrorCreateFromStatic(benchmark::State& state) { TrackCounters track_counters; @@ -136,7 +132,7 @@ BENCHMARK(BM_ErrorGetIntFromNoError); static void BM_ErrorGetMissingInt(benchmark::State& state) { TrackCounters track_counters; - ErrorHandleHolder error(grpc_error_set_int( + ErrorPtr error(grpc_error_set_int( GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error"), GRPC_ERROR_INT_INDEX, 1)); for (auto _ : state) { intptr_t value; @@ -148,7 +144,7 @@ BENCHMARK(BM_ErrorGetMissingInt); static void BM_ErrorGetPresentInt(benchmark::State& state) { TrackCounters track_counters; - ErrorHandleHolder error(grpc_error_set_int( + ErrorPtr error(grpc_error_set_int( GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error"), GRPC_ERROR_INT_OFFSET, 1)); for (auto _ : state) { intptr_t value; @@ -184,7 +180,7 @@ class SimpleError { private: const grpc_millis deadline_ = GRPC_MILLIS_INF_FUTURE; - ErrorHandleHolder error_{GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error")}; + ErrorPtr error_{GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error")}; }; class ErrorWithGrpcStatus { @@ -194,7 +190,7 @@ class ErrorWithGrpcStatus { private: const grpc_millis deadline_ = GRPC_MILLIS_INF_FUTURE; - ErrorHandleHolder error_{grpc_error_set_int( + ErrorPtr error_{grpc_error_set_int( GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error"), GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_UNIMPLEMENTED)}; }; @@ -206,7 +202,7 @@ class ErrorWithHttpError { private: const grpc_millis deadline_ = GRPC_MILLIS_INF_FUTURE; - ErrorHandleHolder error_{grpc_error_set_int( + ErrorPtr error_{grpc_error_set_int( GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error"), GRPC_ERROR_INT_HTTP2_ERROR, GRPC_HTTP2_COMPRESSION_ERROR)}; }; @@ -218,11 +214,11 @@ class ErrorWithNestedGrpcStatus { private: const grpc_millis deadline_ = GRPC_MILLIS_INF_FUTURE; - ErrorHandleHolder nested_error_{grpc_error_set_int( + ErrorPtr 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()}; - ErrorHandleHolder error_{GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( + ErrorPtr error_{GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( "Error", nested_errors_, 1)}; }; @@ -231,7 +227,7 @@ static void BM_ErrorStringOnNewError(benchmark::State& state) { TrackCounters track_counters; for (auto _ : state) { Fixture fixture; - grpc_error_std_string(fixture.error()); + grpc_error_string(fixture.error()); } track_counters.Finish(state); } @@ -241,7 +237,7 @@ static void BM_ErrorStringRepeatedly(benchmark::State& state) { TrackCounters track_counters; Fixture fixture; for (auto _ : state) { - grpc_error_std_string(fixture.error()); + grpc_error_string(fixture.error()); } track_counters.Finish(state); }