From c2310327fc895aefe4e320af9ba7939dc1334598 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Tue, 5 Jan 2016 06:31:07 +0200 Subject: [PATCH] Fixed idle session processing The current implementation of idle connection timeouts is not safe. The sessions are handled in a way which is not thread-safe and the checking is done from a non-polling thread. With this change, the checks for the session timeouts are done in one of the polling threads in a thread-safe manner only if at least one service has enabled the timing out of idle client connections. --- server/core/housekeeper.c | 2 +- server/core/poll.c | 6 ++++ server/core/service.c | 20 ++++++----- server/core/session.c | 62 ++++++++++++++++++++++++----------- server/include/hk_heartbeat.h | 6 ++-- server/include/service.h | 6 +++- server/include/session.h | 11 +++++-- 7 files changed, 77 insertions(+), 36 deletions(-) diff --git a/server/core/housekeeper.c b/server/core/housekeeper.c index 9349590ec..98c4f2344 100644 --- a/server/core/housekeeper.c +++ b/server/core/housekeeper.c @@ -53,7 +53,7 @@ static HKTASK *tasks = NULL; static SPINLOCK tasklock = SPINLOCK_INIT; static int do_shutdown = 0; -unsigned long hkheartbeat = 0; +long hkheartbeat = 0; /*< One heartbeat is 100 milliseconds */ static void hkthread(void *); diff --git a/server/core/poll.c b/server/core/poll.c index defaa9a82..f00e120de 100644 --- a/server/core/poll.c +++ b/server/core/poll.c @@ -35,6 +35,7 @@ #include #include #include +#include #define PROFILE_POLL 0 @@ -705,6 +706,11 @@ poll_waitevents(void *arg) timeout_bias = 1; } + if (check_timeouts && hkheartbeat >= next_timeout_check) + { + process_idle_sessions(); + } + if (thread_data) { thread_data[thread_id].state = THREAD_ZPROCESSING; diff --git a/server/core/service.c b/server/core/service.c index 90d23761a..b0241191b 100644 --- a/server/core/service.c +++ b/server/core/service.c @@ -98,7 +98,7 @@ static int find_type(typelib_t* tl, const char* needle, int maxlen); static void service_add_qualified_param(SERVICE* svc, CONFIG_PARAMETER* param); -void service_interal_restart(void *data); +static void service_internal_restart(void *data); /** * Allocate a new service for the gateway to support @@ -142,6 +142,7 @@ service_alloc(const char *servname, const char *router) service->resources = NULL; service->localhost_match_wildcard_host = SERVICE_PARAM_UNINIT; service->retry_start = true; + service->conn_idle_timeout = SERVICE_NO_SESSION_TIMEOUT; service->weightby = NULL; service->credentials.authdata = NULL; service->credentials.name = NULL; @@ -431,11 +432,6 @@ int serviceStartAllPorts(SERVICE* service) { service->state = SERVICE_STATE_STARTED; service->stats.started = time(0); - /** Add the task that monitors session timeouts */ - if (service->conn_timeout > 0) - { - hktask_add("connection_timeout", session_close_timeouts, NULL, 5); - } } else if (service->retry_start) { @@ -445,7 +441,7 @@ int serviceStartAllPorts(SERVICE* service) int retry_after = MIN(service->stats.n_failed_starts * 10, SERVICE_MAX_RETRY_INTERVAL); snprintf(taskname, sizeof (taskname), "%s_start_retry_%d", service->name, service->stats.n_failed_starts); - hktask_oneshot(taskname, service_interal_restart, + hktask_oneshot(taskname, service_internal_restart, (void*) service, retry_after); MXS_NOTICE("Failed to start service %s, retrying in %d seconds.", service->name, retry_after); @@ -1117,7 +1113,13 @@ serviceSetTimeout(SERVICE *service, int val) { return 0; } - service->conn_timeout = val; + + /** Enable the session timeout checks if and only if at least one service is + * configured with a idle timeout. */ + if ((service->conn_idle_timeout = val)) + { + enable_session_timeouts(); + } return 1; } @@ -2241,7 +2243,7 @@ int serviceInitSSL(SERVICE* service) * Function called by the housekeeper thread to retry starting of a service * @param data Service to restart */ -void service_interal_restart(void *data) +static void service_internal_restart(void *data) { SERVICE* service = (SERVICE*)data; serviceStartAllPorts(service); diff --git a/server/core/session.c b/server/core/session.c index a2f687a38..285abc4d0 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -54,6 +54,14 @@ static SESSION *allSessions = NULL; static struct session session_dummy_struct; +/** + * These two are declared in session.h + */ +bool check_timeouts = false; +long next_timeout_check = 0; + +static SPINLOCK timeout_lock = SPINLOCK_INIT; + static int session_setup_filters(SESSION *session); static void session_simple_free(SESSION *session, DCB *dcb); @@ -961,34 +969,48 @@ SESSION *get_all_sessions() return allSessions; } +/** + * Enable the timing out of idle connections. + * + * This will prevent unnecessary acquisitions of the session spinlock if no + * service is configured with a session idle timeout. + */ +void enable_session_timeouts() +{ + check_timeouts = true; +} + /** * Close sessions that have been idle for too long. * - * If the time since a session last sent data is grater than the set value in the - * service, it is disconnected. The default value for the timeout for a service is 0. - * This means that connections are never timed out. - * @param data NULL, this is only here to satisfy the housekeeper function requirements. + * If the time since a session last sent data is greater than the set value in the + * service, it is disconnected. The connection timeout is disabled by default. */ -void session_close_timeouts(void* data) +void process_idle_sessions() { - SESSION* ses; - - spinlock_acquire(&session_spin); - ses = get_all_sessions(); - spinlock_release(&session_spin); - - while (ses) + if (spinlock_acquire_nowait(&timeout_lock)) { - if (ses->client && ses->client->state == DCB_STATE_POLLING && - ses->service->conn_timeout > 0 && - hkheartbeat - ses->client->last_read > ses->service->conn_timeout * 10) + if (hkheartbeat >= next_timeout_check) { - dcb_close(ses->client); - } + /** Because the resolution of the timeout is one second, we only need to + * check for it once per second. One heartbeat is 100 milliseconds. */ + next_timeout_check = hkheartbeat + 10; + spinlock_acquire(&session_spin); + SESSION *ses = get_all_sessions(); - spinlock_acquire(&session_spin); - ses = ses->next; - spinlock_release(&session_spin); + while (ses) + { + if (ses->service && ses->client && ses->client->state == DCB_STATE_POLLING && + hkheartbeat - ses->client->last_read > ses->service->conn_idle_timeout * 10) + { + dcb_close(ses->client); + } + + ses = ses->next; + } + spinlock_release(&session_spin); + } + spinlock_release(&timeout_lock); } } diff --git a/server/include/hk_heartbeat.h b/server/include/hk_heartbeat.h index f1ea1e0bc..3f46f7a26 100644 --- a/server/include/hk_heartbeat.h +++ b/server/include/hk_heartbeat.h @@ -2,10 +2,10 @@ #define _HK_HEARTBEAT_H /** - * The global housekeeper heartbeat value. This value is increamente - * every 100ms and may be used for crude timing etc. + * The global housekeeper heartbeat value. This value is incremented + * every 100 milliseconds and may be used for crude timing etc. */ -extern unsigned long hkheartbeat; +extern long hkheartbeat; #endif diff --git a/server/include/service.h b/server/include/service.h index dd96819dc..db9d76ad0 100644 --- a/server/include/service.h +++ b/server/include/service.h @@ -135,6 +135,10 @@ enum #define DEFAULT_SSL_CERT_VERIFY_DEPTH 100 /*< The default certificate verification depth */ #define SERVICE_MAX_RETRY_INTERVAL 3600 /*< The maximum interval between service start retries */ + +/** Value of service timeout if timeout checks are disabled */ +#define SERVICE_NO_SESSION_TIMEOUT LONG_MAX + /** * Parameters that are automatically detected but can also be configured by the * user are initially set to this value. @@ -180,7 +184,7 @@ typedef struct service SERVICE_REFRESH_RATE rate_limit; /**< The refresh rate limit for users table */ FILTER_DEF **filters; /**< Ordered list of filters */ int n_filters; /**< Number of filters */ - int conn_timeout; /*< Session timeout in seconds */ + long conn_idle_timeout; /**< Session timeout in seconds */ ssl_mode_t ssl_mode; /*< one of DISABLED, ENABLED or REQUIRED */ char *weightby; struct service *next; /**< The next service in the linked list */ diff --git a/server/include/session.h b/server/include/session.h index 45bb91723..76d9e718f 100644 --- a/server/include/session.h +++ b/server/include/session.h @@ -146,6 +146,13 @@ typedef struct session #endif } SESSION; +/** Whether to do session timeout checks */ +extern bool check_timeouts; + +/** When the next timeout check is done. This is compared to hkheartbeat in + * hk_heartbeat.h */ +extern long next_timeout_check; + #define SESSION_PROTOCOL(x, type) DCB_PROTOCOL((x)->client, type) /** @@ -184,7 +191,7 @@ int session_unlink_dcb(SESSION*, DCB*); SESSION* get_session_by_router_ses(void* rses); void session_enable_log_priority(SESSION* ses, int priority); void session_disable_log_priority(SESSION* ses, int priority); -void session_close_timeouts(void* data); RESULTSET *sessionGetList(SESSIONLISTFILTER); - +void process_idle_sessions(); +void enable_session_timeouts(); #endif