From 0fab454e66ddc41a527778d38077a107baec8008 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Wed, 7 Dec 2016 08:53:43 +0200 Subject: [PATCH] Close the DCB and the related backend at the same time Closing the DCB and the backend reference that uses it at the same time makes the error handling code clearer and removes some of the assumptions that the code made. It will cause the DCB to be closed in multiple places but the logic of why a DCB is being closed is more visible from the code. This change should remove all cases where a DCB is closed without a tightly coupled backend reference. --- .../routing/readwritesplit/readwritesplit.c | 68 +++++-------------- 1 file changed, 16 insertions(+), 52 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 89629a7b2..0320b60dd 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -4473,15 +4473,17 @@ static void handleError(ROUTER *instance, void *router_session, } session = problem_dcb->session; - bool close_dcb = true; backend_ref_t *bref = get_bref_from_dcb(rses, problem_dcb); - if (session == NULL || rses == NULL) + if (session == NULL) { + MXS_ERROR("Session of DCB %p is NULL, won't close the DCB.", problem_dcb); + ss_dassert(false); *succp = false; } else if (DCB_ROLE_CLIENT_HANDLER == problem_dcb->dcb_role) { + dcb_close(problem_dcb); *succp = false; } else @@ -4529,6 +4531,9 @@ static void handleError(ROUTER *instance, void *router_session, if (bref != NULL) { CHK_BACKEND_REF(bref); + RW_CHK_DCB(bref, problem_dcb); + dcb_close(problem_dcb); + RW_CLOSE_BREF(bref); close_failed_bref(bref, true); } else @@ -4545,21 +4550,13 @@ static void handleError(ROUTER *instance, void *router_session, *succp = handle_error_new_connection(inst, &rses, problem_dcb, errmsgbuf); } - RW_CHK_DCB(bref, problem_dcb); - if (bref) { /** This is a valid DCB for a backend ref */ - - if (!BREF_IS_IN_USE(bref) || bref->bref_dcb != problem_dcb) + if (BREF_IS_IN_USE(bref) && bref->bref_dcb == problem_dcb) { - /** The backend is closed or the reference was replaced */ - dcb_close(problem_dcb); - RW_CLOSE_BREF(bref); - } - else - { - MXS_ERROR("Backend '%s' is still in use and points to the problem DCB. Not closing.", + ss_dassert(false); + MXS_ERROR("Backend '%s' is still in use and points to the problem DCB.", bref->bref_backend->backend_server->unique_name); } } @@ -4571,29 +4568,13 @@ static void handleError(ROUTER *instance, void *router_session, MXS_ERROR("DCB connected to '%s' is not in use by the router " "session, not closing it. DCB is in state '%s'", remote, STRDCBSTATE(problem_dcb->state)); - MXS_ERROR("Backends currently in use:"); - - for (int i = 0; i < rses->rses_nbackends; i++) - { - dcb_state_t state = DCB_STATE_UNDEFINED; - if (BREF_IS_IN_USE(&rses->rses_backend_ref[i])) - { - state = rses->rses_backend_ref[i].bref_dcb->state; - } - - MXS_ERROR("%p: %s - %p", &rses->rses_backend_ref[i], STRDCBSTATE(state), - rses->rses_backend_ref[i].bref_dcb); - } } - - close_dcb = false; break; } case ERRACT_REPLY_CLIENT: { handle_error_reply_client(session, rses, problem_dcb, errmsgbuf); - close_dcb = false; *succp = false; /*< no new backend servers were made available */ break; } @@ -4605,12 +4586,6 @@ static void handleError(ROUTER *instance, void *router_session, } } - if (close_dcb) - { - RW_CHK_DCB(bref, problem_dcb); - dcb_close(problem_dcb); - RW_CLOSE_BREF(bref); - } rses_end_locked_router_action(rses); } @@ -4686,9 +4661,12 @@ static bool handle_error_new_connection(ROUTER_INSTANCE *inst, /** * If bref == NULL it has been replaced already with another one. + * + * NOTE: This can never happen. */ if ((bref = get_bref_from_dcb(myrses, backend_dcb)) == NULL) { + ss_dassert(false); succp = true; goto return_succp; } @@ -4705,25 +4683,11 @@ static bool handle_error_new_connection(ROUTER_INSTANCE *inst, client_dcb->func.write(client_dcb, gwbuf_clone(errmsg)); } + RW_CHK_DCB(bref, backend_dcb); + dcb_close(backend_dcb); + RW_CLOSE_BREF(bref); close_failed_bref(bref, false); - /** - * Error handler is already called for this DCB because - * it's not polling anymore. It can be assumed that - * it succeed because rses isn't closed. - */ - if (backend_dcb->state != DCB_STATE_POLLING) - { - succp = true; - goto return_succp; - } - /** - * Remove callback because this DCB won't be used - * unless it is reconnected later, and then the callback - * is set again. - */ - dcb_remove_callback(backend_dcb, DCB_REASON_NOT_RESPONDING, - &router_handle_state_switch, (void *)bref); router_nservers = router_get_servercount(inst); max_nslaves = rses_get_max_slavecount(myrses, router_nservers); max_slave_rlag = rses_get_max_replication_lag(myrses);