From 74c106eff3d1a43ee9e4823c9dfdd28540591ec1 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 15 Nov 2017 18:43:00 -0800 Subject: [PATCH 1/7] Add error string to C++ --- include/grpc++/impl/codegen/call.h | 6 ++++-- include/grpc++/impl/codegen/status.h | 13 +++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/include/grpc++/impl/codegen/call.h b/include/grpc++/impl/codegen/call.h index af2c2b510c1..6a2ac8b70ca 100644 --- a/include/grpc++/impl/codegen/call.h +++ b/include/grpc++/impl/codegen/call.h @@ -574,7 +574,7 @@ class CallOpClientRecvStatus { op->data.recv_status_on_client.trailing_metadata = metadata_map_->arr(); op->data.recv_status_on_client.status = &status_code_; op->data.recv_status_on_client.status_details = &error_message_; - op->data.recv_status_on_client.error_string = nullptr; + op->data.recv_status_on_client.error_string = &error_string_; op->flags = 0; op->reserved = NULL; } @@ -591,14 +591,16 @@ class CallOpClientRecvStatus { *recv_status_ = Status(static_cast(status_code_), grpc::string(GRPC_SLICE_START_PTR(error_message_), GRPC_SLICE_END_PTR(error_message_)), - binary_error_details); + binary_error_details, grpc::string(error_string_)); g_core_codegen_interface->grpc_slice_unref(error_message_); + g_core_codegen_interface->gpr_free((void*)error_string_); recv_status_ = nullptr; } private: MetadataMap* metadata_map_; Status* recv_status_; + const char* error_string_; grpc_status_code status_code_; grpc_slice error_message_; }; diff --git a/include/grpc++/impl/codegen/status.h b/include/grpc++/impl/codegen/status.h index 6f013cf0ca4..cdaf5e17064 100644 --- a/include/grpc++/impl/codegen/status.h +++ b/include/grpc++/impl/codegen/status.h @@ -46,6 +46,16 @@ class Status { error_message_(error_message), binary_error_details_(error_details) {} + /// Construct an instance with \a code, \a error_message and + /// \a error_details. It is an error to construct an OK status with non-empty + /// \a error_message and/or \a error_details. + Status(StatusCode code, const grpc::string& error_message, + const grpc::string& error_details, const grpc::string& error_string) + : code_(code), + error_message_(error_message), + binary_error_details_(error_details), + error_string_(error_string) {} + // Pre-defined special status objects. /// An OK pre-defined instance. static const Status& OK; @@ -59,6 +69,8 @@ class Status { /// Return the (binary) error details. // Usually it contains a serialized google.rpc.Status proto. grpc::string error_details() const { return binary_error_details_; } + /// Return the full fidelity error string, which includes all child errors. + grpc::string error_string() const { return error_string_; } /// Is the status OK? bool ok() const { return code_ == StatusCode::OK; } @@ -72,6 +84,7 @@ class Status { StatusCode code_; grpc::string error_message_; grpc::string binary_error_details_; + grpc::string error_string_; }; } // namespace grpc From 0d7f5e77c0ca60fd715e00f98b705c99fba2c097 Mon Sep 17 00:00:00 2001 From: Noah Eisen Date: Tue, 21 Nov 2017 12:19:37 -0800 Subject: [PATCH 2/7] No null string ctor --- include/grpc++/impl/codegen/call.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/include/grpc++/impl/codegen/call.h b/include/grpc++/impl/codegen/call.h index 6a2ac8b70ca..4c2e550473b 100644 --- a/include/grpc++/impl/codegen/call.h +++ b/include/grpc++/impl/codegen/call.h @@ -588,10 +588,12 @@ class CallOpClientRecvStatus { binary_error_details = grpc::string(iter->second.begin(), iter->second.length()); } - *recv_status_ = Status(static_cast(status_code_), - grpc::string(GRPC_SLICE_START_PTR(error_message_), - GRPC_SLICE_END_PTR(error_message_)), - binary_error_details, grpc::string(error_string_)); + *recv_status_ = + Status(static_cast(status_code_), + grpc::string(GRPC_SLICE_START_PTR(error_message_), + GRPC_SLICE_END_PTR(error_message_)), + binary_error_details, + error_string_ != nullptr ? grpc::string(error_string_) : ""); g_core_codegen_interface->grpc_slice_unref(error_message_); g_core_codegen_interface->gpr_free((void*)error_string_); recv_status_ = nullptr; From b9cff78f9eaa2647a20813c03fe08199bdc8a668 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 4 Dec 2017 13:16:13 -0800 Subject: [PATCH 3/7] Reviewer feedback --- include/grpc++/impl/codegen/call.h | 22 ++++++++++++-------- include/grpc++/impl/codegen/client_context.h | 9 ++++++++ include/grpc++/impl/codegen/status.h | 13 ------------ 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/include/grpc++/impl/codegen/call.h b/include/grpc++/impl/codegen/call.h index 4c2e550473b..21b1f0a423d 100644 --- a/include/grpc++/impl/codegen/call.h +++ b/include/grpc++/impl/codegen/call.h @@ -558,10 +558,11 @@ class CallOpRecvInitialMetadata { class CallOpClientRecvStatus { public: - CallOpClientRecvStatus() : recv_status_(nullptr) {} + CallOpClientRecvStatus() : recv_status_(nullptr), error_string_(nullptr) {} void ClientRecvStatus(ClientContext* context, Status* status) { - metadata_map_ = &context->trailing_metadata_; + client_context_ = context; + metadata_map_ = &client_context_->trailing_metadata_; recv_status_ = status; error_message_ = g_core_codegen_interface->grpc_empty_slice(); } @@ -588,18 +589,21 @@ class CallOpClientRecvStatus { binary_error_details = grpc::string(iter->second.begin(), iter->second.length()); } - *recv_status_ = - Status(static_cast(status_code_), - grpc::string(GRPC_SLICE_START_PTR(error_message_), - GRPC_SLICE_END_PTR(error_message_)), - binary_error_details, - error_string_ != nullptr ? grpc::string(error_string_) : ""); + *recv_status_ = Status(static_cast(status_code_), + grpc::string(GRPC_SLICE_START_PTR(error_message_), + GRPC_SLICE_END_PTR(error_message_)), + binary_error_details); + client_context_->set_debug_error_string( + error_string_ != nullptr ? grpc::string(error_string_) : ""); g_core_codegen_interface->grpc_slice_unref(error_message_); - g_core_codegen_interface->gpr_free((void*)error_string_); + if (error_string_ != nullptr) { + g_core_codegen_interface->gpr_free((void*)error_string_); + } recv_status_ = nullptr; } private: + ClientContext* client_context_; MetadataMap* metadata_map_; Status* recv_status_; const char* error_string_; diff --git a/include/grpc++/impl/codegen/client_context.h b/include/grpc++/impl/codegen/client_context.h index 22b581cbc55..9fe5f4093e7 100644 --- a/include/grpc++/impl/codegen/client_context.h +++ b/include/grpc++/impl/codegen/client_context.h @@ -348,6 +348,8 @@ class ClientContext { /// Applications never need to call this method. grpc_call* c_call() { return call_; } + grpc::string debug_error_string() const { return debug_error_string_; } + private: // Disallow copy and assign. ClientContext(const ClientContext&); @@ -374,6 +376,11 @@ class ClientContext { template friend class ::grpc::internal::BlockingUnaryCallImpl; + // Used by friend class CallOpClientRecvStatus + void set_debug_error_string(grpc::string debug_error_string) { + debug_error_string_ = debug_error_string; + } + grpc_call* call() const { return call_; } void set_call(grpc_call* call, const std::shared_ptr& channel); @@ -412,6 +419,8 @@ class ClientContext { grpc_compression_algorithm compression_algorithm_; bool initial_metadata_corked_; + + grpc::string debug_error_string_; }; } // namespace grpc diff --git a/include/grpc++/impl/codegen/status.h b/include/grpc++/impl/codegen/status.h index cdaf5e17064..6f013cf0ca4 100644 --- a/include/grpc++/impl/codegen/status.h +++ b/include/grpc++/impl/codegen/status.h @@ -46,16 +46,6 @@ class Status { error_message_(error_message), binary_error_details_(error_details) {} - /// Construct an instance with \a code, \a error_message and - /// \a error_details. It is an error to construct an OK status with non-empty - /// \a error_message and/or \a error_details. - Status(StatusCode code, const grpc::string& error_message, - const grpc::string& error_details, const grpc::string& error_string) - : code_(code), - error_message_(error_message), - binary_error_details_(error_details), - error_string_(error_string) {} - // Pre-defined special status objects. /// An OK pre-defined instance. static const Status& OK; @@ -69,8 +59,6 @@ class Status { /// Return the (binary) error details. // Usually it contains a serialized google.rpc.Status proto. grpc::string error_details() const { return binary_error_details_; } - /// Return the full fidelity error string, which includes all child errors. - grpc::string error_string() const { return error_string_; } /// Is the status OK? bool ok() const { return code_ == StatusCode::OK; } @@ -84,7 +72,6 @@ class Status { StatusCode code_; grpc::string error_message_; grpc::string binary_error_details_; - grpc::string error_string_; }; } // namespace grpc From 18bc278c818bc68f50918c86d428690b20da0b95 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 4 Dec 2017 13:25:14 -0800 Subject: [PATCH 4/7] Add tests --- test/cpp/end2end/end2end_test.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index c71034bbe8e..1608b00fb0d 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -741,6 +741,7 @@ TEST_P(End2endTest, RequestStreamOneRequest) { Status s = stream->Finish(); EXPECT_EQ(response.message(), request.message()); EXPECT_TRUE(s.ok()); + EXPECT_TRUE(context.debug_error_string() == ""); } TEST_P(End2endTest, RequestStreamOneRequestWithCoalescingApi) { @@ -1258,6 +1259,13 @@ TEST_P(End2endTest, ExpectErrorTest) { EXPECT_EQ(iter->code(), s.error_code()); EXPECT_EQ(iter->error_message(), s.error_message()); EXPECT_EQ(iter->binary_error_details(), s.error_details()); + EXPECT_TRUE(context.debug_error_string().find("created") != + std::string::npos); + EXPECT_TRUE(context.debug_error_string().find("file") != std::string::npos); + EXPECT_TRUE(context.debug_error_string().find("line") != std::string::npos); + EXPECT_TRUE(context.debug_error_string().find("status") != + std::string::npos); + EXPECT_TRUE(context.debug_error_string().find("13") != std::string::npos); } } From 64e0b10a972238adffeaf4e4a736ee031d439a31 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 4 Dec 2017 13:26:26 -0800 Subject: [PATCH 5/7] Variable name consistency --- include/grpc++/impl/codegen/call.h | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/include/grpc++/impl/codegen/call.h b/include/grpc++/impl/codegen/call.h index 21b1f0a423d..73773f366a7 100644 --- a/include/grpc++/impl/codegen/call.h +++ b/include/grpc++/impl/codegen/call.h @@ -558,7 +558,8 @@ class CallOpRecvInitialMetadata { class CallOpClientRecvStatus { public: - CallOpClientRecvStatus() : recv_status_(nullptr), error_string_(nullptr) {} + CallOpClientRecvStatus() + : recv_status_(nullptr), debug_error_string_(nullptr) {} void ClientRecvStatus(ClientContext* context, Status* status) { client_context_ = context; @@ -575,7 +576,7 @@ class CallOpClientRecvStatus { op->data.recv_status_on_client.trailing_metadata = metadata_map_->arr(); op->data.recv_status_on_client.status = &status_code_; op->data.recv_status_on_client.status_details = &error_message_; - op->data.recv_status_on_client.error_string = &error_string_; + op->data.recv_status_on_client.error_string = &debug_error_string_; op->flags = 0; op->reserved = NULL; } @@ -594,10 +595,11 @@ class CallOpClientRecvStatus { GRPC_SLICE_END_PTR(error_message_)), binary_error_details); client_context_->set_debug_error_string( - error_string_ != nullptr ? grpc::string(error_string_) : ""); + debug_error_string_ != nullptr ? grpc::string(debug_error_string_) + : ""); g_core_codegen_interface->grpc_slice_unref(error_message_); - if (error_string_ != nullptr) { - g_core_codegen_interface->gpr_free((void*)error_string_); + if (debug_error_string_ != nullptr) { + g_core_codegen_interface->gpr_free((void*)debug_error_string_); } recv_status_ = nullptr; } @@ -606,7 +608,7 @@ class CallOpClientRecvStatus { ClientContext* client_context_; MetadataMap* metadata_map_; Status* recv_status_; - const char* error_string_; + const char* debug_error_string_; grpc_status_code status_code_; grpc_slice error_message_; }; From 11f6780bfd4600398da1ccb6b7c624e6704e8aa5 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 4 Dec 2017 13:29:33 -0800 Subject: [PATCH 6/7] Add comment with API --- include/grpc++/impl/codegen/client_context.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/grpc++/impl/codegen/client_context.h b/include/grpc++/impl/codegen/client_context.h index 9fe5f4093e7..b06b1fa44fe 100644 --- a/include/grpc++/impl/codegen/client_context.h +++ b/include/grpc++/impl/codegen/client_context.h @@ -348,6 +348,11 @@ class ClientContext { /// Applications never need to call this method. grpc_call* c_call() { return call_; } + /// EXPERIMENTAL debugging API + /// + /// if status is not ok() for an RPC, this will return a detailed string + /// of the gRPC Core error that led to the failure. It should not be relied + /// upon for anything other than gaining more debug data in failure cases. grpc::string debug_error_string() const { return debug_error_string_; } private: From 6193c63dabf37023bd9a985f2db807f0551dd1ef Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 8 Dec 2017 10:49:31 -0800 Subject: [PATCH 7/7] Reviewer feedback --- include/grpc++/impl/codegen/call.h | 3 +-- include/grpc++/impl/codegen/client_context.h | 2 +- test/cpp/end2end/end2end_test.cc | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/include/grpc++/impl/codegen/call.h b/include/grpc++/impl/codegen/call.h index 73773f366a7..e581049e7f3 100644 --- a/include/grpc++/impl/codegen/call.h +++ b/include/grpc++/impl/codegen/call.h @@ -595,8 +595,7 @@ class CallOpClientRecvStatus { GRPC_SLICE_END_PTR(error_message_)), binary_error_details); client_context_->set_debug_error_string( - debug_error_string_ != nullptr ? grpc::string(debug_error_string_) - : ""); + debug_error_string_ != nullptr ? debug_error_string_ : ""); g_core_codegen_interface->grpc_slice_unref(error_message_); if (debug_error_string_ != nullptr) { g_core_codegen_interface->gpr_free((void*)debug_error_string_); diff --git a/include/grpc++/impl/codegen/client_context.h b/include/grpc++/impl/codegen/client_context.h index b06b1fa44fe..61d97ce8180 100644 --- a/include/grpc++/impl/codegen/client_context.h +++ b/include/grpc++/impl/codegen/client_context.h @@ -382,7 +382,7 @@ class ClientContext { friend class ::grpc::internal::BlockingUnaryCallImpl; // Used by friend class CallOpClientRecvStatus - void set_debug_error_string(grpc::string debug_error_string) { + void set_debug_error_string(const grpc::string& debug_error_string) { debug_error_string_ = debug_error_string; } diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index 1608b00fb0d..1d63f395dc6 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -741,7 +741,7 @@ TEST_P(End2endTest, RequestStreamOneRequest) { Status s = stream->Finish(); EXPECT_EQ(response.message(), request.message()); EXPECT_TRUE(s.ok()); - EXPECT_TRUE(context.debug_error_string() == ""); + EXPECT_TRUE(context.debug_error_string().empty()); } TEST_P(End2endTest, RequestStreamOneRequestWithCoalescingApi) {