Cache: Make config and storage factory sharable

In antecipation of a CachePT (cache per thread), rearrange things
so that the storage factory and config can be shared between cache
instances.

server/modules/filter/cache/cache.cc

@@ -20,15 +20,14 @@
 #include "storage.h"
 
 Cache::Cache(const char* zName,
-             CACHE_CONFIG& config,
+             const CACHE_CONFIG* pConfig,
              CACHE_RULES* pRules,
              StorageFactory* pFactory)
     : m_zName(zName)
-    , m_config(config)
+    , m_config(*pConfig)
     , m_pRules(pRules)
     , m_pFactory(pFactory)
 {
-    cache_config_reset(config);
 }
 
 Cache::~Cache()
@@ -39,11 +38,9 @@ Cache::~Cache()
 
 //static
 bool Cache::Create(const CACHE_CONFIG& config,
-                   CACHE_RULES**       ppRules,
+                   CACHE_RULES**       ppRules)
-                   StorageFactory**    ppFactory)
 {
     CACHE_RULES* pRules = NULL;
-    StorageFactory* pFactory = NULL;
 
     if (config.rules)
     {
@@ -55,29 +52,42 @@ bool Cache::Create(const CACHE_CONFIG& config,
     }
 
     if (pRules)
-    {
-        pFactory = StorageFactory::Open(config.storage);
-
-        if (!pFactory)
-        {
-            MXS_ERROR("Could not open storage factory '%s'.", config.storage);
-        }
-    }
-
-    bool rv = (pRules && pFactory);
-
-    if (rv)
     {
         *ppRules = pRules;
-        *ppFactory = pFactory;
     }
     else
     {
-        cache_rules_free(pRules);
+        MXS_ERROR("Could not create rules.");
-        delete pFactory;
     }
 
-    return rv;
+    return pRules != NULL;
+}
+
+//static
+bool Cache::Create(const CACHE_CONFIG& config,
+                   CACHE_RULES**       ppRules,
+                   StorageFactory**    ppFactory)
+{
+    CACHE_RULES* pRules = NULL;
+    StorageFactory* pFactory = NULL;
+
+    if (Create(config, &pRules))
+    {
+        pFactory = StorageFactory::Open(config.storage);
+
+        if (pFactory)
+        {
+            *ppFactory = pFactory;
+            *ppRules = pRules;
+        }
+        else
+        {
+            MXS_ERROR("Could not open storage factory '%s'.", config.storage);
+            cache_rules_free(pRules);
+        }
+    }
+
+    return pFactory != NULL;
 }
 
 bool Cache::shouldStore(const char* zDefaultDb, const GWBUF* pQuery)

server/modules/filter/cache/cache.h

@@ -74,10 +74,13 @@ public:
 
 protected:
     Cache(const char* zName,
-          CACHE_CONFIG& config,
+          const CACHE_CONFIG* pConfig,
           CACHE_RULES* pRules,
           StorageFactory* pFactory);
 
+    static bool Create(const CACHE_CONFIG& config,
+                       CACHE_RULES**       ppRules);
+
     static bool Create(const CACHE_CONFIG& config,
                        CACHE_RULES**       ppRules,
                        StorageFactory**    ppFactory);
@@ -88,7 +91,7 @@ private:
 
 protected:
     const char*         m_zName;    // The name of the instance; the section name in the config.
-    CACHE_CONFIG    m_config;   // The configuration of the cache instance.
+    const CACHE_CONFIG& m_config;   // The configuration of the cache instance.
     CACHE_RULES*        m_pRules;   // The rules of the cache instance.
     StorageFactory*     m_pFactory; // The storage factory.
 };

server/modules/filter/cache/cachefilter.cc

@@ -14,6 +14,7 @@
 #define MXS_MODULE_NAME "cache"
 #include "cachefilter.h"
 #include <exception>
+#include <new>
 #include <maxscale/alloc.h>
 #include <maxscale/filter.h>
 #include <maxscale/gwdirs.h>
@@ -35,6 +36,28 @@ static const CACHE_CONFIG DEFAULT_CONFIG =
     CACHE_DEFAULT_DEBUG
 };
 
