From 7f3f30f33304db3c58210af0f355364c0b39fbc5 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Sat, 4 Nov 2017 00:24:12 -0700 Subject: Fix resource_quota_server bug --- src/core/ext/transport/chttp2/transport/chttp2_transport.cc | 11 ++++++++++- src/core/ext/transport/chttp2/transport/internal.h | 8 +++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 02fc53122d..42664a753e 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -1121,6 +1121,7 @@ void grpc_chttp2_add_incoming_goaway(grpc_exec_ctx *exec_ctx, // GRPC_CHTTP2_IF_TRACING( // gpr_log(GPR_DEBUG, "got goaway [%d]: %s", goaway_error, msg)); t->seen_goaway = 1; + t->goaway_error = goaway_error; /* When a client receives a GOAWAY with error code ENHANCE_YOUR_CALM and debug * data equal to "too_many_pings", it should log the occurrence at a log level @@ -2074,7 +2075,6 @@ void grpc_chttp2_fake_status(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, grpc_status_code status; grpc_slice slice; grpc_error_get_status(exec_ctx, error, s->deadline, &status, &slice, NULL); - if (status != GRPC_STATUS_OK) { s->seen_error = true; } @@ -2542,6 +2542,15 @@ static void read_action_locked(grpc_exec_ctx *exec_ctx, void *tp, "Transport closed", &t->closed_with_error, 1); } if (error != GRPC_ERROR_NONE) { + /* If a goaway frame was received, this might be the reason why the read + * failed. Add this info to the error */ + if (t->seen_goaway) { + error = grpc_error_add_child( + error, grpc_error_set_int( + GRPC_ERROR_CREATE_FROM_STATIC_STRING("GOAWAY received"), + GRPC_ERROR_INT_HTTP2_ERROR, (intptr_t)t->goaway_error)); + } + close_transport_locked(exec_ctx, t, GRPC_ERROR_REF(error)); t->endpoint_reading = 0; } else if (t->closed_with_error == GRPC_ERROR_NONE) { diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index 9e0796e820..bd25e51ec0 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -310,6 +310,9 @@ struct grpc_chttp2_transport { bool seen_goaway; /** have we sent a goaway */ grpc_chttp2_sent_goaway_state sent_goaway_state; + /** http2 error code received in the GOAWAY frame. Only relevant if + * seen_goaway is true */ + uint32_t goaway_error; /** are the local settings dirty and need to be sent? */ bool dirtied_local_settings; @@ -376,11 +379,6 @@ struct grpc_chttp2_transport { grpc_chttp2_transport *t, grpc_chttp2_stream *s, grpc_slice slice, int is_last); - /* goaway data */ - grpc_status_code goaway_error; - uint32_t goaway_last_stream_index; - grpc_slice goaway_text; - grpc_chttp2_write_cb *write_cb_pool; /* bdp estimator */ -- cgit v1.2.3 From 0c1d8e2086b055b6e322e00e52223ec5359fca4b Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Mon, 6 Nov 2017 10:25:28 -0800 Subject: Review comment: merge seen_goaway and goaway_error fields on transport --- .../transport/chttp2/transport/chttp2_transport.cc | 32 ++++++++++++---------- src/core/ext/transport/chttp2/transport/internal.h | 10 +++---- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 42664a753e..2a156f5f7e 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -205,6 +205,8 @@ static void destruct_transport(grpc_exec_ctx *exec_ctx, GPR_ASSERT(t->lists[i].tail == NULL); } + GRPC_ERROR_UNREF(t->goaway_error); + GPR_ASSERT(grpc_chttp2_stream_map_size(&t->stream_map) == 0); grpc_chttp2_stream_map_destroy(&t->stream_map); @@ -320,6 +322,7 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, keepalive_watchdog_fired_locked, t, grpc_combiner_scheduler(t->combiner)); + t->goaway_error = GRPC_ERROR_NONE; grpc_chttp2_goaway_parser_init(&t->goaway_parser); grpc_chttp2_hpack_parser_init(exec_ctx, &t->hpack_parser); @@ -1120,8 +1123,16 @@ void grpc_chttp2_add_incoming_goaway(grpc_exec_ctx *exec_ctx, grpc_slice goaway_text) { // GRPC_CHTTP2_IF_TRACING( // gpr_log(GPR_DEBUG, "got goaway [%d]: %s", goaway_error, msg)); - t->seen_goaway = 1; - t->goaway_error = goaway_error; + + // Discard the error from a previous goaway frame (if any) + if (t->goaway_error != GRPC_ERROR_NONE) { + GRPC_ERROR_UNREF(t->goaway_error); + } + t->goaway_error = grpc_error_set_str( + grpc_error_set_int( + GRPC_ERROR_CREATE_FROM_STATIC_STRING("GOAWAY received"), + GRPC_ERROR_INT_HTTP2_ERROR, (intptr_t)goaway_error), + GRPC_ERROR_STR_RAW_BYTES, goaway_text); /* When a client receives a GOAWAY with error code ENHANCE_YOUR_CALM and debug * data equal to "too_many_pings", it should log the occurrence at a log level @@ -1142,14 +1153,8 @@ void grpc_chttp2_add_incoming_goaway(grpc_exec_ctx *exec_ctx, /* lie: use transient failure from the transport to indicate goaway has been * received */ - connectivity_state_set( - exec_ctx, t, GRPC_CHANNEL_TRANSIENT_FAILURE, - grpc_error_set_str( - grpc_error_set_int( - GRPC_ERROR_CREATE_FROM_STATIC_STRING("GOAWAY received"), - GRPC_ERROR_INT_HTTP2_ERROR, (intptr_t)goaway_error), - GRPC_ERROR_STR_RAW_BYTES, goaway_text), - "got_goaway"); + connectivity_state_set(exec_ctx, t, GRPC_CHANNEL_TRANSIENT_FAILURE, + GRPC_ERROR_REF(t->goaway_error), "got_goaway"); } static void maybe_start_some_streams(grpc_exec_ctx *exec_ctx, @@ -2544,11 +2549,8 @@ static void read_action_locked(grpc_exec_ctx *exec_ctx, void *tp, if (error != GRPC_ERROR_NONE) { /* If a goaway frame was received, this might be the reason why the read * failed. Add this info to the error */ - if (t->seen_goaway) { - error = grpc_error_add_child( - error, grpc_error_set_int( - GRPC_ERROR_CREATE_FROM_STATIC_STRING("GOAWAY received"), - GRPC_ERROR_INT_HTTP2_ERROR, (intptr_t)t->goaway_error)); + if (t->goaway_error != GRPC_ERROR_NONE) { + error = grpc_error_add_child(error, GRPC_ERROR_REF(t->goaway_error)); } close_transport_locked(exec_ctx, t, GRPC_ERROR_REF(error)); diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index bd25e51ec0..3a20b653a4 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -306,13 +306,11 @@ struct grpc_chttp2_transport { */ uint32_t write_buffer_size; - /** have we seen a goaway */ - bool seen_goaway; - /** have we sent a goaway */ + /** Set to a grpc_error object if a goaway frame is received. By default, set + * to GRPC_ERROR_NONE */ + grpc_error *goaway_error; + grpc_chttp2_sent_goaway_state sent_goaway_state; - /** http2 error code received in the GOAWAY frame. Only relevant if - * seen_goaway is true */ - uint32_t goaway_error; /** are the local settings dirty and need to be sent? */ bool dirtied_local_settings; -- cgit v1.2.3 From b6727b6f70c40b9f9362745650dd7e86ba74a7f8 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Wed, 8 Nov 2017 15:18:12 -0800 Subject: clang format --- src/core/ext/transport/chttp2/transport/internal.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index 11c7bbec23..60cc280c43 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -308,7 +308,7 @@ struct grpc_chttp2_transport { /** Set to a grpc_error object if a goaway frame is received. By default, set * to GRPC_ERROR_NONE */ - grpc_error *goaway_error; + grpc_error* goaway_error; grpc_chttp2_sent_goaway_state sent_goaway_state; @@ -377,7 +377,7 @@ struct grpc_chttp2_transport { grpc_chttp2_transport* t, grpc_chttp2_stream* s, grpc_slice slice, int is_last); - grpc_chttp2_write_cb *write_cb_pool; + grpc_chttp2_write_cb* write_cb_pool; /* bdp estimator */ grpc_closure next_bdp_ping_timer_expired_locked; -- cgit v1.2.3