hcq/MaxScale
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Add check so DCB can't be added to poll list without a linked session; small safety improvement in session; tidying.

Browse Source
This commit is contained in:
Martin Brampton
2015-08-23 09:34:26 +01:00
parent 53c3cc4b41
commit 6b2d90fc50
2 changed files with 157 additions and 134 deletions
Download Patch File Download Diff File Expand all files Collapse all files

151
server/core/poll.c
View File

@ -74,6 +74,8 @@ int max_poll_sleep;
* thread utilisation and fairer scheduling of the event * thread utilisation and fairer scheduling of the event
* processing. * processing.
* 07/07/15 Martin Brampton Simplified add and remove DCB, improve error handling. * 07/07/15 Martin Brampton Simplified add and remove DCB, improve error handling.
* 23/08/15 Martin Brampton Provisionally added test so only DCB with a
* session link can be added to the poll list
* *
* @endverbatim * @endverbatim
*/ */
@ -254,12 +256,12 @@ int i;
int int
poll_add_dcb(DCB *dcb) poll_add_dcb(DCB *dcb)
{ {
int rc = -1; int rc = -1;
dcb_state_t old_state = dcb->state; dcb_state_t old_state = dcb->state;
dcb_state_t new_state; dcb_state_t new_state;
struct epoll_event ev; struct epoll_event ev;
CHK_DCB(dcb); CHK_DCB(dcb);
#ifdef EPOLLRDHUP #ifdef EPOLLRDHUP
ev.events = EPOLLIN | EPOLLOUT | EPOLLRDHUP | EPOLLHUP | EPOLLET; ev.events = EPOLLIN | EPOLLOUT | EPOLLRDHUP | EPOLLHUP | EPOLLET;
@ -268,65 +270,86 @@ poll_add_dcb(DCB *dcb)
#endif #endif
ev.data.ptr = dcb; ev.data.ptr = dcb;
/*< /*<
* Choose new state according to the role of dcb. * Choose new state according to the role of dcb.
*/ */
spinlock_acquire(&dcb->dcb_initlock); spinlock_acquire(&dcb->dcb_initlock);
if (dcb->dcb_role == DCB_ROLE_REQUEST_HANDLER) { if (dcb->dcb_role == DCB_ROLE_REQUEST_HANDLER)
new_state = DCB_STATE_POLLING; {
} else { new_state = DCB_STATE_POLLING;
ss_dassert(dcb->dcb_role == DCB_ROLE_SERVICE_LISTENER); }
new_state = DCB_STATE_LISTENING; else
} {
/* ss_dassert(dcb->dcb_role == DCB_ROLE_SERVICE_LISTENER);
* Check DCB current state seems sensible new_state = DCB_STATE_LISTENING;
*/ }
if (DCB_STATE_DISCONNECTED == dcb->state /*
|| DCB_STATE_ZOMBIE == dcb->state * Check DCB current state seems sensible
|| DCB_STATE_UNDEFINED == dcb->state) */
{ if (DCB_STATE_DISCONNECTED == dcb->state
LOGIF(LE, (skygw_log_write_flush( || DCB_STATE_ZOMBIE == dcb->state
LOGFILE_ERROR, || DCB_STATE_UNDEFINED == dcb->state)
"%lu [poll_add_dcb] Error : existing state of dcb %p " {
"is %s, but this should be impossible, crashing.", LOGIF(LE, (skygw_log_write_flush(
pthread_self(), LOGFILE_ERROR,
dcb, "%lu [poll_add_dcb] Error : existing state of dcb %p "
STRDCBSTATE(dcb->state)))); "is %s, but this should be impossible, crashing.",
raise(SIGABRT); pthread_self(),
} dcb,
if (DCB_STATE_POLLING == dcb->state STRDCBSTATE(dcb->state))));
|| DCB_STATE_LISTENING == dcb->state) raise(SIGABRT);
{ }
LOGIF(LE, (skygw_log_write_flush( /*
LOGFILE_ERROR, * This test could be wrong. On the face of it, we don't want to add a
"%lu [poll_add_dcb] Error : existing state of dcb %p " * DCB to the poll list if it is not linked to a session because the code
"is %s, but this is probably an error, not crashing.", * that handles events will expect to find a session. Test added by
pthread_self(), * Martin as an experiment on 23 August 2015
dcb, */
STRDCBSTATE(dcb->state)))); if (NULL == dcb->session)
} {
dcb->state = new_state; LOGIF(LE, (skygw_log_write_flush(
spinlock_release(&dcb->dcb_initlock); LOGFILE_ERROR,
/* "%lu [%s] Error : Attempt to add dcb %p "
* The only possible failure that will not cause a crash is "to poll list but it is not linked to a session, crashing.",
* running out of system resources. __func__,
*/ pthread_self(),
rc = epoll_ctl(epoll_fd, EPOLL_CTL_ADD, dcb->fd, &ev); dcb)));
if (rc) raise(SIGABRT);
{ }
rc = poll_resolve_error(dcb, errno, true); if (DCB_STATE_POLLING == dcb->state
} || DCB_STATE_LISTENING == dcb->state)
if (0 == rc) {
{ LOGIF(LE, (skygw_log_write_flush(
LOGIF(LD, (skygw_log_write( LOGFILE_ERROR,
LOGFILE_DEBUG, "%lu [poll_add_dcb] Error : existing state of dcb %p "
"%lu [poll_add_dcb] Added dcb %p in state %s to poll set.", "is %s, but this is probably an error, not crashing.",
pthread_self(), pthread_self(),
dcb, dcb,
STRDCBSTATE(dcb->state)))); STRDCBSTATE(dcb->state))));
} }
else dcb->state = old_state; dcb->state = new_state;
return rc; spinlock_release(&dcb->dcb_initlock);
/*
* The only possible failure that will not cause a crash is
* running out of system resources.
*/
rc = epoll_ctl(epoll_fd, EPOLL_CTL_ADD, dcb->fd, &ev);
if (rc)
{
/* Some errors are actually considered acceptable */
rc = poll_resolve_error(dcb, errno, true);
}
if (0 == rc)
{
LOGIF(LD, (skygw_log_write(
LOGFILE_DEBUG,
"%lu [poll_add_dcb] Added dcb %p in state %s to poll set.",
pthread_self(),
dcb,
STRDCBSTATE(dcb->state))));
}
else dcb->state = old_state;
return rc;
} }
/** /**

140
server/core/session.c
View File

@ -26,6 +26,7 @@
* 17/06/13 Mark Riddoch Initial implementation * 17/06/13 Mark Riddoch Initial implementation
* 02/09/13 Massimiliano Pinto Added session refcounter * 02/09/13 Massimiliano Pinto Added session refcounter
* 29/05/14 Mark Riddoch Addition of filter mechanism * 29/05/14 Mark Riddoch Addition of filter mechanism
* 23/08/15 Martin Brampton Tidying; slight improvement in safety
* *
* @endverbatim * @endverbatim
*/ */
@ -72,65 +73,66 @@ static int session_setup_filters(SESSION *session);
SESSION * SESSION *
session_alloc(SERVICE *service, DCB *client_dcb) session_alloc(SERVICE *service, DCB *client_dcb)
{ {
SESSION *session; SESSION *session;
session = (SESSION *)calloc(1, sizeof(SESSION)); session = (SESSION *)calloc(1, sizeof(SESSION));
ss_info_dassert(session != NULL, ss_info_dassert(session != NULL,
"Allocating memory for session failed."); "Allocating memory for session failed.");
if (session == NULL) if (session == NULL)
{ {
LOGIF(LE, (skygw_log_write_flush( LOGIF(LE, (skygw_log_write_flush(
LOGFILE_ERROR, LOGFILE_ERROR,
"Error : Failed to allocate memory for " "Error : Failed to allocate memory for "
"session object due error %d, %s.", "session object due error %d, %s.",
errno, errno,
strerror(errno)))); strerror(errno))));
if (client_dcb->data && !DCB_IS_CLONE(client_dcb)) if (client_dcb->data && !DCB_IS_CLONE(client_dcb))
{ {
free(client_dcb->data); void *clientdata = client_dcb->data;
client_dcb->data = NULL; client_dcb->data = NULL;
} free(clientdata);
goto return_session;
} }
goto return_session;
}
#if defined(SS_DEBUG) #if defined(SS_DEBUG)
session->ses_chk_top = CHK_NUM_SESSION; session->ses_chk_top = CHK_NUM_SESSION;
session->ses_chk_tail = CHK_NUM_SESSION; session->ses_chk_tail = CHK_NUM_SESSION;
#endif #endif
if (DCB_IS_CLONE(client_dcb)) if (DCB_IS_CLONE(client_dcb))
{ {
session->ses_is_child = true; session->ses_is_child = true;
} }
spinlock_init(&session->ses_lock); spinlock_init(&session->ses_lock);
/*< /*<
* Prevent backend threads from accessing before session is completely * Prevent backend threads from accessing before session is completely
* initialized. * initialized.
*/ */
spinlock_acquire(&session->ses_lock); spinlock_acquire(&session->ses_lock);
session->service = service; session->service = service;
session->client = client_dcb; session->client = client_dcb;
session->n_filters = 0; session->n_filters = 0;
memset(&session->stats, 0, sizeof(SESSION_STATS)); memset(&session->stats, 0, sizeof(SESSION_STATS));
session->stats.connect = time(0); session->stats.connect = time(0);
session->state = SESSION_STATE_ALLOC; session->state = SESSION_STATE_ALLOC;
/*< /*<
* Associate the session to the client DCB and set the reference count on * Associate the session to the client DCB and set the reference count on
* the session to indicate that there is a single reference to the * the session to indicate that there is a single reference to the
* session. There is no need to protect this or use atomic add as the * session. There is no need to protect this or use atomic add as the
* session has not been made available to the other threads at this * session has not been made available to the other threads at this
* point. * point.
*/ */
session->data = client_dcb->data; session->data = client_dcb->data;
client_dcb->session = session; client_dcb->session = session;
session->refcount = 1; session->refcount = 1;
/*< /*<
* This indicates that session is ready to be shared with backend * This indicates that session is ready to be shared with backend
* DCBs. Note that this doesn't mean that router is initialized yet! * DCBs. Note that this doesn't mean that router is initialized yet!
*/ */
session->state = SESSION_STATE_READY; session->state = SESSION_STATE_READY;
/*< Release session lock */ /*< Release session lock */
spinlock_release(&session->ses_lock); spinlock_release(&session->ses_lock);
/* /*
* Only create a router session if we are not the listening * Only create a router session if we are not the listening
@ -140,36 +142,34 @@ session_alloc(SERVICE *service, DCB *client_dcb)
* *
* Router session creation may create other DCBs that link to the * Router session creation may create other DCBs that link to the
* session, therefore it is important that the session lock is * session, therefore it is important that the session lock is
* relinquished beforethe router call. * relinquished before the router call.
*/ */
if (client_dcb->state != DCB_STATE_LISTENING && if (client_dcb->state != DCB_STATE_LISTENING &&
client_dcb->dcb_role != DCB_ROLE_INTERNAL) client_dcb->dcb_role != DCB_ROLE_INTERNAL)
{ {
session->router_session = session->router_session =
service->router->newSession(service->router_instance, service->router->newSession(service->router_instance, session);
session); if (session->router_session == NULL)
if (session->router_session == NULL)
{ {
/** /**
* Inform other threads that session is closing. * Inform other threads that session is closing.
*/ */
session->state = SESSION_STATE_STOPPING; session->state = SESSION_STATE_STOPPING;
/*< /*<
* Decrease refcount, set dcb's session pointer NULL * Decrease refcount, set dcb's session pointer NULL
* and set session pointer to NULL. * and set session pointer to NULL.
*/ */
session->client = NULL; session->client = NULL;
session_free(session); session_free(session);
client_dcb->session = NULL; client_dcb->session = NULL;
session = NULL; session = NULL;
LOGIF(LE, (skygw_log_write_flush( LOGIF(LE, (skygw_log_write_flush(
LOGFILE_ERROR, LOGFILE_ERROR,
"Error : Failed to create %s session.", "Error : Failed to create %s session.",
service->name))); service->name)));
goto return_session; goto return_session;
} }
/* /*
* Pending filter chain being setup set the head of the chain to * Pending filter chain being setup set the head of the chain to
* be the router. As filters are inserted the current head will * be the router. As filters are inserted the current head will