+typedef struct cache_filter
+{
+    cache_filter()
+        : config(DEFAULT_CONFIG)
+        , pCache(NULL)
+    {
+    }
+
+    ~cache_filter()
+    {
+        delete pCache;
+        cache_config_finish(config);
+    }
+
+    CACHE_CONFIG config;
+    Cache*       pCache;
+
+private:
+    cache_filter(const cache_filter&);
+    cache_filter& operator = (const cache_filter&);
+} CACHE_FILTER;
+
 static FILTER*  createInstance(const char* zName, char** pzOptions, FILTER_PARAMETER** ppParams);
 static void*    newSession(FILTER* pInstance, SESSION* pSession);
 static void     closeSession(FILTER* pInstance, void* pSessionData);
@@ -114,20 +137,24 @@ extern "C" FILTER_OBJECT *GetModuleObject()
  */
 static FILTER *createInstance(const char* zName, char** pzOptions, FILTER_PARAMETER** ppParams)
 {
-    Cache* pCache = NULL;
+    CACHE_FILTER* pFilter = new (std::nothrow) CACHE_FILTER;
-    CACHE_CONFIG config = DEFAULT_CONFIG;
 
-    if (process_params(pzOptions, ppParams, config))
+    if (pFilter)
     {
-        CPP_GUARD(pCache = CacheMT::Create(zName, config));
+        if (process_params(pzOptions, ppParams, pFilter->config))
+        {
+            CPP_GUARD(pFilter->pCache = CacheMT::Create(zName, &pFilter->config));
 
-        if (!pCache)
+            if (!pFilter->pCache)
             {
-            cache_config_finish(config);
+                cache_config_finish(pFilter->config);
+                delete pFilter;
+                pFilter = NULL;
+            }
         }
     }
 
-    return reinterpret_cast<FILTER*>(pCache);
+    return reinterpret_cast<FILTER*>(pFilter);
 }
 
 /**
@@ -140,7 +167,8 @@ static FILTER *createInstance(const char* zName, char** pzOptions, FILTER_PARAME
  */
 static void *newSession(FILTER* pInstance, SESSION* pSession)
 {
-    Cache* pCache = reinterpret_cast<Cache*>(pInstance);
+    CACHE_FILTER *pFilter = reinterpret_cast<CACHE_FILTER*>(pInstance);
+    Cache* pCache = pFilter->pCache;
 
     SessionCache* pSessionCache = NULL;
     CPP_GUARD(pSessionCache = SessionCache::Create(pCache, pSession));
@@ -447,11 +475,7 @@ void cache_config_finish(CACHE_CONFIG& config)
     MXS_FREE(config.rules);
     MXS_FREE(config.storage);
     MXS_FREE(config.storage_options);
-
+    MXS_FREE(config.storage_argv); // The items need not be freed, they point into storage_options.
-    for (int i = 0; i < config.storage_argc; ++i)
-    {
-        MXS_FREE(config.storage_argv[i]);
-    }
 
     config.max_resultset_rows = 0;
     config.max_resultset_size = 0;

server/modules/filter/cache/cachemt.cc

@@ -16,12 +16,12 @@
 #include "storagefactory.h"
 
 CacheMT::CacheMT(const char* zName,
-                 CACHE_CONFIG& config,
+                 const CACHE_CONFIG* pConfig,
                  CACHE_RULES* pRules,
                  StorageFactory* pFactory,
-                 Storage* pStorage,
+                 HASHTABLE* pPending,
-                 HASHTABLE* pPending)
+                 Storage* pStorage)
-    : CacheSimple(zName, config, pRules, pFactory, pStorage, pPending)
+    : CacheSimple(zName, pConfig, pRules, pFactory, pPending, pStorage)
 {
     spinlock_init(&m_lockPending);
 }
@@ -30,39 +30,38 @@ CacheMT::~CacheMT()
 {
 }
 
