From 3b07449daa9be28377e8ecc716efc360cdbdef42 Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Mon, 10 Nov 2014 14:07:51 +0200 Subject: [PATCH] Fix to bug #614, http://bugs.skysql.com/show_bug.cgi?id=614 Added protected state check to mysql_client.c, fixed locking in session.c --- server/core/session.c | 20 ++++++++++------- server/modules/protocol/mysql_backend.c | 13 ++++++----- server/modules/protocol/mysql_client.c | 29 +++++++++++++++---------- 3 files changed, 38 insertions(+), 24 deletions(-) diff --git a/server/core/session.c b/server/core/session.c index d83881532..5795cd878 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -133,8 +133,9 @@ session_alloc(SERVICE *service, DCB *client_dcb) session->router_session = service->router->newSession(service->router_instance, session); - - if (session->router_session == NULL) { + + if (session->router_session == NULL) + { /** * Inform other threads that session is closing. */ @@ -153,7 +154,6 @@ session_alloc(SERVICE *service, DCB *client_dcb) goto return_session; } - /* * Pending filter chain being setup set the head of the chain to * be the router. As filters are inserted the current head will @@ -196,11 +196,12 @@ session_alloc(SERVICE *service, DCB *client_dcb) } } - spinlock_acquire(&session_spin); - + spinlock_acquire(&session->ses_lock); + if (session->state != SESSION_STATE_READY) { - session_free(session); + spinlock_release(&session->ses_lock); + session_free(session); client_dcb->session = NULL; session = NULL; LOGIF(LE, (skygw_log_write_flush( @@ -212,10 +213,13 @@ session_alloc(SERVICE *service, DCB *client_dcb) else { session->state = SESSION_STATE_ROUTER_READY; - session->next = allSessions; + spinlock_release(&session->ses_lock); + spinlock_acquire(&session_spin); + session->next = allSessions; allSessions = session; spinlock_release(&session_spin); - atomic_add(&service->stats.n_sessions, 1); + + atomic_add(&service->stats.n_sessions, 1); atomic_add(&service->stats.n_current, 1); CHK_SESSION(session); } diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index b61b20c48..2ec3712c5 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -378,7 +378,6 @@ static int gw_read_backend_event(DCB *dcb) { ERRACT_REPLY_CLIENT, &succp); gwbuf_free(errbuf); - ss_dassert(!succp); LOGIF(LD, (skygw_log_write( LOGFILE_DEBUG, "%lu [gw_read_backend_event] " @@ -854,8 +853,12 @@ static int gw_error_backend_event(DCB *dcb) &succp); gwbuf_free(errbuf); - /** There are not required backends available, close session. */ - if (!succp) { + /** + * If error handler fails it means that routing session can't continue + * and it must be closed. In success, only this DCB is closed. + */ + if (!succp) + { spinlock_acquire(&session->ses_lock); session->state = SESSION_STATE_STOPPING; spinlock_release(&session->ses_lock); @@ -1082,8 +1085,8 @@ gw_backend_close(DCB *dcb) mysql_protocol_done(dcb); /** - * If session->state is set to STOPPING the client and the session must - * be closed too. + * If session->state is STOPPING, start closing client session. + * Otherwise only this backend connection is closed. */ if (session != NULL && session->state == SESSION_STATE_STOPPING) { diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index d25039f4c..31e8f8cb3 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -830,7 +830,7 @@ int gw_read_client_event( } /** succeed */ - if (rc) + if (rc) { rc = 0; /**< here '0' means success */ } @@ -850,7 +850,6 @@ int gw_read_client_event( "Error : Routing the query failed. " "Session will be closed."))); - dcb_close(dcb); } } @@ -1363,7 +1362,6 @@ gw_client_close(DCB *dcb) SESSION* session; ROUTER_OBJECT* router; void* router_instance; - void* rsession; #if defined(SS_DEBUG) MySQLProtocol* protocol = (MySQLProtocol *)dcb->protocol; if (dcb->state == DCB_STATE_POLLING || @@ -1380,7 +1378,7 @@ gw_client_close(DCB *dcb) * session may be NULL if session_alloc failed. * In that case, router session wasn't created. */ - if (session != NULL) + if (session != NULL) { CHK_SESSION(session); spinlock_acquire(&session->ses_lock); @@ -1389,13 +1387,22 @@ gw_client_close(DCB *dcb) { session->state = SESSION_STATE_STOPPING; } - spinlock_release(&session->ses_lock); - - router = session->service->router; - router_instance = session->service->router_instance; - rsession = session->router_session; - /** Close router session and all its connections */ - router->closeSession(router_instance, rsession); + router_instance = session->service->router_instance; + router = session->service->router; + /** + * If router session is being created concurrently router + * session might be NULL and it shouldn't be closed. + */ + if (session->router_session != NULL) + { + spinlock_release(&session->ses_lock); + /** Close router session and all its connections */ + router->closeSession(router_instance, session->router_session); + } + else + { + spinlock_release(&session->ses_lock); + } } return 1; }