Re: Changing the order of cleanup for some core objects

[Bojan Smojver]

On Tue, 2008-07-22 at 07:29 +1000, Bojan Smojver wrote:

> Or you can malloc() the resource container in the constructor (i.e. rec
> in dbd_construct()) and then free() it in destructor.

Something like this. Completely untested, of course :-)

-- 
Bojan

Index: modules/database/mod_dbd.c
===================================================================
--- modules/database/mod_dbd.c	(revision 678578)
+++ modules/database/mod_dbd.c	(working copy)
@@ -66,11 +66,9 @@
 struct dbd_group_t {
     dbd_cfg_t *cfg;
     dbd_group_t *next;
-    apr_pool_t *pool;
 #if APR_HAS_THREADS
     apr_thread_mutex_t *mutex;
     apr_reslist_t *reslist;
-    int destroyed;
 #else
     ap_dbd_t *rec;
 #endif
@@ -429,20 +427,21 @@
 {
     ap_dbd_t *rec = data;
 
+    rec->pool = NULL; /* so that destructor doesn't destroy the pool again */
     return apr_dbd_close(rec->driver, rec->handle);
 }
 
 #if APR_HAS_THREADS
 static apr_status_t dbd_destruct(void *data, void *params, apr_pool_t *pool)
 {
-    dbd_group_t *group = params;
+    ap_dbd_t *rec = data;
 
-    if (!group->destroyed) {
-        ap_dbd_t *rec = data;
-
+    if (rec->pool) {
         apr_pool_destroy(rec->pool);
     }
 
+    free(rec); /* free non-pool space */
+
     return APR_SUCCESS;
 }
 #endif
@@ -467,7 +466,7 @@
         return rv;
     }
 
-    rec = apr_pcalloc(rec_pool, sizeof(ap_dbd_t));
+    rec = calloc(1, sizeof(ap_dbd_t)); /* non-pool space */
 
     rec->pool = rec_pool;
 
@@ -551,52 +550,22 @@
 }
 
 #if APR_HAS_THREADS
-static apr_status_t dbd_destroy(void *data)
+static apr_status_t dbd_setup(server_rec *s, dbd_group_t *group,
+                              apr_pool_t *pool)
 {
-    dbd_group_t *group = data;
-
-    group->destroyed = 1;
-
-    return APR_SUCCESS;
-}
-
-static apr_status_t dbd_setup(server_rec *s, dbd_group_t *group)
-{
     dbd_cfg_t *cfg = group->cfg;
     apr_status_t rv;
 
-    /* We create the reslist using a sub-pool of the pool passed to our
-     * child_init hook.  No other threads can be here because we're
-     * either in the child_init phase or dbd_setup_lock() acquired our mutex.
-     * No other threads will use this sub-pool after this, except via
-     * reslist calls, which have an internal mutex.
-     *
-     * We need to short-circuit the cleanup registered internally by
-     * apr_reslist_create().  We do this by registering dbd_destroy()
-     * as a cleanup afterwards, so that it will run before the reslist's
-     * internal cleanup.
-     *
-     * If we didn't do this, then we could free memory twice when the pool
-     * was destroyed.  When apr_pool_destroy() runs, it first destroys all
-     * all the per-connection sub-pools created in dbd_construct(), and
-     * then it runs the reslist's cleanup.  The cleanup calls dbd_destruct()
-     * on each resource, which would then attempt to destroy the sub-pools
-     * a second time.
-     */
     rv = apr_reslist_create(&group->reslist,
                             cfg->nmin, cfg->nkeep, cfg->nmax,
                             apr_time_from_sec(cfg->exptime),
-                            dbd_construct, dbd_destruct, group,
-                            group->pool);
+                            dbd_construct, dbd_destruct, group, pool);
     if (rv != APR_SUCCESS) {
         ap_log_error(APLOG_MARK, APLOG_ERR, rv, s,
                      "DBD: failed to initialise");
         return rv;
     }
 
-    apr_pool_cleanup_register(group->pool, group, dbd_destroy,
-                              apr_pool_cleanup_null);
-
     return APR_SUCCESS;
 }
 #endif
@@ -609,15 +578,8 @@
     for (group = group_list; group; group = group->next) {
         apr_status_t rv2;
 
-        rv2 = apr_pool_create(&group->pool, pool);
-        if (rv2 != APR_SUCCESS) {
-            ap_log_error(APLOG_MARK, APLOG_CRIT, rv2, s,
-                         "DBD: Failed to create reslist cleanup memory pool");
-            return rv2;
-        }
-
 #if APR_HAS_THREADS
-        rv2 = dbd_setup(s, group);
+        rv2 = dbd_setup(s, group, pool);
         if (rv2 == APR_SUCCESS) {
             continue;
         }
@@ -642,7 +604,8 @@
 }
 
 #if APR_HAS_THREADS
-static apr_status_t dbd_setup_lock(server_rec *s, dbd_group_t *group)
+static apr_status_t dbd_setup_lock(server_rec *s, dbd_group_t *group,
+                                   apr_pool_t *pool)
 {
     apr_status_t rv = APR_SUCCESS, rv2;
 
@@ -662,7 +625,7 @@
     }
 
     if (!group->reslist) {
-        rv = dbd_setup(s, group);
+        rv = dbd_setup(s, group, pool);
     }
 
     rv2 = apr_thread_mutex_unlock(group->mutex);
@@ -745,7 +708,7 @@
 
 #if APR_HAS_THREADS
     if (!group->reslist) {
-        if (dbd_setup_lock(s, group) != APR_SUCCESS) {
+        if (dbd_setup_lock(s, group, pool) != APR_SUCCESS) {
             return NULL;
         }
     }
@@ -775,7 +738,7 @@
 
     /* We don't have a connection right now, so we'll open one */
     if (!rec) {
-        dbd_construct((void*) &rec, group, group->pool);
+        dbd_construct((void*) &rec, group, pool);
         group->rec = rec;
     }
 #endif