From e944ae2e70e1f1308361e634816edf300d3c0e78 Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Thu, 3 Jul 2014 00:43:30 +0300 Subject: [PATCH] http://bugs.skysql.com/show_bug.cgi?id=453 Fixed bug in session command resul handling. In case where backend sent error message the session command cursor wasn't updated properly. Added check to bref_clear_state, if bref's waiter counter would go negative, decrement to global operation counter is skipped. --- server/core/dcb.c | 34 +++++++--- .../include/mysql_client_server_protocol.h | 1 + server/modules/protocol/mysql_backend.c | 68 +++++++++++-------- .../routing/readwritesplit/readwritesplit.c | 34 ++++++---- 4 files changed, 85 insertions(+), 52 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 26eac3415..26544ed10 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -957,18 +957,34 @@ int below_water; } } /* if (dcb->writeq) */ - if (saved_errno != 0 && - queue != NULL && - saved_errno != EAGAIN && + if (saved_errno != 0 && + queue != NULL && + saved_errno != EAGAIN && saved_errno != EWOULDBLOCK) { - LOGIF(LE, (skygw_log_write_flush( - LOGFILE_ERROR, - "Error : Writing to %s socket failed due %d, %s.", - dcb_isclient(dcb) ? "client" : "backend server", - saved_errno, - strerror(saved_errno)))); + bool dolog = true; + /** + * Do not log if writing COM_QUIT to backend failed. + */ + if (GWBUF_IS_TYPE_MYSQL(queue)) + { + uint8_t* data = GWBUF_DATA(queue); + + if (data[4] == 0x01) + { + dolog = false; + } + } + if (dolog) + { + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Error : Writing to %s socket failed due %d, %s.", + dcb_isclient(dcb) ? "client" : "backend server", + saved_errno, + strerror(saved_errno)))); + } spinlock_release(&dcb->writeqlock); return 0; } diff --git a/server/modules/include/mysql_client_server_protocol.h b/server/modules/include/mysql_client_server_protocol.h index ffde09871..ed820fded 100644 --- a/server/modules/include/mysql_client_server_protocol.h +++ b/server/modules/include/mysql_client_server_protocol.h @@ -289,6 +289,7 @@ typedef struct { #define MYSQL_GET_STMTOK_NPARAM(payload) (gw_mysql_get_byte2(&payload[9])) #define MYSQL_GET_STMTOK_NATTR(payload) (gw_mysql_get_byte2(&payload[11])) #define MYSQL_IS_ERROR_PACKET(payload) (MYSQL_GET_COMMAND(payload)==0xff) +#define MYSQL_IS_COM_QUIT(payload) (MYSQL_GET_COMMAND(payload)==0x01) #define MYSQL_GET_NATTR(payload) ((int)payload[4]) #endif /** _MYSQL_PROTOCOL_H */ diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 255b205c1..bcddc18ef 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -430,7 +430,6 @@ static int gw_read_backend_event(DCB *dcb) { CHK_SESSION(session); router = session->service->router; router_instance = session->service->router_instance; - rsession = session->router_session; /* read available backend data */ rc = dcb_read(dcb, &read_buffer); @@ -450,7 +449,7 @@ static int gw_read_backend_event(DCB *dcb) { "Read from backend failed"); router->handleError(router_instance, - rsession, + session->router_session, errbuf, dcb, ERRACT_NEW_CONNECTION, @@ -525,9 +524,10 @@ static int gw_read_backend_event(DCB *dcb) { MYSQL_IDLE) { gwbuf_set_type(read_buffer, GWBUF_TYPE_MYSQL); + router->clientReply( router_instance, - rsession, + session->router_session, read_buffer, dcb); rc = 1; @@ -537,7 +537,7 @@ static int gw_read_backend_event(DCB *dcb) { else if (dcb->session->client->dcb_role == DCB_ROLE_INTERNAL) { gwbuf_set_type(read_buffer, GWBUF_TYPE_MYSQL); - router->clientReply(router_instance, rsession, read_buffer, dcb); + router->clientReply(router_instance, session->router_session, read_buffer, dcb); rc = 1; } } @@ -565,32 +565,42 @@ static int gw_write_backend_event(DCB *dcb) { * Don't write to backend if backend_dcb is not in poll set anymore. */ if (dcb->state != DCB_STATE_POLLING) { - if (dcb->writeq != NULL) { - /*< vraa : errorHandle */ - mysql_send_custom_error( - dcb->session->client, - 1, - 0, - "Writing to backend failed due invalid Maxscale " - "state."); - LOGIF(LD, (skygw_log_write( - LOGFILE_DEBUG, - "%lu [gw_write_backend_event] Write to backend " - "dcb %p fd %d " - "failed due invalid state %s.", - pthread_self(), - dcb, - dcb->fd, - STRDCBSTATE(dcb->state)))); + uint8_t* data; - LOGIF(LE, (skygw_log_write_flush( - LOGFILE_ERROR, - "Error : Attempt to write buffered data to backend " - "failed " - "due internal inconsistent state."))); + if (dcb->writeq != NULL) + { + data = (uint8_t *)GWBUF_DATA(dcb->writeq); - rc = 0; - } else { + if (!(MYSQL_IS_COM_QUIT(data))) + { + /*< vraa : errorHandle */ + mysql_send_custom_error( + dcb->session->client, + 1, + 0, + "Writing to backend failed due invalid Maxscale " + "state."); + LOGIF(LD, (skygw_log_write( + LOGFILE_DEBUG, + "%lu [gw_write_backend_event] Write to backend " + "dcb %p fd %d " + "failed due invalid state %s.", + pthread_self(), + dcb, + dcb->fd, + STRDCBSTATE(dcb->state)))); + + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Error : Attempt to write buffered data to backend " + "failed " + "due internal inconsistent state."))); + + rc = 0; + } + } + else + { LOGIF(LD, (skygw_log_write( LOGFILE_DEBUG, "%lu [gw_write_backend_event] Dcb %p in state %s " @@ -914,6 +924,7 @@ gw_backend_hangup(DCB *dcb) CHK_DCB(dcb); session = dcb->session; CHK_SESSION(session); + rsession = session->router_session; router = session->service->router; router_instance = session->service->router_instance; @@ -971,6 +982,7 @@ gw_backend_close(DCB *dcb) CHK_SESSION(session); quitbuf = mysql_create_com_quit(NULL, 0); + gwbuf_set_type(quitbuf, GWBUF_TYPE_MYSQL); /** Send COM_QUIT to the backend being closed */ mysql_send_com_quit(dcb, 0, quitbuf); diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 5c557c5a7..db0ff81a0 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -1528,7 +1528,8 @@ static void clientReply ( */ if (sescmd_cursor_is_active(scur)) { - if (MYSQL_IS_ERROR_PACKET(((uint8_t *)GWBUF_DATA(writebuf)))) + if (LOG_IS_ENABLED(LOGFILE_ERROR) && + MYSQL_IS_ERROR_PACKET(((uint8_t *)GWBUF_DATA(writebuf)))) { SESSION* ses = backend_dcb->session; uint8_t* buf = @@ -1546,14 +1547,9 @@ static void clientReply ( bref->bref_backend->backend_server->port))); free(cmdstr); - /** Inform the client */ - handle_error_reply_client(ses, writebuf); - - /** Unlock router session */ - rses_end_locked_router_action(router_cli_ses); - goto lock_failed; } - else if (GWBUF_IS_TYPE_SESCMD_RESPONSE(writebuf)) + + if (GWBUF_IS_TYPE_SESCMD_RESPONSE(writebuf)) { /** * Discard all those responses that have already been sent to @@ -1691,12 +1687,17 @@ static void bref_clear_state( /** Decrease waiter count */ prev1 = atomic_add(&bref->bref_num_result_wait, -1); - ss_dassert(prev1 > 0); - /** Decrease global operation count */ - prev2 = atomic_add( - &bref->bref_backend->backend_server->stats.n_current_ops, -1); - ss_dassert(prev2 > 0); + if (prev1 <= 0) { + atomic_add(&bref->bref_num_result_wait, 1); + } + else + { + /** Decrease global operation count */ + prev2 = atomic_add( + &bref->bref_backend->backend_server->stats.n_current_ops, -1); + ss_dassert(prev2 > 0); + } } } @@ -3145,12 +3146,15 @@ static void handleError ( backend_dcb->dcb_errhandle_called = true; #endif session = backend_dcb->session; - CHK_SESSION(session); + + if (session != NULL) + CHK_SESSION(session); switch (action) { case ERRACT_NEW_CONNECTION: { - CHK_CLIENT_RSES(rses); + if (rses != NULL) + CHK_CLIENT_RSES(rses); if (!rses_begin_locked_router_action(rses)) {