From 9ccbab1899c59245972aae5d4a6662d70cc13377 Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Sat, 1 Nov 2014 20:00:59 +0200 Subject: [PATCH] poll.c:dcb_close Don't call poll_remove_dcb anymore if DCB has already been removed from poll set. mysql_backend.c, mysql_client.c free error message GWBUF after calling handleError readconnroute.c:handleError send error message to client before returning. readwritesplit.c:handleError don't free error message buffer anymore since the caller of handleError frees it. --- server/core/dcb.c | 41 ++-- server/modules/protocol/mysql_backend.c | 15 +- server/modules/protocol/mysql_client.c | 1 + server/modules/routing/readconnroute.c | 40 ++-- .../routing/readwritesplit/readwritesplit.c | 190 +++++++++--------- 5 files changed, 148 insertions(+), 139 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index acd553360..b6584f866 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -1140,8 +1140,28 @@ dcb_close(DCB *dcb) /*< * Stop dcb's listening and modify state accordingly. */ - rc = poll_remove_dcb(dcb); - + if (dcb->state == DCB_STATE_POLLING) + { + rc = poll_remove_dcb(dcb); + + if (rc == 0) { + LOGIF(LD, (skygw_log_write( + LOGFILE_DEBUG, + "%lu [dcb_close] Removed dcb %p in state %s from " + "poll set.", + pthread_self(), + dcb, + STRDCBSTATE(dcb->state)))); + } else { + LOGIF(LE, (skygw_log_write( + LOGFILE_ERROR, + "%lu [dcb_close] Error : Removing dcb %p in state %s from " + "poll set failed.", + pthread_self(), + dcb, + STRDCBSTATE(dcb->state)))); + } + } ss_dassert(dcb->state == DCB_STATE_NOPOLLING || dcb->state == DCB_STATE_ZOMBIE); /** @@ -1153,23 +1173,6 @@ dcb_close(DCB *dcb) } dcb_call_callback(dcb, DCB_REASON_CLOSE); - if (rc == 0) { - LOGIF(LD, (skygw_log_write( - LOGFILE_DEBUG, - "%lu [dcb_close] Removed dcb %p in state %s from " - "poll set.", - pthread_self(), - dcb, - STRDCBSTATE(dcb->state)))); - } else { - LOGIF(LE, (skygw_log_write( - LOGFILE_ERROR, - "%lu [dcb_close] Error : Removing dcb %p in state %s from " - "poll set failed.", - pthread_self(), - dcb, - STRDCBSTATE(dcb->state)))); - } if (dcb->state == DCB_STATE_NOPOLLING) { diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index c6ef05ee8..868647cfb 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -377,7 +377,7 @@ static int gw_read_backend_event(DCB *dcb) { dcb, ERRACT_REPLY_CLIENT, &succp); - + gwbuf_free(errbuf); ss_dassert(!succp); LOGIF(LD, (skygw_log_write( LOGFILE_DEBUG, @@ -459,7 +459,8 @@ static int gw_read_backend_event(DCB *dcb) { dcb, ERRACT_NEW_CONNECTION, &succp); - + gwbuf_free(errbuf); + if (!succp) { spinlock_acquire(&session->ses_lock); @@ -848,7 +849,8 @@ static int gw_error_backend_event(DCB *dcb) dcb, ERRACT_NEW_CONNECTION, &succp); - + gwbuf_free(errbuf); + /** There are not required backends available, close session. */ if (!succp) { spinlock_acquire(&session->ses_lock); @@ -1031,7 +1033,8 @@ gw_backend_hangup(DCB *dcb) ERRACT_NEW_CONNECTION, &succp); - /** There are not required backends available, close session. */ + gwbuf_free(errbuf); + /** There are no required backends available, close session. */ if (!succp) { #if defined(SS_DEBUG) @@ -1039,7 +1042,6 @@ gw_backend_hangup(DCB *dcb) LOGFILE_ERROR, "Backend hangup -> closing session."))); #endif - spinlock_acquire(&session->ses_lock); session->state = SESSION_STATE_STOPPING; spinlock_release(&session->ses_lock); @@ -1176,7 +1178,8 @@ static int backend_write_delayqueue(DCB *dcb) dcb, ERRACT_NEW_CONNECTION, &succp); - + gwbuf_free(errbuf); + if (!succp) { if (session != NULL) diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index a2a70e5c2..364aec3f9 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -859,6 +859,7 @@ int gw_read_client_event( dcb, ERRACT_REPLY_CLIENT, &succp); + gwbuf_free(errbuf); ss_dassert(!succp); dcb_close(dcb); diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index b8aad7ae9..949794e67 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -700,8 +700,8 @@ routeQuery(ROUTER *instance, void *router_session, GWBUF *queue) rc = 0; goto return_rc; } - - switch(mysql_command) { + + switch(mysql_command) { case MYSQL_COM_CHANGE_USER: rc = backend_dcb->func.auth( backend_dcb, @@ -714,7 +714,7 @@ routeQuery(ROUTER *instance, void *router_session, GWBUF *queue) rc = backend_dcb->func.write(backend_dcb, queue); break; } - + CHK_PROTOCOL(((MySQLProtocol*)backend_dcb->protocol)); LOGIF(LD, (skygw_log_write( LOGFILE_DEBUG, @@ -815,21 +815,33 @@ clientReply( * @param action The action: REPLY, REPLY_AND_CLOSE, NEW_CONNECTION * */ -static void -handleError( - ROUTER *instance, - void *router_session, - GWBUF *errbuf, - DCB *backend_dcb, - error_action_t action, - bool *succp) +static void handleError( + ROUTER *instance, + void *router_session, + GWBUF *errbuf, + DCB *backend_dcb, + error_action_t action, + bool *succp) + { - DCB *client = NULL; + DCB *client_dcb; SESSION *session = backend_dcb->session; - client = session->client; + session_state_t sesstate; + + spinlock_acquire(&session->ses_lock); + sesstate = session->state; + client_dcb = session->client; + spinlock_release(&session->ses_lock); + ss_dassert(client_dcb != NULL); + + if (sesstate == SESSION_STATE_ROUTER_READY) + { + CHK_DCB(client_dcb); + client_dcb->func.write(client_dcb, gwbuf_clone(errbuf)); + } + /** false because connection is not available anymore */ *succp = false; - ss_dassert(client != NULL); } /** to be inline'd */ diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index f76eab3d3..8cf2dfb41 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -4023,7 +4023,7 @@ static void rwsplit_process_router_options( * * @param instance The router instance * @param router_session The router session - * @param message The error message to reply + * @param errmsgbuf The error message to reply * @param backend_dcb The backend DCB * @param action The action: REPLY, REPLY_AND_CLOSE, NEW_CONNECTION * @param succp Result of action. @@ -4088,114 +4088,104 @@ static void handleError ( static void handle_error_reply_client( - SESSION* ses, - GWBUF* errmsg) + SESSION* ses, + GWBUF* errmsg) { - session_state_t sesstate; - DCB* client_dcb; - - spinlock_acquire(&ses->ses_lock); - sesstate = ses->state; - client_dcb = ses->client; - spinlock_release(&ses->ses_lock); - - if (sesstate == SESSION_STATE_ROUTER_READY) - { - CHK_DCB(client_dcb); - client_dcb->func.write(client_dcb, errmsg); - } - else - { - while ((errmsg=gwbuf_consume(errmsg, GWBUF_LENGTH(errmsg))) != NULL) - ; - } + session_state_t sesstate; + DCB* client_dcb; + + spinlock_acquire(&ses->ses_lock); + sesstate = ses->state; + client_dcb = ses->client; + spinlock_release(&ses->ses_lock); + + if (sesstate == SESSION_STATE_ROUTER_READY) + { + CHK_DCB(client_dcb); + client_dcb->func.write(client_dcb, gwbuf_clone(errmsg)); + } } /** * This must be called with router lock */ static bool handle_error_new_connection( - ROUTER_INSTANCE* inst, - ROUTER_CLIENT_SES* rses, - DCB* backend_dcb, - GWBUF* errmsg) + ROUTER_INSTANCE* inst, + ROUTER_CLIENT_SES* rses, + DCB* backend_dcb, + GWBUF* errmsg) { - SESSION* ses; - int router_nservers; - int max_nslaves; - int max_slave_rlag; - backend_ref_t* bref; - bool succp; - - ss_dassert(SPINLOCK_IS_LOCKED(&rses->rses_lock)); - - ses = backend_dcb->session; - CHK_SESSION(ses); - - bref = get_bref_from_dcb(rses, backend_dcb); - - /** failed DCB has already been replaced */ - if (bref == NULL) - { - succp = true; - goto return_succp; - } - /** - * 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; - } - - CHK_BACKEND_REF(bref); - - if (BREF_IS_WAITING_RESULT(bref)) - { - DCB* client_dcb; - client_dcb = ses->client; - client_dcb->func.write(client_dcb, errmsg); - bref_clear_state(bref, BREF_WAITING_RESULT); - } - else - { - while ((errmsg=gwbuf_consume(errmsg, GWBUF_LENGTH(errmsg))) != NULL) - ; - } - bref_clear_state(bref, BREF_IN_USE); - bref_set_state(bref, BREF_CLOSED); - /** - * 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(rses, router_nservers); - max_slave_rlag = rses_get_max_replication_lag(rses); - /** - * Try to get replacement slave or at least the minimum - * number of slave connections for router session. - */ - succp = select_connect_backend_servers( - &rses->rses_master_ref, - rses->rses_backend_ref, - router_nservers, - max_nslaves, - max_slave_rlag, - rses->rses_config.rw_slave_select_criteria, - ses, - inst); - + SESSION* ses; + int router_nservers; + int max_nslaves; + int max_slave_rlag; + backend_ref_t* bref; + bool succp; + + ss_dassert(SPINLOCK_IS_LOCKED(&rses->rses_lock)); + + ses = backend_dcb->session; + CHK_SESSION(ses); + + bref = get_bref_from_dcb(rses, backend_dcb); + + /** failed DCB has already been replaced */ + if (bref == NULL) + { + succp = true; + goto return_succp; + } + /** + * 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; + } + + CHK_BACKEND_REF(bref); + + if (BREF_IS_WAITING_RESULT(bref)) + { + DCB* client_dcb; + client_dcb = ses->client; + client_dcb->func.write(client_dcb, gwbuf_clone(errmsg)); + bref_clear_state(bref, BREF_WAITING_RESULT); + } + bref_clear_state(bref, BREF_IN_USE); + bref_set_state(bref, BREF_CLOSED); + /** + * 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(rses, router_nservers); + max_slave_rlag = rses_get_max_replication_lag(rses); + /** + * Try to get replacement slave or at least the minimum + * number of slave connections for router session. + */ + succp = select_connect_backend_servers( + &rses->rses_master_ref, + rses->rses_backend_ref, + router_nservers, + max_nslaves, + max_slave_rlag, + rses->rses_config.rw_slave_select_criteria, + ses, + inst); + return_succp: - return succp; + return succp; }