-CacheMT* CacheMT::Create(const char* zName, CACHE_CONFIG& config)
+CacheMT* CacheMT::Create(const char* zName, const CACHE_CONFIG* pConfig)
 {
+    ss_dassert(pConfig);
+
     CacheMT* pCache = NULL;
 
     CACHE_RULES* pRules = NULL;
     HASHTABLE* pPending = NULL;
     StorageFactory* pFactory = NULL;
 
-    if (CacheSimple::Create(config, &pRules, &pFactory, &pPending))
+    if (CacheSimple::Create(*pConfig, &pRules, &pPending, &pFactory))
     {
-        uint32_t ttl = config.ttl;
+        pCache = Create(zName, pConfig, pRules, pFactory, pPending);
-        int argc = config.storage_argc;
-        char** argv = config.storage_argv;
-
-        Storage* pStorage = pFactory->createStorage(CACHE_THREAD_MODEL_MT, zName, ttl, argc, argv);
-
-        if (pStorage)
-        {
-            CPP_GUARD(pCache = new CacheMT(zName,
-                                           config,
-                                           pRules,
-                                           pFactory,
-                                           pStorage,
-                                           pPending));
-
-            if (!pCache)
-            {
-                cache_rules_free(pRules);
-                hashtable_free(pPending);
-                delete pStorage;
-                delete pFactory;
     }
+
+    return pCache;
 }
+
+// static
+CacheMT* CacheMT::Create(const char* zName, StorageFactory* pFactory, const CACHE_CONFIG* pConfig)
+{
+    ss_dassert(pConfig);
+    ss_dassert(pFactory);
+
+    CacheMT* pCache = NULL;
+
+    CACHE_RULES* pRules = NULL;
+    HASHTABLE* pPending = NULL;
+
+    if (CacheSimple::Create(*pConfig, &pRules, &pPending))
+    {
+        pCache = Create(zName, pConfig, pRules, pFactory, pPending);
     }
 
     return pCache;
@@ -87,3 +86,39 @@ void CacheMT::refreshed(const CACHE_KEY& key,  const SessionCache* pSessionCache
     CacheSimple::refreshed(k, pSessionCache);
     spinlock_release(&m_lockPending);
 }
+
+// static
+CacheMT* CacheMT::Create(const char*         zName,
+                         const CACHE_CONFIG* pConfig,
+                         CACHE_RULES*        pRules,
+                         StorageFactory*     pFactory,
+                         HASHTABLE*          pPending)
+{
+    CacheMT* pCache = NULL;
+
+    uint32_t ttl = pConfig->ttl;
+    int argc = pConfig->storage_argc;
+    char** argv = pConfig->storage_argv;
+
+    Storage* pStorage = pFactory->createStorage(CACHE_THREAD_MODEL_MT, zName, ttl, argc, argv);
+
+    if (pStorage)
+    {
+        CPP_GUARD(pCache = new CacheMT(zName,
+                                       pConfig,
+                                       pRules,
+                                       pFactory,
+                                       pPending,
+                                       pStorage));
+
+        if (!pCache)
+        {
+            delete pStorage;
+            cache_rules_free(pRules);
+            hashtable_free(pPending);
+            delete pFactory;
+        }
+    }
+
+    return pCache;
+}

server/modules/filter/cache/cachemt.h

@@ -21,7 +21,8 @@ class CacheMT : public CacheSimple
 public:
     ~CacheMT();
 
-    static CacheMT* Create(const char* zName, CACHE_CONFIG& config);
+    static CacheMT* Create(const char* zName, const CACHE_CONFIG* pConfig);
+    static CacheMT* Create(const char* zName, StorageFactory* pFactory, const CACHE_CONFIG* pConfig);
 
     bool mustRefresh(const CACHE_KEY& key, const SessionCache* pSessionCache);
 
@@ -29,10 +30,16 @@ public:
 
 private:
     CacheMT(const char*         zName,
-            CACHE_CONFIG& config,
+            const CACHE_CONFIG* pConfig,
+            CACHE_RULES*        pRules,
+            StorageFactory*     pFactory,
+            HASHTABLE*          pPending,
+            Storage*            pStorage);
+
+    static CacheMT* Create(const char*         zName,
+                           const CACHE_CONFIG* pConfig,
                            CACHE_RULES*        pRules,
                            StorageFactory*     pFactory,
-            Storage* pStorage,
                            HASHTABLE*          pPending);
 
 private:

server/modules/filter/cache/cachesimple.cc

@@ -61,14 +61,14 @@ int hashcmp(const void* address1, const void* address2)
 
 
 CacheSimple::CacheSimple(const char*         zName,
-                         CACHE_CONFIG& config,
+                         const CACHE_CONFIG* pConfig,
                          CACHE_RULES*        pRules,
                          StorageFactory*     pFactory,
-                         Storage* pStorage,
+                         HASHTABLE*          pPending,
-                         HASHTABLE* pPending)
+                         Storage*            pStorage)
-    : Cache(zName, config, pRules, pFactory)
+    : Cache(zName, pConfig, pRules, pFactory)
-    , m_pStorage(pStorage)
     , m_pPending(pPending)
+    , m_pStorage(pStorage)
 {
 }
 
@@ -79,23 +79,42 @@ CacheSimple::~CacheSimple()
 }
 
 
-// static protected
+// static
 bool CacheSimple::Create(const CACHE_CONFIG& config,
                          CACHE_RULES**       ppRules,
-                         StorageFactory**    ppFactory,
                          HASHTABLE**         ppPending)
 {
     int rv = false;
 
+    CACHE_RULES* pRules = NULL;
+    HASHTABLE* pPending = NULL;
+
+    if (Cache::Create(config, &pRules) && Create(&pPending))
+    {
+        *ppRules = pRules;
+        *ppPending = pPending;
+    }
+    else
+    {
+        cache_rules_free(pRules);
+    }
+
+    return pPending != NULL;;
+}
+
+// static
+bool CacheSimple::Create(const CACHE_CONFIG& config,
+                         CACHE_RULES**       ppRules,
+                         HASHTABLE**         ppPending,
+                         StorageFactory**    ppFactory)
+{
+    int rv = false;
+
     CACHE_RULES* pRules = NULL;
     StorageFactory* pFactory = NULL;
     HASHTABLE* pPending = NULL;
 
-    if (Cache::Create(config, &pRules, &pFactory))
+    if (Cache::Create(config, &pRules, &pFactory) && Create(&pPending))
-    {
-        pPending = hashtable_alloc(CACHE_PENDING_ITEMS, hashfn, hashcmp);
-
-        if (pPending)
     {
         *ppRules = pRules;
         *ppPending = pPending;
@@ -106,7 +125,6 @@ bool CacheSimple::Create(const CACHE_CONFIG& config,
         cache_rules_free(pRules);
         delete pFactory;
     }
-    }
 
     return pPending != NULL;
 }
@@ -162,3 +180,16 @@ void CacheSimple::refreshed(long key, const SessionCache* pSessionCache)
     ss_debug(int n =) hashtable_delete(m_pPending, (void*)key);
     ss_dassert(n == 1);
 }
+
+// static
+bool CacheSimple::Create(HASHTABLE** ppPending)
+{
+    HASHTABLE* pPending = hashtable_alloc(CACHE_PENDING_ITEMS, hashfn, hashcmp);
+
+    if (pPending)
+    {
+        *ppPending = pPending;
+    }
+
+    return pPending != NULL;
+}

server/modules/filter/cache/cachesimple.h

@@ -33,17 +33,22 @@ public:
 
 protected:
     CacheSimple(const char*         zName,
-                CACHE_CONFIG& config,
+                const CACHE_CONFIG* pConfig,
                 CACHE_RULES*        pRules,
                 StorageFactory*     pFactory,
-                Storage* pStorage,
+                HASHTABLE*          pPending,
-                HASHTABLE* pPending);
+                Storage*            pStorage);
 
     static bool Create(const CACHE_CONFIG& config,
                        CACHE_RULES**       ppRules,
-                       StorageFactory**    ppFactory,
                        HASHTABLE**         ppPending);
 
