From dd95194a086b81966fd94726c04f53d279e247d8 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 4 Sep 2018 16:26:14 -0700 Subject: [PATCH 1/2] Prefer grpc status over http status test --- .../ext/transport/chttp2/transport/parsing.cc | 2 ++ src/core/lib/surface/call.cc | 4 ++-- test/core/end2end/tests/filter_status_code.cc | 20 ++++++++++++++++++- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc index 1e491d2ef86..205fb8c3700 100644 --- a/src/core/ext/transport/chttp2/transport/parsing.cc +++ b/src/core/ext/transport/chttp2/transport/parsing.cc @@ -393,6 +393,7 @@ error_handler: static void free_timeout(void* p) { gpr_free(p); } static void on_initial_header(void* tp, grpc_mdelem md) { + gpr_log(GPR_INFO, "on initial header"); GPR_TIMER_SCOPE("on_initial_header", 0); grpc_chttp2_transport* t = static_cast(tp); @@ -475,6 +476,7 @@ static void on_initial_header(void* tp, grpc_mdelem md) { } static void on_trailing_header(void* tp, grpc_mdelem md) { + gpr_log(GPR_INFO, "on_trailing_header"); GPR_TIMER_SCOPE("on_trailing_header", 0); grpc_chttp2_transport* t = static_cast(tp); diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index b07c4d6c10b..496de1150a2 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -685,10 +685,10 @@ static void cancel_with_status(grpc_call* c, grpc_status_code status, } static void set_final_status(grpc_call* call, grpc_error* error) { - if (grpc_call_error_trace.enabled()) { + //if (grpc_call_error_trace.enabled()) { gpr_log(GPR_DEBUG, "set_final_status %s", call->is_client ? "CLI" : "SVR"); gpr_log(GPR_DEBUG, "%s", grpc_error_string(error)); - } + //} if (call->is_client) { grpc_error_get_status(error, call->send_deadline, call->final_op.client.status, diff --git a/test/core/end2end/tests/filter_status_code.cc b/test/core/end2end/tests/filter_status_code.cc index ba3cbfa6d11..447ff520ee2 100644 --- a/test/core/end2end/tests/filter_status_code.cc +++ b/test/core/end2end/tests/filter_status_code.cc @@ -249,6 +249,24 @@ typedef struct final_status_data { grpc_call_stack* call; } final_status_data; +static void start_transport_stream_op_batch(grpc_call_element *elem, + grpc_transport_stream_op_batch *op) { + auto* data = static_cast(elem->call_data); + if(data->call == g_server_call_stack) { + gpr_log(GPR_INFO, "here"); + } + if(op->send_initial_metadata) { + auto *batch = op->payload->send_initial_metadata.send_initial_metadata; + gpr_log(GPR_INFO, "init %p %p", batch->idx.named.status, batch->idx.named.grpc_status); + grpc_metadata_batch_substitute(batch, batch->idx.named.status, GRPC_MDELEM_STATUS_404); + } + if(op->send_trailing_metadata) { + auto *batch = op->payload->send_trailing_metadata.send_trailing_metadata; + gpr_log(GPR_INFO, "trai %p %p", batch->idx.named.status, batch->idx.named.grpc_status); + } + grpc_call_next_op(elem, op); +} + static grpc_error* init_call_elem(grpc_call_element* elem, const grpc_call_element_args* args) { final_status_data* data = static_cast(elem->call_data); @@ -307,7 +325,7 @@ static const grpc_channel_filter test_client_filter = { "client_filter_status_code"}; static const grpc_channel_filter test_server_filter = { - grpc_call_next_op, + start_transport_stream_op_batch, grpc_channel_next_op, sizeof(final_status_data), init_call_elem, From 3a41245e465e176dc2cae642cf701f5b476188b6 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 4 Sep 2018 19:18:15 -0700 Subject: [PATCH 2/2] Rectify the condition and add a test --- .../filters/http/client/http_client_filter.cc | 7 +++- .../ext/transport/chttp2/transport/parsing.cc | 2 -- src/core/lib/surface/call.cc | 4 +-- test/core/end2end/tests/filter_status_code.cc | 34 +++++++++++-------- 4 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/core/ext/filters/http/client/http_client_filter.cc b/src/core/ext/filters/http/client/http_client_filter.cc index f44dc032a7e..91fa163fecb 100644 --- a/src/core/ext/filters/http/client/http_client_filter.cc +++ b/src/core/ext/filters/http/client/http_client_filter.cc @@ -79,7 +79,12 @@ struct channel_data { static grpc_error* client_filter_incoming_metadata(grpc_call_element* elem, grpc_metadata_batch* b) { if (b->idx.named.status != nullptr) { - if (grpc_mdelem_eq(b->idx.named.status->md, GRPC_MDELEM_STATUS_200)) { + /* If both gRPC status and HTTP status are provided in the response, we + * should prefer the gRPC status code, as mentioned in + * https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md. + */ + if (b->idx.named.grpc_status != nullptr || + grpc_mdelem_eq(b->idx.named.status->md, GRPC_MDELEM_STATUS_200)) { grpc_metadata_batch_remove(b, b->idx.named.status); } else { char* val = grpc_dump_slice(GRPC_MDVALUE(b->idx.named.status->md), diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc index 205fb8c3700..1e491d2ef86 100644 --- a/src/core/ext/transport/chttp2/transport/parsing.cc +++ b/src/core/ext/transport/chttp2/transport/parsing.cc @@ -393,7 +393,6 @@ error_handler: static void free_timeout(void* p) { gpr_free(p); } static void on_initial_header(void* tp, grpc_mdelem md) { - gpr_log(GPR_INFO, "on initial header"); GPR_TIMER_SCOPE("on_initial_header", 0); grpc_chttp2_transport* t = static_cast(tp); @@ -476,7 +475,6 @@ static void on_initial_header(void* tp, grpc_mdelem md) { } static void on_trailing_header(void* tp, grpc_mdelem md) { - gpr_log(GPR_INFO, "on_trailing_header"); GPR_TIMER_SCOPE("on_trailing_header", 0); grpc_chttp2_transport* t = static_cast(tp); diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 496de1150a2..b07c4d6c10b 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -685,10 +685,10 @@ static void cancel_with_status(grpc_call* c, grpc_status_code status, } static void set_final_status(grpc_call* call, grpc_error* error) { - //if (grpc_call_error_trace.enabled()) { + if (grpc_call_error_trace.enabled()) { gpr_log(GPR_DEBUG, "set_final_status %s", call->is_client ? "CLI" : "SVR"); gpr_log(GPR_DEBUG, "%s", grpc_error_string(error)); - //} + } if (call->is_client) { grpc_error_get_status(error, call->send_deadline, call->final_op.client.status, diff --git a/test/core/end2end/tests/filter_status_code.cc b/test/core/end2end/tests/filter_status_code.cc index 447ff520ee2..5ffc3d00a3f 100644 --- a/test/core/end2end/tests/filter_status_code.cc +++ b/test/core/end2end/tests/filter_status_code.cc @@ -16,6 +16,14 @@ * */ +/* This test verifies - + * 1) grpc_call_final_info passed to the filters on destroying a call contains + * the proper status. + * 2) If the response has both an HTTP status code and a gRPC status code, then + * we should prefer the gRPC status code as mentioned in + * https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md + */ + #include "test/core/end2end/end2end_tests.h" #include @@ -249,20 +257,18 @@ typedef struct final_status_data { grpc_call_stack* call; } final_status_data; -static void start_transport_stream_op_batch(grpc_call_element *elem, - grpc_transport_stream_op_batch *op) { +static void server_start_transport_stream_op_batch( + grpc_call_element* elem, grpc_transport_stream_op_batch* op) { auto* data = static_cast(elem->call_data); - if(data->call == g_server_call_stack) { - gpr_log(GPR_INFO, "here"); - } - if(op->send_initial_metadata) { - auto *batch = op->payload->send_initial_metadata.send_initial_metadata; - gpr_log(GPR_INFO, "init %p %p", batch->idx.named.status, batch->idx.named.grpc_status); - grpc_metadata_batch_substitute(batch, batch->idx.named.status, GRPC_MDELEM_STATUS_404); - } - if(op->send_trailing_metadata) { - auto *batch = op->payload->send_trailing_metadata.send_trailing_metadata; - gpr_log(GPR_INFO, "trai %p %p", batch->idx.named.status, batch->idx.named.grpc_status); + if (data->call == g_server_call_stack) { + if (op->send_initial_metadata) { + auto* batch = op->payload->send_initial_metadata.send_initial_metadata; + if (batch->idx.named.status != nullptr) { + /* Replace the HTTP status with 404 */ + grpc_metadata_batch_substitute(batch, batch->idx.named.status, + GRPC_MDELEM_STATUS_404); + } + } } grpc_call_next_op(elem, op); } @@ -325,7 +331,7 @@ static const grpc_channel_filter test_client_filter = { "client_filter_status_code"}; static const grpc_channel_filter test_server_filter = { - start_transport_stream_op_batch, + server_start_transport_stream_op_batch, grpc_channel_next_op, sizeof(final_status_data), init_call_elem,