MXS-1929: Store service active state atomically

When a session is closed, it releases a reference on the service and
checks if it was the last session for a destroyed service. The state of
the service was loaded after the reference count was decremented.  This
behavior introduced a race condition where it was possible for a service
to be freed twice, first by the thread that marked the service as
destroyed and again by the last session for that service. By always
loading the service state before decrementing the reference count, we
avoid this race condition.

Currently, the memory ordering used for the reference counting is too
strict and could be relaxed. By default, all atomic operations use
sequentially consistent memory ordering. This guarantees correct behavior
but imposes a performance penalty. Incrementing the reference counts could
be done with a relaxed memory order as long as as we know the reference
we're incrementing is valid. Releasing a reference must use an
acquire-release order to guarantee the read-modify-write operation is
successful.
This commit is contained in:
Markus Mäkelä
2018-07-25 09:02:11 +03:00
parent b9500e6329
commit efd953c434
3 changed files with 6 additions and 4 deletions

View File

@ -157,7 +157,7 @@ typedef struct service
uint64_t capabilities; /**< The capabilities of the service, @see enum routing_capability */
int max_retry_interval; /**< Maximum retry interval */
bool session_track_trx_state; /**< Get transaction state via session track mechanism */
bool active; /**< Whether the service is still active */
int active; /**< Whether the service is still active */
} SERVICE;
typedef enum count_spec_t

View File

@ -26,6 +26,7 @@
#include <sys/types.h>
#include <math.h>
#include <fcntl.h>
#include <atomic>
#include <map>
#include <string>
#include <set>
@ -246,7 +247,7 @@ void service_destroy(SERVICE* service)
#endif
ss_dassert(service->active);
service->active = false;
atomic_store_int(&service->active, false);
char filename[PATH_MAX + 1];
snprintf(filename, sizeof(filename), "%s/%s.cnf", get_config_persistdir(),
@ -1297,7 +1298,7 @@ service_find(const char *servname)
service = allServices;
while (service)
{
if (strcmp(service->name, servname) == 0 && service->active)
if (strcmp(service->name, servname) == 0 && atomic_load_int(&service->active))
{
break;
}

View File

@ -413,8 +413,9 @@ static void session_free(MXS_SESSION *session)
session->state = SESSION_STATE_FREE;
session_final_free(session);
bool should_destroy = !atomic_load_int(&service->active);
if (atomic_add(&service->client_count, -1) == 1 && !service->active)
if (atomic_add(&service->client_count, -1) && should_destroy)
{
// Destroy the service in the main routing worker thread
mxs::RoutingWorker* main_worker = mxs::RoutingWorker::get(mxs::RoutingWorker::MAIN);