From f0d8ed0cf21b4ae4ac0ad859500e449b9f8162da Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Tue, 23 Dec 2014 00:26:57 +0200 Subject: [PATCH] Fix to #657, http://bugs.mariadb.com/show_bug.cgi?id=657 session.c:session_free:if session is child of another service (tee in this case), it is the parent which releases child's allocated memory back to the system. This now also includes the child router session. dcb.h: Added DCB_IS_CLONE macro tee.c:freeSession:if parent session triggered closing of tee, then child session may not be closed yet. In that case free the child session first and only then free child router session and release child session's memory back to system. tee.c:routeQuery: only route if child session is ready for routing. Log if session is not ready for routing and set tee session inactive mysql_client.c:gw_client_close:if DCB is cloned one don't close the protocol because they it is shared with the original DCB. --- server/core/dcb.c | 9 +- server/core/session.c | 7 +- server/include/dcb.h | 3 +- server/modules/filter/tee.c | 112 ++++++++++++------ server/modules/monitor/mysql_mon.c | 6 +- server/modules/protocol/mysql_client.c | 11 +- .../routing/readwritesplit/readwritesplit.c | 7 -- 7 files changed, 98 insertions(+), 57 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index fca1c73e1..dc37add6c 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -310,7 +310,7 @@ DCB *clone; return NULL; } - clone->fd = DCBFD_CLONED;; + clone->fd = DCBFD_CLOSED; clone->flags |= DCBF_CLONE; clone->state = orig->state; clone->data = orig->data; @@ -321,11 +321,10 @@ DCB *clone; clone->protocol = orig->protocol; clone->func.write = dcb_null_write; -#if 1 + /** + * Close triggers closing of router session as well which is needed. + */ clone->func.close = orig->func.close; -#else - clone->func.close = dcb_null_close; -#endif clone->func.auth = dcb_null_auth; return clone; diff --git a/server/core/session.c b/server/core/session.c index 2638483eb..07291073f 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -357,7 +357,6 @@ bool session_free( int i; CHK_SESSION(session); - /*< * Remove one reference. If there are no references left, * free session. @@ -389,14 +388,14 @@ bool session_free( atomic_add(&session->service->stats.n_current, -1); /** - * Free router_session and set it NULL + * If session is not child of some other session, free router_session. + * Otherwise let the parent free it. */ - if (session->router_session) + if (!session->ses_is_child && session->router_session) { session->service->router->freeSession( session->service->router_instance, session->router_session); - session->router_session = NULL; } if (session->n_filters) { diff --git a/server/include/dcb.h b/server/include/dcb.h index c0bb80d73..57e204462 100644 --- a/server/include/dcb.h +++ b/server/include/dcb.h @@ -130,7 +130,6 @@ typedef struct { #define GWPROTOCOL_VERSION {1, 0, 0} #define DCBFD_CLOSED -1 -#define DCBFD_CLONED -2 /** * The statitics gathered on a descriptor control block @@ -332,4 +331,6 @@ bool dcb_get_ses_log_info(DCB* dcb, size_t* sesid, int* enabled_logs); */ #define DCBF_CLONE 0x0001 /*< DCB is a clone */ #define DCBF_HUNG 0x0002 /*< Hangup has been dispatched */ + +#define DCB_IS_CLONE(d) ((d)->flags & DCBF_CLONE) #endif /* _DCB_H */ diff --git a/server/modules/filter/tee.c b/server/modules/filter/tee.c index f3c78a71f..d18f4a103 100644 --- a/server/modules/filter/tee.c +++ b/server/modules/filter/tee.c @@ -351,7 +351,7 @@ char *remote, *userName; if ((dcb = dcb_clone(session->client)) == NULL) { - freeSession(my_instance, (void *)my_session); + freeSession(instance, (void *)my_session); my_session = NULL; LOGIF(LE, (skygw_log_write_flush( @@ -364,7 +364,7 @@ char *remote, *userName; if ((ses = session_alloc(my_instance->service, dcb)) == NULL) { dcb_close(dcb); - freeSession(my_instance, (void *)my_session); + freeSession(instance, (void *)my_session); my_session = NULL; LOGIF(LE, (skygw_log_write_flush( LOGFILE_ERROR, @@ -438,12 +438,26 @@ freeSession(FILTER *instance, void *session) TEE_SESSION *my_session = (TEE_SESSION *)session; SESSION* ses = my_session->branch_session; - if (ses != NULL && ses->state == SESSION_STATE_TO_BE_FREED) + if (ses != NULL) { - ses->state = SESSION_STATE_FREE; - free(ses); + if (ses->state == SESSION_STATE_ROUTER_READY) + { + session_free(ses); + } + + if (ses->state == SESSION_STATE_TO_BE_FREED) + { + /** Free branch router session */ + ses->service->router->freeSession( + ses->service->router_instance, + ses->router_session); + /** Free memory of branch client session */ + ses->state = SESSION_STATE_FREE; + free(ses); + /** This indicates that branch session is not available anymore */ + my_session->branch_session = NULL; + } } - free(session); return; } @@ -490,46 +504,76 @@ char *ptr; int length, rval, residual = 0; GWBUF *clone = NULL; - if (my_session->residual) + if (my_session->branch_session->state == SESSION_STATE_ROUTER_READY) { - clone = gwbuf_clone(queue); - if (my_session->residual < GWBUF_LENGTH(clone)) - GWBUF_RTRIM(clone, GWBUF_LENGTH(clone) - residual); - my_session->residual -= GWBUF_LENGTH(clone); - if (my_session->residual < 0) - my_session->residual = 0; - } - else if ( my_session->active && (ptr = modutil_get_SQL(queue)) != NULL) - { - if ((my_instance->match == NULL || - regexec(&my_instance->re, ptr, 0, NULL, 0) == 0) && - (my_instance->nomatch == NULL || - regexec(&my_instance->nore,ptr,0,NULL, 0) != 0)) - { - char *dummy; - - modutil_MySQL_Query(queue, &dummy, &length, &residual); - clone = gwbuf_clone(queue); - my_session->residual = residual; + if (my_session->residual) + { + clone = gwbuf_clone(queue); + + if (my_session->residual < GWBUF_LENGTH(clone)) + { + GWBUF_RTRIM(clone, GWBUF_LENGTH(clone) - residual); + } + my_session->residual -= GWBUF_LENGTH(clone); + + if (my_session->residual < 0) + { + my_session->residual = 0; + } } - free(ptr); - } - else if (packet_is_required(queue)) - { - clone = gwbuf_clone(queue); - } + else if (my_session->active && (ptr = modutil_get_SQL(queue)) != NULL) + { + if ((my_instance->match == NULL || + regexec(&my_instance->re, ptr, 0, NULL, 0) == 0) && + (my_instance->nomatch == NULL || + regexec(&my_instance->nore,ptr,0,NULL, 0) != 0)) + { + char *dummy; + modutil_MySQL_Query(queue, &dummy, &length, &residual); + clone = gwbuf_clone(queue); + my_session->residual = residual; + + } + free(ptr); + } + else if (packet_is_required(queue)) + { + clone = gwbuf_clone(queue); + } + } /* Pass the query downstream */ rval = my_session->down.routeQuery(my_session->down.instance, - my_session->down.session, queue); + my_session->down.session, + queue); if (clone) { my_session->n_duped++; - SESSION_ROUTE_QUERY(my_session->branch_session, clone); + + if (my_session->branch_session->state == SESSION_STATE_ROUTER_READY) + { + SESSION_ROUTE_QUERY(my_session->branch_session, clone); + } + else + { + /** Close tee session */ + my_session->active = 0; + LOGIF(LT, (skygw_log_write( + LOGFILE_TRACE, + "Closed tee filter session."))); + gwbuf_free(clone); + } } else { + if (my_session->active) + { + LOGIF(LT, (skygw_log_write( + LOGFILE_TRACE, + "Closed tee filter session."))); + my_session->active = 0; + } my_session->n_rejected++; } return rval; diff --git a/server/modules/monitor/mysql_mon.c b/server/modules/monitor/mysql_mon.c index fadb7a23c..e09b1e2f4 100644 --- a/server/modules/monitor/mysql_mon.c +++ b/server/modules/monitor/mysql_mon.c @@ -790,14 +790,14 @@ int log_no_master = 1; { LOGIF(LM, (skygw_log_write( LOGFILE_MESSAGE, - "Info: A Master Server is now available: %s:%i", + "Info : A Master Server is now available: %s:%i", root_master->server->name, root_master->server->port))); } } else { LOGIF(LE, (skygw_log_write_flush( LOGFILE_ERROR, - "Error: No Master can be determined. Last known was %s:%i", + "Error : No Master can be determined. Last known was %s:%i", root_master->server->name, root_master->server->port))); } @@ -807,7 +807,7 @@ int log_no_master = 1; { LOGIF(LE, (skygw_log_write_flush( LOGFILE_ERROR, - "Error: No Master can be determined"))); + "Error : No Master can be determined"))); log_no_master = 0; } } diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index fbddb6275..7ab43b6ce 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -1401,9 +1401,14 @@ gw_client_close(DCB *dcb) LOGIF(LD, (skygw_log_write(LOGFILE_DEBUG, "%lu [gw_client_close]", pthread_self()))); - - mysql_protocol_done(dcb); - + /** + * Since only protocol pointer is copied from original DCB to clone in + * dcb_clone, only dcb_close for the original DCB closes protocol. + */ + if (!DCB_IS_CLONE(dcb)) + { + mysql_protocol_done(dcb); + } session = dcb->session; /** * session may be NULL if session_alloc failed. diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index c5b9912f6..fc4b16646 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -1032,13 +1032,6 @@ static void freeSession( router = (ROUTER_INSTANCE *)router_instance; backend_ref = router_cli_ses->rses_backend_ref; - for (i=0; irses_nbackends; i++) - { - if (!BREF_IS_IN_USE((&backend_ref[i]))) - { - continue; - } - } spinlock_acquire(&router->lock); if (router->connections == router_cli_ses) {