From c6058b5e6b0f53d45794c01320b1acdaba232772 Mon Sep 17 00:00:00 2001 From: Esun Kim Date: Wed, 15 Jun 2022 15:06:15 -0700 Subject: [PATCH] Enable GRPC_ERROR_IS_ABSEIL_STATUS (#29869) * Enable GRPC_ERROR_IS_ABSEIL_STATUS * Sanitize * Fix ServerRequestCallTest --- BUILD | 15 ++++++++++++--- bazel/grpc_build_system.bzl | 2 +- include/grpc/impl/codegen/port_platform.h | 7 ++++--- .../ext/filters/client_channel/backup_poller.cc | 2 ++ .../ext/filters/client_channel/dynamic_filters.cc | 2 ++ src/core/ext/filters/deadline/deadline_filter.cc | 1 + .../message_compress/message_compress_filter.cc | 1 + .../message_compress/message_decompress_filter.cc | 1 + .../chttp2/transport/chttp2_transport.cc | 1 + .../ext/transport/chttp2/transport/flow_control.h | 2 ++ .../ext/transport/chttp2/transport/frame_data.cc | 1 + .../transport/chttp2/transport/hpack_parser.cc | 1 + .../ext/transport/chttp2/transport/parsing.cc | 1 + .../cronet/transport/cronet_transport.cc | 1 + src/core/lib/surface/completion_queue.cc | 1 + src/core/lib/transport/byte_stream.cc | 2 ++ src/core/lib/transport/error_utils.cc | 8 +++++++- src/cpp/client/secure_credentials.cc | 2 +- .../grpcio_tests/tests/unit/_rpc_part_1_test.py | 6 ++---- src/ruby/lib/grpc/errors.rb | 2 +- test/cpp/server/server_request_call_test.cc | 10 +++++++++- .../linux/grpc_bazel_build_in_docker.sh | 2 +- 22 files changed, 55 insertions(+), 16 deletions(-) diff --git a/BUILD b/BUILD index d0a6c546f2b..4d96c66378a 100644 --- a/BUILD +++ b/BUILD @@ -144,8 +144,8 @@ config_setting( ) config_setting( - name = "use_abseil_status", - values = {"define": "use_abseil_status=true"}, + name = "disable_use_abseil_status", + values = {"define": "use_abseil_status=false"}, ) python_config_settings() @@ -2716,6 +2716,7 @@ grpc_cc_library( "slice_buffer", "slice_refcount", "sockaddr_utils", + "status_helper", "table", "thread_quota", "time", @@ -3268,7 +3269,10 @@ grpc_cc_library( hdrs = [ "src/core/ext/filters/deadline/deadline_filter.h", ], - external_deps = ["absl/types:optional"], + external_deps = [ + "absl/status", + "absl/types:optional", + ], language = "c++", tags = ["grpc-autodeps"], deps = [ @@ -4720,6 +4724,7 @@ grpc_cc_library( ], external_deps = [ "absl/memory", + "absl/status", "absl/status:statusor", "absl/strings", ], @@ -6057,6 +6062,7 @@ grpc_cc_library( "resource_quota_trace", "slice", "slice_refcount", + "status_helper", "time", "uri_parser", "useful", @@ -6280,6 +6286,7 @@ grpc_cc_library( hdrs = GRPCXX_HDRS, external_deps = [ "absl/base:core_headers", + "absl/status", "absl/strings", "absl/synchronization", "absl/memory", @@ -6323,6 +6330,7 @@ grpc_cc_library( hdrs = GRPCXX_HDRS, external_deps = [ "absl/base:core_headers", + "absl/status", "absl/strings", "absl/synchronization", "absl/memory", @@ -6580,6 +6588,7 @@ grpc_cc_library( ], external_deps = [ "absl/base:core_headers", + "absl/status", "absl/time", "absl/types:optional", "upb_lib", diff --git a/bazel/grpc_build_system.bzl b/bazel/grpc_build_system.bzl index c09d2bd3921..a51f1677c08 100644 --- a/bazel/grpc_build_system.bzl +++ b/bazel/grpc_build_system.bzl @@ -178,7 +178,7 @@ def grpc_cc_library( "//conditions:default": [], }) + select({ - "//:use_abseil_status": ["GRPC_ERROR_IS_ABSEIL_STATUS=1"], + "//:disable_use_abseil_status": ["GRPC_ERROR_IS_NOT_ABSEIL_STATUS=1"], "//conditions:default": [], }), hdrs = hdrs + public_hdrs, diff --git a/include/grpc/impl/codegen/port_platform.h b/include/grpc/impl/codegen/port_platform.h index a4bd31d1524..ae0e35cfb7f 100644 --- a/include/grpc/impl/codegen/port_platform.h +++ b/include/grpc/impl/codegen/port_platform.h @@ -49,10 +49,11 @@ #endif // GPR_ABSEIL_SYNC /* - * Defines GRPC_ERROR_IS_ABSEIL_STATUS to use absl::Status for grpc_error_handle + * Defines GRPC_ERROR_IS_NOT_ABSEIL_STATUS to not use absl::Status for + * grpc_error_handle. This is a temporary knob for migration process. */ -#ifndef GRPC_ERROR_IS_ABSEIL_STATUS -// #define GRPC_ERROR_IS_ABSEIL_STATUS 1 +#ifndef GRPC_ERROR_IS_NOT_ABSEIL_STATUS +#define GRPC_ERROR_IS_ABSEIL_STATUS 1 #endif /* Get windows.h included everywhere (we need it) */ diff --git a/src/core/ext/filters/client_channel/backup_poller.cc b/src/core/ext/filters/client_channel/backup_poller.cc index 394d075079a..1ee9d136ab3 100644 --- a/src/core/ext/filters/client_channel/backup_poller.cc +++ b/src/core/ext/filters/client_channel/backup_poller.cc @@ -22,6 +22,8 @@ #include +#include "absl/status/status.h" + #include #include #include diff --git a/src/core/ext/filters/client_channel/dynamic_filters.cc b/src/core/ext/filters/client_channel/dynamic_filters.cc index f312ec7c6da..be1d2589f5e 100644 --- a/src/core/ext/filters/client_channel/dynamic_filters.cc +++ b/src/core/ext/filters/client_channel/dynamic_filters.cc @@ -24,6 +24,8 @@ #include #include +#include "absl/status/status.h" + #include #include diff --git a/src/core/ext/filters/deadline/deadline_filter.cc b/src/core/ext/filters/deadline/deadline_filter.cc index 369cc7fd218..2e34c08c53c 100644 --- a/src/core/ext/filters/deadline/deadline_filter.cc +++ b/src/core/ext/filters/deadline/deadline_filter.cc @@ -20,6 +20,7 @@ #include +#include "absl/status/status.h" #include "absl/types/optional.h" #include diff --git a/src/core/ext/filters/http/message_compress/message_compress_filter.cc b/src/core/ext/filters/http/message_compress/message_compress_filter.cc index aa3eb7cc313..e31f34ddb7c 100644 --- a/src/core/ext/filters/http/message_compress/message_compress_filter.cc +++ b/src/core/ext/filters/http/message_compress/message_compress_filter.cc @@ -28,6 +28,7 @@ #include #include "absl/meta/type_traits.h" +#include "absl/status/status.h" #include "absl/types/optional.h" #include diff --git a/src/core/ext/filters/http/message_compress/message_decompress_filter.cc b/src/core/ext/filters/http/message_compress/message_decompress_filter.cc index 508d58f654d..277989004df 100644 --- a/src/core/ext/filters/http/message_compress/message_decompress_filter.cc +++ b/src/core/ext/filters/http/message_compress/message_decompress_filter.cc @@ -27,6 +27,7 @@ #include #include +#include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" #include "absl/types/optional.h" diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index b8ab5156bab..e17a0b36bff 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -65,6 +65,7 @@ #include "src/core/lib/gprpp/manual_constructor.h" #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted.h" +#include "src/core/lib/gprpp/status_helper.h" #include "src/core/lib/gprpp/time.h" #include "src/core/lib/http/parser.h" #include "src/core/lib/iomgr/combiner.h" diff --git a/src/core/ext/transport/chttp2/transport/flow_control.h b/src/core/ext/transport/chttp2/transport/flow_control.h index 185f18b38ca..c50aa5f1e23 100644 --- a/src/core/ext/transport/chttp2/transport/flow_control.h +++ b/src/core/ext/transport/chttp2/transport/flow_control.h @@ -26,6 +26,8 @@ #include +#include "absl/status/status.h" + #include "src/core/ext/transport/chttp2/transport/http2_settings.h" #include "src/core/lib/debug/trace.h" #include "src/core/lib/gprpp/time.h" diff --git a/src/core/ext/transport/chttp2/transport/frame_data.cc b/src/core/ext/transport/chttp2/transport/frame_data.cc index b3eb18c1d4b..8cb4354f37f 100644 --- a/src/core/ext/transport/chttp2/transport/frame_data.cc +++ b/src/core/ext/transport/chttp2/transport/frame_data.cc @@ -23,6 +23,7 @@ #include #include "absl/base/attributes.h" +#include "absl/status/status.h" #include "absl/strings/str_format.h" #include diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser.cc b/src/core/ext/transport/chttp2/transport/hpack_parser.cc index e73d3753ca6..5354338cba3 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_parser.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_parser.cc @@ -31,6 +31,7 @@ #include #include "absl/base/attributes.h" +#include "absl/status/status.h" #include "absl/strings/match.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc index 21577204811..d91af3801be 100644 --- a/src/core/ext/transport/chttp2/transport/parsing.cc +++ b/src/core/ext/transport/chttp2/transport/parsing.cc @@ -24,6 +24,7 @@ #include #include "absl/base/attributes.h" +#include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.cc b/src/core/ext/transport/cronet/transport/cronet_transport.cc index 45ddfcc8f02..2fe5fdf311e 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.cc +++ b/src/core/ext/transport/cronet/transport/cronet_transport.cc @@ -29,6 +29,7 @@ #include #include +#include "absl/status/status.h" #include "absl/strings/match.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" diff --git a/src/core/lib/surface/completion_queue.cc b/src/core/lib/surface/completion_queue.cc index c4255acaca0..2e9f7a5be05 100644 --- a/src/core/lib/surface/completion_queue.cc +++ b/src/core/lib/surface/completion_queue.cc @@ -29,6 +29,7 @@ #include #include +#include "absl/status/status.h" #include "absl/strings/str_format.h" #include "absl/strings/str_join.h" diff --git a/src/core/lib/transport/byte_stream.cc b/src/core/lib/transport/byte_stream.cc index 3b909182f69..ac49481f4dc 100644 --- a/src/core/lib/transport/byte_stream.cc +++ b/src/core/lib/transport/byte_stream.cc @@ -23,6 +23,8 @@ #include #include +#include "absl/status/status.h" + #include #include diff --git a/src/core/lib/transport/error_utils.cc b/src/core/lib/transport/error_utils.cc index 49a964ab165..4ae57c03b8d 100644 --- a/src/core/lib/transport/error_utils.cc +++ b/src/core/lib/transport/error_utils.cc @@ -22,13 +22,19 @@ #include +#include + #include "absl/strings/string_view.h" #include -#include "src/core/lib/iomgr/error_internal.h" +#include "src/core/lib/gprpp/status_helper.h" #include "src/core/lib/transport/status_conversion.h" +#ifndef GRPC_ERROR_IS_ABSEIL_STATUS +#include "src/core/lib/iomgr/error_internal.h" +#endif + static grpc_error_handle recursively_find_error_with_field( grpc_error_handle error, grpc_error_ints which) { intptr_t unused; diff --git a/src/cpp/client/secure_credentials.cc b/src/cpp/client/secure_credentials.cc index 012a8f7c62e..4db969b6b8f 100644 --- a/src/cpp/client/secure_credentials.cc +++ b/src/cpp/client/secure_credentials.cc @@ -39,7 +39,7 @@ #include #include #include -// TODO(yashykt): We shouldn't be including "src/core" headers. + #include "src/core/lib/gpr/env.h" #include "src/core/lib/iomgr/closure.h" #include "src/core/lib/iomgr/error.h" diff --git a/src/python/grpcio_tests/tests/unit/_rpc_part_1_test.py b/src/python/grpcio_tests/tests/unit/_rpc_part_1_test.py index 1c0886cc069..c3354a621a1 100644 --- a/src/python/grpcio_tests/tests/unit/_rpc_part_1_test.py +++ b/src/python/grpcio_tests/tests/unit/_rpc_part_1_test.py @@ -114,10 +114,8 @@ class RPCPart1Test(BaseRPCTest, unittest.TestCase): # sanity checks on to make sure returned string contains default members # of the error debug_error_string = exception_context.exception.debug_error_string() - self.assertIn('created', debug_error_string) - self.assertIn('description', debug_error_string) - self.assertIn('file', debug_error_string) - self.assertIn('file_line', debug_error_string) + self.assertIn('grpc_status', debug_error_string) + self.assertIn('grpc_message', debug_error_string) def testFailedUnaryRequestFutureUnaryResponse(self): request = b'\x37\x17' diff --git a/src/ruby/lib/grpc/errors.rb b/src/ruby/lib/grpc/errors.rb index 1ab0811d327..1fceb6ba447 100644 --- a/src/ruby/lib/grpc/errors.rb +++ b/src/ruby/lib/grpc/errors.rb @@ -43,7 +43,7 @@ module GRPC debug_error_string = nil) exception_message = "#{code}:#{details}" if debug_error_string - exception_message += ". debug_error_string:#{debug_error_string}" + exception_message += ". debug_error_string:{#{debug_error_string}}" end super(exception_message) @code = code diff --git a/test/cpp/server/server_request_call_test.cc b/test/cpp/server/server_request_call_test.cc index 467188b2e72..b9875efd08c 100644 --- a/test/cpp/server/server_request_call_test.cc +++ b/test/cpp/server/server_request_call_test.cc @@ -135,7 +135,15 @@ TEST(ServerRequestCallTest, ShortDeadlineDoesNotCauseOkayFalse) { ctx.set_deadline(std::chrono::system_clock::now() + std::chrono::milliseconds(1)); grpc::Status status = stub->Echo(&ctx, request, &response); - EXPECT_EQ(StatusCode::DEADLINE_EXCEEDED, status.error_code()); + if (i == 0 && status.error_code() == grpc::OK) { + // This first call may take more time to trigger a deadline excceded + // because code needs various initiations so it can get OK instead of + // DEADLINE_EXCEEDED. Server-side delay isn't affected so a call + // would be complete before the client-side deadline timer is fired. + // To prevent this case, OK is also accepted for the first call only. + } else { + EXPECT_EQ(StatusCode::DEADLINE_EXCEEDED, status.error_code()); + } gpr_log(GPR_INFO, "Success."); } gpr_log(GPR_INFO, "Done sending RPCs."); diff --git a/tools/internal_ci/linux/grpc_bazel_build_in_docker.sh b/tools/internal_ci/linux/grpc_bazel_build_in_docker.sh index 71b585a8fc1..86664bab613 100755 --- a/tools/internal_ci/linux/grpc_bazel_build_in_docker.sh +++ b/tools/internal_ci/linux/grpc_bazel_build_in_docker.sh @@ -35,7 +35,7 @@ python3 tools/run_tests/python_utils/bazel_report_helper.py --report_path bazel_ bazel_build_with_abseil_status/bazel_wrapper \ --bazelrc=tools/remote_build/include/test_locally_with_resultstore_results.bazelrc \ build \ - --define=use_strict_warning=true --define=use_abseil_status=true \ + --define=use_strict_warning=true --define=use_abseil_status=false \ -- \ //src/core/... \ //src/compiler/... \