+    static bool Create(const CACHE_CONFIG& config,
+                       CACHE_RULES**       ppRules,
+                       HASHTABLE**         ppPending,
+                       StorageFactory**    ppFactory);
+
+
     long hashOfKey(const CACHE_KEY& key);
 
     bool mustRefresh(long key, const SessionCache* pSessionCache);
@@ -54,7 +59,9 @@ private:
     CacheSimple(const Cache&);
     CacheSimple& operator = (const CacheSimple&);
 
+    static bool Create(HASHTABLE** ppPending);
+
 protected:
-    Storage*   m_pStorage;  // The storage instance to use.
     HASHTABLE* m_pPending;  // Pending items; being fetched from the backend.
+    Storage*   m_pStorage;  // The storage instance to use.
 };

server/modules/filter/cache/cachest.cc

@@ -16,12 +16,12 @@
 #include "storagefactory.h"
 
 CacheST::CacheST(const char* zName,
-                 CACHE_CONFIG& config,
+                 const CACHE_CONFIG* pConfig,
                  CACHE_RULES* pRules,
                  StorageFactory* pFactory,
-                 Storage* pStorage,
+                 HASHTABLE* pPending,
-                 HASHTABLE* pPending)
+                 Storage* pStorage)
-    : CacheSimple(zName, config, pRules, pFactory, pStorage, pPending)
+    : CacheSimple(zName, pConfig, pRules, pFactory, pPending, pStorage)
 {
 }
 
@@ -29,39 +29,38 @@ CacheST::~CacheST()
 {
 }
 
