From 40737d67ee93f3c60be1f0ff6486d6f045646312 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 13 Nov 2017 08:02:35 -0800 Subject: Add error string to recv status API --- .../transport/chttp2/transport/chttp2_transport.cc | 9 +++--- src/core/lib/channel/channel_stack.h | 1 + src/core/lib/surface/call.cc | 34 +++++++++++++--------- src/core/lib/transport/error_utils.cc | 8 +++-- src/core/lib/transport/error_utils.h | 8 +++-- 5 files changed, 37 insertions(+), 23 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 034e6ed8ca..d5c61d80ae 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -1756,7 +1756,7 @@ static void send_goaway(grpc_exec_ctx* exec_ctx, grpc_chttp2_transport* t, grpc_http2_error_code http_error; grpc_slice slice; grpc_error_get_status(exec_ctx, error, GRPC_MILLIS_INF_FUTURE, NULL, &slice, - &http_error); + &http_error, NULL); grpc_chttp2_goaway_append(t->last_new_stream_id, (uint32_t)http_error, grpc_slice_ref_internal(slice), &t->qbuf); grpc_chttp2_initiate_write(exec_ctx, t, @@ -2059,7 +2059,7 @@ void grpc_chttp2_cancel_stream(grpc_exec_ctx* exec_ctx, if (s->id != 0) { grpc_http2_error_code http_error; grpc_error_get_status(exec_ctx, due_to_error, s->deadline, NULL, NULL, - &http_error); + &http_error, NULL); grpc_slice_buffer_add( &t->qbuf, grpc_chttp2_rst_stream_create(s->id, (uint32_t)http_error, &s->stats.outgoing)); @@ -2077,7 +2077,8 @@ void grpc_chttp2_fake_status(grpc_exec_ctx* exec_ctx, grpc_chttp2_transport* t, grpc_chttp2_stream* s, grpc_error* error) { grpc_status_code status; grpc_slice slice; - grpc_error_get_status(exec_ctx, error, s->deadline, &status, &slice, NULL); + grpc_error_get_status(exec_ctx, error, s->deadline, &status, &slice, NULL, + NULL); if (status != GRPC_STATUS_OK) { s->seen_error = true; @@ -2243,7 +2244,7 @@ static void close_from_api(grpc_exec_ctx* exec_ctx, grpc_chttp2_transport* t, grpc_status_code grpc_status; grpc_slice slice; grpc_error_get_status(exec_ctx, error, s->deadline, &grpc_status, &slice, - NULL); + NULL, NULL); GPR_ASSERT(grpc_status >= 0 && (int)grpc_status < 100); diff --git a/src/core/lib/channel/channel_stack.h b/src/core/lib/channel/channel_stack.h index aa993112a0..41851382ae 100644 --- a/src/core/lib/channel/channel_stack.h +++ b/src/core/lib/channel/channel_stack.h @@ -84,6 +84,7 @@ typedef struct { typedef struct { grpc_call_stats stats; grpc_status_code final_status; + const char* error_string; } grpc_call_final_info; /* Channel filters specify: diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 9fd4fdbef9..4ce494872a 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -234,6 +234,7 @@ struct grpc_call { struct { grpc_status_code* status; grpc_slice* status_details; + const char** error_string; } client; struct { int* cancelled; @@ -286,7 +287,8 @@ static void receiving_slice_ready(grpc_exec_ctx* exec_ctx, void* bctlp, static void get_final_status(grpc_exec_ctx* exec_ctx, grpc_call* call, void (*set_value)(grpc_status_code code, void* user_data), - void* set_value_user_data, grpc_slice* details); + void* set_value_user_data, grpc_slice* details, + const char** error_string); static void set_status_value_directly(grpc_status_code status, void* dest); static void set_status_from_error(grpc_exec_ctx* exec_ctx, grpc_call* call, status_source source, grpc_error* error); @@ -548,7 +550,8 @@ static void destroy_call(grpc_exec_ctx* exec_ctx, void* call, } get_final_status(exec_ctx, c, set_status_value_directly, - &c->final_info.final_status, NULL); + &c->final_info.final_status, NULL, + &c->final_info.error_string); c->final_info.stats.latency = gpr_time_sub(gpr_now(GPR_CLOCK_MONOTONIC), c->start_time); @@ -734,16 +737,15 @@ static void cancel_with_status(grpc_exec_ctx* exec_ctx, grpc_call* c, * FINAL STATUS CODE MANIPULATION */ -static bool get_final_status_from(grpc_exec_ctx* exec_ctx, grpc_call* call, - grpc_error* error, bool allow_ok_status, - void (*set_value)(grpc_status_code code, - void* user_data), - void* set_value_user_data, - grpc_slice* details) { +static bool get_final_status_from( + grpc_exec_ctx* exec_ctx, grpc_call* call, grpc_error* error, + bool allow_ok_status, + void (*set_value)(grpc_status_code code, void* user_data), + void* set_value_user_data, grpc_slice* details, const char** error_string) { grpc_status_code code; grpc_slice slice = grpc_empty_slice(); grpc_error_get_status(exec_ctx, error, call->send_deadline, &code, &slice, - NULL); + NULL, error_string); if (code == GRPC_STATUS_OK && !allow_ok_status) { return false; } @@ -758,7 +760,8 @@ static bool get_final_status_from(grpc_exec_ctx* exec_ctx, grpc_call* call, static void get_final_status(grpc_exec_ctx* exec_ctx, grpc_call* call, void (*set_value)(grpc_status_code code, void* user_data), - void* set_value_user_data, grpc_slice* details) { + void* set_value_user_data, grpc_slice* details, + const char** error_string) { int i; received_status status[STATUS_SOURCE_COUNT]; for (i = 0; i < STATUS_SOURCE_COUNT; i++) { @@ -782,7 +785,7 @@ static void get_final_status(grpc_exec_ctx* exec_ctx, grpc_call* call, grpc_error_has_clear_grpc_status(status[i].error)) { if (get_final_status_from(exec_ctx, call, status[i].error, allow_ok_status != 0, set_value, - set_value_user_data, details)) { + set_value_user_data, details, error_string)) { return; } } @@ -792,7 +795,7 @@ static void get_final_status(grpc_exec_ctx* exec_ctx, grpc_call* call, if (status[i].is_set) { if (get_final_status_from(exec_ctx, call, status[i].error, allow_ok_status != 0, set_value, - set_value_user_data, details)) { + set_value_user_data, details, error_string)) { return; } } @@ -1333,10 +1336,11 @@ static void post_batch_completion(grpc_exec_ctx* exec_ctx, if (call->is_client) { get_final_status(exec_ctx, call, set_status_value_directly, call->final_op.client.status, - call->final_op.client.status_details); + call->final_op.client.status_details, + call->final_op.client.error_string); } else { get_final_status(exec_ctx, call, set_cancelled_value, - call->final_op.server.cancelled, NULL); + call->final_op.server.cancelled, NULL, NULL); } GRPC_ERROR_UNREF(error); @@ -1993,6 +1997,8 @@ static grpc_call_error call_start_batch(grpc_exec_ctx* exec_ctx, call->final_op.client.status = op->data.recv_status_on_client.status; call->final_op.client.status_details = op->data.recv_status_on_client.status_details; + call->final_op.client.error_string = + op->data.recv_status_on_client.error_string; stream_op->recv_trailing_metadata = true; stream_op->collect_stats = true; stream_op_payload->recv_trailing_metadata.recv_trailing_metadata = diff --git a/src/core/lib/transport/error_utils.cc b/src/core/lib/transport/error_utils.cc index d968b04fd8..15f9875270 100644 --- a/src/core/lib/transport/error_utils.cc +++ b/src/core/lib/transport/error_utils.cc @@ -41,8 +41,12 @@ static grpc_error* recursively_find_error_with_field(grpc_error* error, void grpc_error_get_status(grpc_exec_ctx* exec_ctx, grpc_error* error, grpc_millis deadline, grpc_status_code* code, - grpc_slice* slice, - grpc_http2_error_code* http_error) { + grpc_slice* slice, grpc_http2_error_code* http_error, + const char** full_error_details) { + if (full_error_details != NULL) { + *full_error_details = grpc_error_string(error); + } + // Start with the parent error and recurse through the tree of children // until we find the first one that has a status code. grpc_error* found_error = diff --git a/src/core/lib/transport/error_utils.h b/src/core/lib/transport/error_utils.h index 690e42058a..d47c4f2a3b 100644 --- a/src/core/lib/transport/error_utils.h +++ b/src/core/lib/transport/error_utils.h @@ -30,13 +30,15 @@ extern "C" { /// A utility function to get the status code and message to be returned /// to the application. If not set in the top-level message, looks /// through child errors until it finds the first one with these attributes. -/// All attributes are pulled from the same child error. If any of the -/// attributes (code, msg, http_status) are unneeded, they can be passed as +/// All attributes are pulled from the same child error. full_error_details will +/// be populated with the entire error string. If any of the attributes (code, +/// msg, http_status, full_error_details) are unneeded, they can be passed as /// NULL. void grpc_error_get_status(grpc_exec_ctx* exec_ctx, grpc_error* error, grpc_millis deadline, grpc_status_code* code, grpc_slice* slice, - grpc_http2_error_code* http_status); + grpc_http2_error_code* http_status, + const char** full_error_details); /// A utility function to check whether there is a clear status code that /// doesn't need to be guessed in \a error. This means that \a error or some -- cgit v1.2.3 From d21b96cbc75e7f9d6684907585a490b61325b63f Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 15 Nov 2017 17:05:47 -0800 Subject: Dup the memory --- src/core/lib/transport/error_utils.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src/core') diff --git a/src/core/lib/transport/error_utils.cc b/src/core/lib/transport/error_utils.cc index 061445d44c..bcaedeeac9 100644 --- a/src/core/lib/transport/error_utils.cc +++ b/src/core/lib/transport/error_utils.cc @@ -18,6 +18,7 @@ #include "src/core/lib/transport/error_utils.h" +#include #include "src/core/lib/iomgr/error_internal.h" #include "src/core/lib/transport/status_conversion.h" @@ -44,7 +45,7 @@ void grpc_error_get_status(grpc_exec_ctx* exec_ctx, grpc_error* error, grpc_slice* slice, grpc_http2_error_code* http_error, const char** full_error_details) { if (full_error_details != NULL) { - *full_error_details = grpc_error_string(error); + *full_error_details = gpr_strdup(grpc_error_string(error)); } // Start with the parent error and recurse through the tree of children -- cgit v1.2.3 From 9e2be4998e453ef66a595b230519a006ca257529 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 15 Nov 2017 23:52:55 -0800 Subject: Only alloc if status <> ok --- src/core/lib/transport/error_utils.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/transport/error_utils.cc b/src/core/lib/transport/error_utils.cc index bcaedeeac9..723663a6d7 100644 --- a/src/core/lib/transport/error_utils.cc +++ b/src/core/lib/transport/error_utils.cc @@ -44,10 +44,6 @@ void grpc_error_get_status(grpc_exec_ctx* exec_ctx, grpc_error* error, grpc_millis deadline, grpc_status_code* code, grpc_slice* slice, grpc_http2_error_code* http_error, const char** full_error_details) { - if (full_error_details != NULL) { - *full_error_details = gpr_strdup(grpc_error_string(error)); - } - // Start with the parent error and recurse through the tree of children // until we find the first one that has a status code. grpc_error* found_error = @@ -74,6 +70,10 @@ void grpc_error_get_status(grpc_exec_ctx* exec_ctx, grpc_error* error, } if (code != nullptr) *code = status; + if (full_error_details != NULL && status != GRPC_STATUS_OK) { + *full_error_details = gpr_strdup(grpc_error_string(error)); + } + if (http_error != nullptr) { if (grpc_error_get_int(found_error, GRPC_ERROR_INT_HTTP2_ERROR, &integer)) { *http_error = (grpc_http2_error_code)integer; -- cgit v1.2.3 From f852c060384027ae1c82b1ea5d8dae62c9c5ea3c Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 17 Nov 2017 16:50:12 -0800 Subject: Fix TSAN attempt --- src/core/lib/transport/error_utils.cc | 9 ++++++--- src/core/lib/transport/error_utils.h | 6 +++--- 2 files changed, 9 insertions(+), 6 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/transport/error_utils.cc b/src/core/lib/transport/error_utils.cc index 723663a6d7..acac1330b5 100644 --- a/src/core/lib/transport/error_utils.cc +++ b/src/core/lib/transport/error_utils.cc @@ -43,7 +43,7 @@ static grpc_error* recursively_find_error_with_field(grpc_error* error, void grpc_error_get_status(grpc_exec_ctx* exec_ctx, grpc_error* error, grpc_millis deadline, grpc_status_code* code, grpc_slice* slice, grpc_http2_error_code* http_error, - const char** full_error_details) { + const char** error_string) { // Start with the parent error and recurse through the tree of children // until we find the first one that has a status code. grpc_error* found_error = @@ -70,8 +70,11 @@ void grpc_error_get_status(grpc_exec_ctx* exec_ctx, grpc_error* error, } if (code != nullptr) *code = status; - if (full_error_details != NULL && status != GRPC_STATUS_OK) { - *full_error_details = gpr_strdup(grpc_error_string(error)); + if (error_string != NULL && status != GRPC_STATUS_OK) { + const char* str = grpc_error_string(error); + if (str != nullptr) { + *error_string = gpr_strdup(str); + } } if (http_error != nullptr) { diff --git a/src/core/lib/transport/error_utils.h b/src/core/lib/transport/error_utils.h index d47c4f2a3b..6f21f484e5 100644 --- a/src/core/lib/transport/error_utils.h +++ b/src/core/lib/transport/error_utils.h @@ -30,15 +30,15 @@ extern "C" { /// A utility function to get the status code and message to be returned /// to the application. If not set in the top-level message, looks /// through child errors until it finds the first one with these attributes. -/// All attributes are pulled from the same child error. full_error_details will +/// All attributes are pulled from the same child error. error_string will /// be populated with the entire error string. If any of the attributes (code, -/// msg, http_status, full_error_details) are unneeded, they can be passed as +/// msg, http_status, error_string) are unneeded, they can be passed as /// NULL. void grpc_error_get_status(grpc_exec_ctx* exec_ctx, grpc_error* error, grpc_millis deadline, grpc_status_code* code, grpc_slice* slice, grpc_http2_error_code* http_status, - const char** full_error_details); + const char** error_string); /// A utility function to check whether there is a clear status code that /// doesn't need to be guessed in \a error. This means that \a error or some -- cgit v1.2.3 From 8d1a3ca5fc909899b4895733d496fb563c02dae9 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 20 Nov 2017 15:32:51 -0500 Subject: use aquire release --- src/core/lib/iomgr/error.cc | 2 +- src/core/lib/transport/error_utils.cc | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/error.cc b/src/core/lib/iomgr/error.cc index 581b903f1a..5fe6514802 100644 --- a/src/core/lib/iomgr/error.cc +++ b/src/core/lib/iomgr/error.cc @@ -754,7 +754,7 @@ const char* grpc_error_string(grpc_error* err) { if (!gpr_atm_rel_cas(&err->atomics.error_string, 0, (gpr_atm)out)) { gpr_free(out); - out = (char*)gpr_atm_no_barrier_load(&err->atomics.error_string); + out = (char*)gpr_atm_acq_load(&err->atomics.error_string); } GPR_TIMER_END("grpc_error_string", 0); diff --git a/src/core/lib/transport/error_utils.cc b/src/core/lib/transport/error_utils.cc index acac1330b5..69c8ae6de3 100644 --- a/src/core/lib/transport/error_utils.cc +++ b/src/core/lib/transport/error_utils.cc @@ -71,10 +71,7 @@ void grpc_error_get_status(grpc_exec_ctx* exec_ctx, grpc_error* error, if (code != nullptr) *code = status; if (error_string != NULL && status != GRPC_STATUS_OK) { - const char* str = grpc_error_string(error); - if (str != nullptr) { - *error_string = gpr_strdup(str); - } + *error_string = gpr_strdup(grpc_error_string(error)); } if (http_error != nullptr) { -- cgit v1.2.3 From 11c1c47fe95e6ec8c9ff97d1e702561eee36434e Mon Sep 17 00:00:00 2001 From: Noah Eisen Date: Mon, 20 Nov 2017 21:41:51 -0800 Subject: Fix real TSAN/ASAN bug --- src/core/lib/channel/channel_stack.h | 2 +- src/core/lib/iomgr/error.cc | 2 +- src/core/lib/surface/call.cc | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/channel/channel_stack.h b/src/core/lib/channel/channel_stack.h index 41851382ae..6419f1ac48 100644 --- a/src/core/lib/channel/channel_stack.h +++ b/src/core/lib/channel/channel_stack.h @@ -84,7 +84,7 @@ typedef struct { typedef struct { grpc_call_stats stats; grpc_status_code final_status; - const char* error_string; + const char** error_string; } grpc_call_final_info; /* Channel filters specify: diff --git a/src/core/lib/iomgr/error.cc b/src/core/lib/iomgr/error.cc index 5fe6514802..581b903f1a 100644 --- a/src/core/lib/iomgr/error.cc +++ b/src/core/lib/iomgr/error.cc @@ -754,7 +754,7 @@ const char* grpc_error_string(grpc_error* err) { if (!gpr_atm_rel_cas(&err->atomics.error_string, 0, (gpr_atm)out)) { gpr_free(out); - out = (char*)gpr_atm_acq_load(&err->atomics.error_string); + out = (char*)gpr_atm_no_barrier_load(&err->atomics.error_string); } GPR_TIMER_END("grpc_error_string", 0); diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index c67b577ba1..d9185ec81e 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -551,7 +551,7 @@ static void destroy_call(grpc_exec_ctx* exec_ctx, void* call, get_final_status(exec_ctx, c, set_status_value_directly, &c->final_info.final_status, nullptr, - &c->final_info.error_string); + c->final_info.error_string); c->final_info.stats.latency = gpr_time_sub(gpr_now(GPR_CLOCK_MONOTONIC), c->start_time); -- cgit v1.2.3