From 6306519e5e75575ba083ee2f0edfe7e624da5d26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 2 Jan 2020 13:50:23 +0200 Subject: [PATCH] MXS-2710: Move client_count handling inside Session By incrementing the counters when the session is created, we know that the counter will always be decremented correctly. This does cause the listener session to be counted as an actual session but this is already present in the statistics calculations and is something we have to live with in 2.3 This change also makes it possible to overshoot the connection count limitation as the session creation is delayed until authentication fails. Both of these problems are fixed in 2.4. --- server/core/dcb.cc | 5 ---- server/core/session.cc | 23 ++++++++++--------- server/modules/filter/test/mock_session.cc | 3 +++ .../MySQL/mariadbclient/mysql_client.cc | 14 +++-------- 4 files changed, 18 insertions(+), 27 deletions(-) diff --git a/server/core/dcb.cc b/server/core/dcb.cc index f466d2135..de0572e6d 100644 --- a/server/core/dcb.cc +++ b/server/core/dcb.cc @@ -2533,11 +2533,6 @@ DCB* dcb_accept(DCB* dcb) } } - if (client_dcb) - { - mxb::atomic::add(&client_dcb->service->client_count, 1); - } - return client_dcb; } diff --git a/server/core/session.cc b/server/core/session.cc index 70da75a36..16b7c0654 100644 --- a/server/core/session.cc +++ b/server/core/session.cc @@ -361,18 +361,7 @@ private: static void session_free(MXS_SESSION* session) { MXS_INFO("Stopped %s client session [%" PRIu64 "]", session->service->name, session->ses_id); - Service* service = static_cast(session->service); - session_final_free(session); - bool should_destroy = !mxb::atomic::load(&service->active); - - if (mxb::atomic::add(&service->client_count, -1) == 1 && should_destroy) - { - // Destroy the service in the main routing worker thread - mxs::RoutingWorker* main_worker = mxs::RoutingWorker::get(mxs::RoutingWorker::MAIN); - main_worker->execute(std::unique_ptr(new ServiceDestroyTask(service)), - Worker::EXECUTE_AUTO); - } } static void session_final_free(MXS_SESSION* ses) @@ -1264,6 +1253,7 @@ Session::Session(SERVICE* service) mxb::atomic::add(&service->stats.n_current, 1, mxb::atomic::RELAXED); mxb_assert(service->stats.n_current >= 0); + mxb::atomic::add(&service->client_count, 1, mxb::atomic::RELAXED); } Session::~Session() @@ -1281,6 +1271,17 @@ Session::~Session() mxb::atomic::add(&service->stats.n_current, -1, mxb::atomic::RELAXED); mxb_assert(service->stats.n_current >= 0); + + bool should_destroy = !mxb::atomic::load(&service->active); + + if (mxb::atomic::add(&service->client_count, -1) == 1 && should_destroy) + { + // Destroy the service in the main routing worker thread + mxs::RoutingWorker* main_worker = mxs::RoutingWorker::get(mxs::RoutingWorker::MAIN); + main_worker->execute( + std::unique_ptr(new ServiceDestroyTask(static_cast(service))), + Worker::EXECUTE_AUTO); + } } namespace diff --git a/server/modules/filter/test/mock_session.cc b/server/modules/filter/test/mock_session.cc index aaa3d233a..c4b445795 100644 --- a/server/modules/filter/test/mock_session.cc +++ b/server/modules/filter/test/mock_session.cc @@ -44,6 +44,9 @@ Session::Session(Client* pClient) m_client_dcb.data = &m_mysql_session; service = &dummy_service; + + // This prevents the destruction of the dummy service + service->active = true; } Session::~Session() diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index 638a235ca..9bf24c96c 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -740,20 +740,12 @@ static void check_packet(DCB* dcb, GWBUF* buf, int bytes) */ static int gw_read_do_authentication(DCB* dcb, GWBUF* read_buffer, int nbytes_read) { - // If this function fails and dcb is closed, the service client count must be decremented manually. - // After authentication, the session closing code decrements the count. - auto decrement_and_close = [](DCB* dcb) - { - mxb::atomic::add(&dcb->service->client_count, -1); - dcb_close(dcb); - }; - MXB_AT_DEBUG(check_packet(dcb, read_buffer, nbytes_read)); /** Allocate the shared session structure */ if (dcb->data == NULL && (dcb->data = mysql_session_alloc()) == NULL) { - decrement_and_close(dcb); + dcb_close(dcb); return 1; } @@ -820,7 +812,7 @@ static int gw_read_do_authentication(DCB* dcb, GWBUF* read_buffer, int nbytes_re MYSQL_session* ses = (MYSQL_session*)dcb->data; if ((dcb->user = MXS_STRDUP(ses->user)) == NULL) { - decrement_and_close(dcb); + dcb_close(dcb); gwbuf_free(read_buffer); return 0; } @@ -873,7 +865,7 @@ static int gw_read_do_authentication(DCB* dcb, GWBUF* read_buffer, int nbytes_re /** * Close DCB and which will release MYSQL_session */ - decrement_and_close(dcb); + dcb_close(dcb); } /* One way or another, the buffer is now fully processed */ gwbuf_free(read_buffer);