From ad5762a2fbb39f5a3e1694a4a457720d377dffc3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= <markus.makela@mariadb.com>
Date: Mon, 30 Jul 2018 02:10:42 +0300
Subject: [PATCH] Fix debug assertion on shutdown

The debug assertion that asserts that services are destroyed only on the
main worker would be triggered on shutdown as there is no current worker
at that point in time. In addition to this, it is wrong to call
service_destroy at shutdown as that will remove persisted
configurations. The service_free function can be called directly as we
know no other thread are running when the services are being torn down.

Also added the missing check that the destroyInstance function is
implemented before calling it.
---
 server/core/gateway.cc          |  2 ++
 server/core/internal/maxscale.h |  4 ++++
 server/core/misc.cc             | 12 ++++++++++++
 server/core/service.cc          | 20 ++++++++++----------
 4 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/server/core/gateway.cc b/server/core/gateway.cc
index c7f186d48..f8f984e95 100644
--- a/server/core/gateway.cc
+++ b/server/core/gateway.cc
@@ -2205,6 +2205,8 @@ int main(int argc, char **argv)
 
     MXS_NOTICE("All workers have shut down.");
 
+    maxscale_start_teardown();
+
     /*<
      * Destroy the router and filter instances of all services.
      */
diff --git a/server/core/internal/maxscale.h b/server/core/internal/maxscale.h
index 128b7f93c..6a895e0af 100644
--- a/server/core/internal/maxscale.h
+++ b/server/core/internal/maxscale.h
@@ -35,4 +35,8 @@ int maxscale_shutdown(void);
  */
 void maxscale_reset_starttime(void);
 
+// Helper functions for debug assertions
+bool maxscale_teardown_in_progress();
+void maxscale_start_teardown();
+
 MXS_END_DECLS
diff --git a/server/core/misc.cc b/server/core/misc.cc
index a03052d2d..97c284de0 100644
--- a/server/core/misc.cc
+++ b/server/core/misc.cc
@@ -50,3 +50,15 @@ int maxscale_shutdown()
 
     return n + 1;
 }
+
+static bool teardown_in_progress = false;
+
+bool maxscale_teardown_in_progress()
+{
+    return teardown_in_progress;
+}
+
+void maxscale_start_teardown()
+{
+    teardown_in_progress = true;
+}
diff --git a/server/core/service.cc b/server/core/service.cc
index 1d003ac14..011366cde 100644
--- a/server/core/service.cc
+++ b/server/core/service.cc
@@ -60,6 +60,7 @@
 #include "internal/modules.h"
 #include "internal/service.hh"
 #include "internal/routingworker.hh"
+#include "internal/maxscale.h"
 
 /** This define is needed in CentOS 6 systems */
 #if !defined(UINT64_MAX)
@@ -197,7 +198,7 @@ Service* service_alloc(const char *name, const char *router, MXS_CONFIG_PARAMETE
 void service_free(Service* service)
 {
     ss_dassert(atomic_load_int(&service->client_count) == 0);
-    ss_dassert(!service->active);
+    ss_dassert(!service->active || maxscale_teardown_in_progress());
 
     spinlock_acquire(&service_spin);
     auto it = std::remove(allServices.begin(), allServices.end(), service);
@@ -208,11 +209,11 @@ void service_free(Service* service)
     {
         auto tmp = service->ports;
         service->ports = service->ports->next;
-        ss_dassert(!tmp->active);
+        ss_dassert(!tmp->active || maxscale_teardown_in_progress());
         listener_free(tmp);
     }
 
-    if (service->router && service->router_instance)
+    if (service->router && service->router_instance && service->router->destroyInstance)
     {
         service->router->destroyInstance(service->router_instance);
     }
@@ -220,7 +221,7 @@ void service_free(Service* service)
     while (service->dbref)
     {
         SERVER_REF* tmp = service->dbref;
-        ss_dassert(!tmp->active);
+        ss_dassert(!tmp->active || maxscale_teardown_in_progress());
         service->dbref = service->dbref->next;
         MXS_FREE(tmp);
     }
@@ -242,7 +243,7 @@ void service_destroy(Service* service)
 #endif
 
     ss_dassert(service->active);
-   atomic_store_int(&service->active, false);
+    atomic_store_int(&service->active, false);
 
     char filename[PATH_MAX + 1];
     snprintf(filename, sizeof(filename), "%s/%s.cnf", get_config_persistdir(),
@@ -1711,14 +1712,13 @@ void service_shutdown()
 
 void service_destroy_instances(void)
 {
-    spinlock_acquire(&service_spin);
+    // The global list is modified by service_free so we need a copy of it
+    std::vector<Service*> my_services = allServices;
 
-    for (Service* s: allServices)
+    for (Service* s: my_services)
     {
-        service_destroy(s);
+        service_free(s);
     }
-
-    spinlock_release(&service_spin);
 }
 
 /**