From 2a95bf4d9a108a6bf7c8d081039d4bb1fd92fb71 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Thu, 7 Sep 2017 11:26:34 -0700 Subject: grpclb: Don't try to pick from shutdown RR --- .../client_channel/lb_policy/grpclb/grpclb.c | 76 ++++++++++++++-------- .../lb_policy/round_robin/round_robin.c | 16 ++++- 2 files changed, 61 insertions(+), 31 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.c b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.c index 087b4076e2..6d01667f36 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.c @@ -1179,35 +1179,56 @@ static int glb_pick_locked(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, "won't work without it. Failing")); return 0; } - glb_lb_policy *glb_policy = (glb_lb_policy *)pol; - bool pick_done; - + bool pick_done = false; if (glb_policy->rr_policy != NULL) { - if (GRPC_TRACER_ON(grpc_lb_glb_trace)) { - gpr_log(GPR_INFO, "grpclb %p about to PICK from RR %p", - (void *)glb_policy, (void *)glb_policy->rr_policy); + const grpc_connectivity_state rr_connectivity_state = + grpc_lb_policy_check_connectivity_locked(exec_ctx, + glb_policy->rr_policy, NULL); + // The glb_policy->rr_policy may have transition to SHUTDOWN but the + // callback registered to capture this event + // (glb_rr_connectivity_changed_locked) may not have been invoked yet. We + // need to make sure we aren't trying to pick from a RR policy instance + // that's in shutdown. + if (rr_connectivity_state == GRPC_CHANNEL_SHUTDOWN) { + if (GRPC_TRACER_ON(grpc_lb_glb_trace)) { + gpr_log(GPR_INFO, + "grpclb %p NOT picking from from RR %p: RR conn state=%s", + (void *)glb_policy, (void *)glb_policy->rr_policy, + grpc_connectivity_state_name(rr_connectivity_state)); + } + // Attempt to switch over to a pending update. + rr_handover_locked(exec_ctx, glb_policy); + add_pending_pick(&glb_policy->pending_picks, pick_args, target, context, + on_complete); + pick_done = false; + } else { // RR not in shutdown + if (GRPC_TRACER_ON(grpc_lb_glb_trace)) { + gpr_log(GPR_INFO, "grpclb %p about to PICK from RR %p", + (void *)glb_policy, (void *)glb_policy->rr_policy); + } + GRPC_LB_POLICY_REF(glb_policy->rr_policy, "glb_pick"); + + wrapped_rr_closure_arg *wc_arg = + gpr_zalloc(sizeof(wrapped_rr_closure_arg)); + + GRPC_CLOSURE_INIT(&wc_arg->wrapper_closure, wrapped_rr_closure, wc_arg, + grpc_schedule_on_exec_ctx); + wc_arg->rr_policy = glb_policy->rr_policy; + wc_arg->target = target; + wc_arg->context = context; + GPR_ASSERT(glb_policy->client_stats != NULL); + wc_arg->client_stats = + grpc_grpclb_client_stats_ref(glb_policy->client_stats); + wc_arg->wrapped_closure = on_complete; + wc_arg->lb_token_mdelem_storage = pick_args->lb_token_mdelem_storage; + wc_arg->initial_metadata = pick_args->initial_metadata; + wc_arg->free_when_done = wc_arg; + pick_done = + pick_from_internal_rr_locked(exec_ctx, glb_policy, pick_args, + false /* force_async */, target, wc_arg); } - GRPC_LB_POLICY_REF(glb_policy->rr_policy, "glb_pick"); - - wrapped_rr_closure_arg *wc_arg = gpr_zalloc(sizeof(wrapped_rr_closure_arg)); - - GRPC_CLOSURE_INIT(&wc_arg->wrapper_closure, wrapped_rr_closure, wc_arg, - grpc_schedule_on_exec_ctx); - wc_arg->rr_policy = glb_policy->rr_policy; - wc_arg->target = target; - wc_arg->context = context; - GPR_ASSERT(glb_policy->client_stats != NULL); - wc_arg->client_stats = - grpc_grpclb_client_stats_ref(glb_policy->client_stats); - wc_arg->wrapped_closure = on_complete; - wc_arg->lb_token_mdelem_storage = pick_args->lb_token_mdelem_storage; - wc_arg->initial_metadata = pick_args->initial_metadata; - wc_arg->free_when_done = wc_arg; - pick_done = - pick_from_internal_rr_locked(exec_ctx, glb_policy, pick_args, - false /* force_async */, target, wc_arg); - } else { + } else { // glb_policy->rr_policy == NULL if (GRPC_TRACER_ON(grpc_lb_glb_trace)) { gpr_log(GPR_DEBUG, "No RR policy in grpclb instance %p. Adding to grpclb's pending " @@ -1216,11 +1237,10 @@ static int glb_pick_locked(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, } add_pending_pick(&glb_policy->pending_picks, pick_args, target, context, on_complete); - + pick_done = false; if (!glb_policy->started_picking) { start_picking_locked(exec_ctx, glb_policy); } - pick_done = false; } return pick_done; } diff --git a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.c b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.c index 866fb9a1eb..0d70409713 100644 --- a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.c @@ -420,10 +420,11 @@ static int rr_pick_locked(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, grpc_call_context_element *context, void **user_data, grpc_closure *on_complete) { round_robin_lb_policy *p = (round_robin_lb_policy *)pol; - GPR_ASSERT(!p->shutdown); if (GRPC_TRACER_ON(grpc_lb_round_robin_trace)) { - gpr_log(GPR_INFO, "[RR %p] Trying to pick", (void *)pol); + gpr_log(GPR_INFO, "[RR %p] Trying to pick (shutdown: %d)", (void *)pol, + p->shutdown); } + GPR_ASSERT(!p->shutdown); if (p->subchannel_list != NULL) { const size_t next_ready_index = get_next_ready_subchannel_index_locked(p); if (next_ready_index < p->subchannel_list->num_subchannels) { @@ -534,7 +535,12 @@ static grpc_connectivity_state update_lb_connectivity_status_locked( GRPC_CHANNEL_SHUTDOWN, GRPC_ERROR_REF(error), "rr_shutdown"); p->shutdown = true; - new_state = GRPC_CHANNEL_SHUTDOWN; + if (GRPC_TRACER_ON(grpc_lb_round_robin_trace)) { + new_state = GRPC_CHANNEL_SHUTDOWN; + gpr_log(GPR_INFO, + "[RR %p] Shutting down: all subchannels have gone into shutdown", + (void *)p); + } } else if (subchannel_list->num_transient_failures == p->subchannel_list->num_subchannels) { /* 4) TRANSIENT_FAILURE */ grpc_connectivity_state_set(exec_ctx, &p->state_tracker, @@ -755,6 +761,10 @@ static void rr_update_locked(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, return; } grpc_lb_addresses *addresses = arg->value.pointer.p; + if (GRPC_TRACER_ON(grpc_lb_round_robin_trace)) { + gpr_log(GPR_INFO, "[RR %p] Update with %lu addressees (shutdown: %d)", + (void *)p, (unsigned long)addresses->num_addresses, p->shutdown); + } rr_subchannel_list *subchannel_list = rr_subchannel_list_create(p, addresses->num_addresses); if (addresses->num_addresses == 0) { -- cgit v1.2.3 From f6c6b92c02612a3fcce3b52dece257ad34c04cff Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Fri, 3 Nov 2017 07:48:16 -0700 Subject: PR comments --- src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index a5f8c59252..82c9482f76 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -1169,7 +1169,7 @@ static int glb_pick_locked(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, const grpc_connectivity_state rr_connectivity_state = grpc_lb_policy_check_connectivity_locked(exec_ctx, glb_policy->rr_policy, NULL); - // The glb_policy->rr_policy may have transition to SHUTDOWN but the + // The glb_policy->rr_policy may have transitioned to SHUTDOWN but the // callback registered to capture this event // (glb_rr_connectivity_changed_locked) may not have been invoked yet. We // need to make sure we aren't trying to pick from a RR policy instance @@ -1182,9 +1182,9 @@ static int glb_pick_locked(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, grpc_connectivity_state_name(rr_connectivity_state)); } // Attempt to switch over to a pending update. - rr_handover_locked(exec_ctx, glb_policy); add_pending_pick(&glb_policy->pending_picks, pick_args, target, context, on_complete); + rr_handover_locked(exec_ctx, glb_policy); pick_done = false; } else { // RR not in shutdown if (GRPC_TRACER_ON(grpc_lb_glb_trace)) { @@ -1192,10 +1192,8 @@ static int glb_pick_locked(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, (void *)glb_policy, (void *)glb_policy->rr_policy); } GRPC_LB_POLICY_REF(glb_policy->rr_policy, "glb_pick"); - - wrapped_rr_closure_arg *wc_arg = - (wrapped_rr_closure_arg *)gpr_zalloc(sizeof(wrapped_rr_closure_arg)); - + wrapped_rr_closure_arg *wc_arg = + (wrapped_rr_closure_arg *)gpr_zalloc(sizeof(wrapped_rr_closure_arg)); GRPC_CLOSURE_INIT(&wc_arg->wrapper_closure, wrapped_rr_closure, wc_arg, grpc_schedule_on_exec_ctx); wc_arg->rr_policy = glb_policy->rr_policy; -- cgit v1.2.3 From 0a9c74f0038a84a4b8d1c9b2a46b3d176c86a8b0 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Fri, 3 Nov 2017 08:55:08 -0700 Subject: PR comments, bis --- src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc | 2 -- 1 file changed, 2 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index 82c9482f76..6cb429ae2e 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -1181,10 +1181,8 @@ static int glb_pick_locked(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, (void *)glb_policy, (void *)glb_policy->rr_policy, grpc_connectivity_state_name(rr_connectivity_state)); } - // Attempt to switch over to a pending update. add_pending_pick(&glb_policy->pending_picks, pick_args, target, context, on_complete); - rr_handover_locked(exec_ctx, glb_policy); pick_done = false; } else { // RR not in shutdown if (GRPC_TRACER_ON(grpc_lb_glb_trace)) { -- cgit v1.2.3