-CacheST* CacheST::Create(const char* zName, CACHE_CONFIG& config)
+CacheST* CacheST::Create(const char* zName, const CACHE_CONFIG* pConfig)
 {
+    ss_dassert(pConfig);
+
     CacheST* pCache = NULL;
 
     CACHE_RULES* pRules = NULL;
     HASHTABLE* pPending = NULL;
     StorageFactory* pFactory = NULL;
 
-    if (CacheSimple::Create(config, &pRules, &pFactory, &pPending))
+    if (CacheSimple::Create(*pConfig, &pRules, &pPending, &pFactory))
     {
-        uint32_t ttl = config.ttl;
+        pCache = Create(zName, pConfig, pRules, pFactory, pPending);
-        int argc = config.storage_argc;
-        char** argv = config.storage_argv;
-
-        Storage* pStorage = pFactory->createStorage(CACHE_THREAD_MODEL_ST, zName, ttl, argc, argv);
-
-        if (pStorage)
-        {
-            CPP_GUARD(pCache = new CacheST(zName,
-                                           config,
-                                           pRules,
-                                           pFactory,
-                                           pStorage,
-                                           pPending));
-
-            if (!pCache)
-            {
-                cache_rules_free(pRules);
-                hashtable_free(pPending);
-                delete pStorage;
-                delete pFactory;
     }
+
+    return pCache;
 }
+
+// static
+CacheST* CacheST::Create(const char* zName, StorageFactory* pFactory, const CACHE_CONFIG* pConfig)
+{
+    ss_dassert(pConfig);
+    ss_dassert(pFactory);
+
+    CacheST* pCache = NULL;
+
+    CACHE_RULES* pRules = NULL;
+    HASHTABLE* pPending = NULL;
+
+    if (CacheSimple::Create(*pConfig, &pRules, &pPending))
+    {
+        pCache = Create(zName, pConfig, pRules, pFactory, pPending);
     }
 
     return pCache;
@@ -80,3 +79,39 @@ void CacheST::refreshed(const CACHE_KEY& key,  const SessionCache* pSessionCache
 
     CacheSimple::refreshed(k, pSessionCache);
 }
+
+// statis
+CacheST* CacheST::Create(const char*         zName,
+                         const CACHE_CONFIG* pConfig,
+                         CACHE_RULES*        pRules,
+                         StorageFactory*     pFactory,
+                         HASHTABLE*          pPending)
+{
+    CacheST* pCache = NULL;
+
+    uint32_t ttl = pConfig->ttl;
+    int argc = pConfig->storage_argc;
+    char** argv = pConfig->storage_argv;
+
+    Storage* pStorage = pFactory->createStorage(CACHE_THREAD_MODEL_ST, zName, ttl, argc, argv);
+
+    if (pStorage)
+    {
+        CPP_GUARD(pCache = new CacheST(zName,
+                                       pConfig,
+                                       pRules,
+                                       pFactory,
+                                       pPending,
+                                       pStorage));
+
+        if (!pCache)
+        {
+            delete pStorage;
+            cache_rules_free(pRules);
+            hashtable_free(pPending);
+            delete pFactory;
+        }
+    }
+
+    return pCache;
+}

server/modules/filter/cache/cachest.h

@@ -20,7 +20,8 @@ class CacheST : public CacheSimple
 public:
     ~CacheST();
 
-    static CacheST* Create(const char* zName, CACHE_CONFIG& config);
+    static CacheST* Create(const char* zName, const CACHE_CONFIG* pConfig);
+    static CacheST* Create(const char* zName, StorageFactory* pFactory, const CACHE_CONFIG* pConfig);
 
     bool mustRefresh(const CACHE_KEY& key, const SessionCache* pSessionCache);
 
@@ -28,12 +29,17 @@ public:
 
 private:
     CacheST(const char*         zName,
-            CACHE_CONFIG& config,
+            const CACHE_CONFIG* pConfig,
             CACHE_RULES*        pRules,
             StorageFactory*     pFactory,
-            Storage* pStorage,
+            HASHTABLE*          pPending,
-            HASHTABLE* pPending);
+            Storage*            pStorage);
 
+    static CacheST* Create(const char*         zName,
+                           const CACHE_CONFIG* pConfig,
+                           CACHE_RULES*        pRules,
+                           StorageFactory*     pFactory,
+                           HASHTABLE*          pPending);
 private:
     CacheST(const CacheST&);
     CacheST& operator = (const CacheST&);