Attempt of a backport of http://svn.apache.org/viewvc?diff_format=h&view=rev&revision=496831 See http://issues.apache.org/bugzilla/show_bug.cgi?id=39985 poeml@suse.de Index: modules/database/mod_dbd.c =================================================================== --- modules/database/mod_dbd.c (revision 497960) +++ modules/database/mod_dbd.c (working copy) @@ -50,10 +50,11 @@ const char *params; int persist; dbd_prepared *prepared; + apr_pool_t *pool; #if APR_HAS_THREADS apr_thread_mutex_t *mutex; - apr_pool_t *pool; apr_reslist_t *dbpool; + int destroyed; int nmin; int nkeep; int nmax; @@ -241,6 +242,12 @@ } return ret; } +static apr_status_t dbd_close(void *data) +{ + ap_dbd_t *rec = data; + return apr_dbd_close(rec->driver, rec->handle); +} + /************ svr cfg: manage db connection pool ****************/ /* an apr_reslist_constructor for SQL connections * Also use this for opening in non-reslist modes, since it gives @@ -249,16 +256,20 @@ static apr_status_t dbd_construct(void **db, void *params, apr_pool_t *pool) { svr_cfg *svr = (svr_cfg*) params; - ap_dbd_t *rec = apr_pcalloc(pool, sizeof(ap_dbd_t)); + apr_pool_t *rec_pool, *prepared_pool; + ap_dbd_t *rec; apr_status_t rv; - /* this pool is mostly so dbd_close can destroy the prepared stmts */ - rv = apr_pool_create(&rec->pool, pool); + rv = apr_pool_create(&rec_pool, pool); if (rv != APR_SUCCESS) { ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, pool, "DBD: Failed to create memory pool"); } + rec = apr_pcalloc(rec_pool, sizeof(ap_dbd_t)); + + rec->pool = rec_pool; + /* The driver is loaded at config time now, so this just checks a hash. * If that changes, the driver DSO could be registered to unload against * our pool, which is probably not what we want. Error checking isn't @@ -299,63 +310,94 @@ case APR_SUCCESS: break; } - *db = rec; - rv = dbd_prepared_init(rec->pool, svr, rec); + + apr_pool_cleanup_register(rec->pool, rec, dbd_close, + apr_pool_cleanup_null); + + /* we use a sub-pool for the prepared statements for each connection so + * that they will be cleaned up first, before the connection is closed + */ + rv = apr_pool_create(&prepared_pool, rec->pool); if (rv != APR_SUCCESS) { + ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, rec->pool, + "DBD: Failed to create memory pool"); + + apr_pool_destroy(rec->pool); + return rv; + } + + rv = dbd_prepared_init(prepared_pool, svr, rec); + if (rv != APR_SUCCESS) { const char *errmsg = apr_dbd_error(rec->driver, rec->handle, rv); ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, rec->pool, "DBD: failed to initialise prepared SQL statements: %s", (errmsg ? errmsg : "[???]")); } + + *db = rec; + return rv; } -static apr_status_t dbd_close(void *CONN) -{ - ap_dbd_t *conn = CONN; - apr_status_t rv = apr_dbd_close(conn->driver, conn->handle); - apr_pool_destroy(conn->pool); - return rv; -} #if APR_HAS_THREADS static apr_status_t dbd_destruct(void *sql, void *params, apr_pool_t *pool) { - return dbd_close(sql); + svr_cfg *svr = params; + + if (!svr->destroyed) { + ap_dbd_t *rec = sql; + + apr_pool_destroy(rec->pool); + } + + return APR_SUCCESS; } -static apr_status_t dbd_setup(apr_pool_t *pool, svr_cfg *svr) +static apr_status_t dbd_destroy(void *data) { + svr_cfg *svr = data; + + svr->destroyed = 1; + + return APR_SUCCESS; +} + +static apr_status_t dbd_setup(server_rec *s, svr_cfg *svr) +{ apr_status_t rv; - /* create a pool just for the reslist from a process-lifetime pool; - * that pool (s->process->pool in the dbd_setup_lock case, - * whatever was passed to ap_run_child_init in the dbd_setup_init case) - * will be shared with other threads doing other non-mod_dbd things - * so we can't use it for the reslist directly + /* 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_pool_create(&svr->pool, pool); - if (rv != APR_SUCCESS) { - ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, pool, - "DBD: Failed to create reslist memory pool"); - return rv; - } - rv = apr_reslist_create(&svr->dbpool, svr->nmin, svr->nkeep, svr->nmax, apr_time_from_sec(svr->exptime), dbd_construct, dbd_destruct, svr, svr->pool); - if (rv == APR_SUCCESS) { - apr_pool_cleanup_register(svr->pool, svr->dbpool, - (void*)apr_reslist_destroy, - apr_pool_cleanup_null); - } - else { - ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, svr->pool, + if (rv != APR_SUCCESS) { + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s, "DBD: failed to initialise"); - apr_pool_destroy(svr->pool); - svr->pool = NULL; + return rv; } - return rv; + apr_pool_cleanup_register(svr->pool, svr, dbd_destroy, + apr_pool_cleanup_null); + return APR_SUCCESS; } +#endif + static apr_status_t dbd_setup_init(apr_pool_t *pool, server_rec *s) { svr_cfg *svr = ap_get_module_config(s->module_config, &dbd_module); @@ -374,7 +416,15 @@ return APR_SUCCESS; } - rv = dbd_setup(pool, svr); + rv = apr_pool_create(&svr->pool, pool); + if (rv != APR_SUCCESS) { + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s, + "DBD: Failed to create reslist cleanup memory pool"); + return rv; + } + +#if APR_HAS_THREADS + rv = dbd_setup(s, svr); if (rv == APR_SUCCESS) { return rv; } @@ -387,9 +437,12 @@ ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, pool, "DBD: Failed to create thread mutex"); } +#endif + return rv; } -static apr_status_t dbd_setup_lock(apr_pool_t *pool, server_rec *s) +#if APR_HAS_THREADS +static apr_status_t dbd_setup_lock(server_rec *s) { svr_cfg *svr = ap_get_module_config(s->module_config, &dbd_module); apr_status_t rv, rv2 = APR_SUCCESS; @@ -404,18 +457,18 @@ rv = apr_thread_mutex_lock(svr->mutex); if (rv != APR_SUCCESS) { - ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, pool, + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s, "DBD: Failed to acquire thread mutex"); return rv; } if (!svr->dbpool) { - rv2 = dbd_setup(s->process->pool, svr); + rv2 = dbd_setup(s, svr); } rv = apr_thread_mutex_unlock(svr->mutex); if (rv != APR_SUCCESS) { - ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, pool, + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s, "DBD: Failed to release thread mutex"); if (rv2 == APR_SUCCESS) { rv2 = rv; @@ -434,7 +487,7 @@ { svr_cfg *svr = ap_get_module_config(s->module_config, &dbd_module); if (!svr->persist) { - dbd_close((void*) sql); + apr_pool_destroy(sql->pool); } #if APR_HAS_THREADS else { @@ -459,12 +512,12 @@ if (!svr->persist) { /* Return a once-only connection */ - rv = dbd_construct(&rec, svr, s->process->pool); + dbd_construct((void*) &rec, svr, pool); return (rv == APR_SUCCESS) ? arec : NULL; } if (!svr->dbpool) { - if (dbd_setup_lock(pool, s) != APR_SUCCESS) { + if (dbd_setup_lock(s) != APR_SUCCESS) { return NULL; } } @@ -503,7 +556,7 @@ if (!svr->persist) { /* Return a once-only connection */ - rv = dbd_construct(&rec, svr, s->process->pool); + dbd_construct((void*) &rec, svr, pool); return (rv == APR_SUCCESS) ? arec : NULL; } @@ -519,14 +572,14 @@ ap_log_perror(APLOG_MARK, APLOG_ERR, rv, pool, "DBD[%s] Error: %s", svr->name, errmsg); svr->conn = NULL; + apr_pool_destroy(rec->pool); } } + /* We don't have a connection right now, so we'll open one */ if (!svr->conn) { - if (dbd_construct(&rec, svr, s->process->pool) == APR_SUCCESS) { + if (dbd_construct(&rec, svr, svr->pool) == APR_SUCCESS) { svr->conn = arec ; - apr_pool_cleanup_register(s->process->pool, svr->conn, - dbd_close, apr_pool_cleanup_null); } } return svr->conn; @@ -569,10 +622,6 @@ apr_pool_cleanup_register(r->pool, req, dbd_release, apr_pool_cleanup_null); } - else { - apr_pool_cleanup_register(r->pool, req->conn, dbd_close, - apr_pool_cleanup_null); - } } } return req->conn; @@ -592,10 +641,6 @@ apr_pool_cleanup_register(c->pool, req, dbd_release, apr_pool_cleanup_null); } - else { - apr_pool_cleanup_register(c->pool, req->conn, dbd_close, - apr_pool_cleanup_null); - } } } return req->conn; @@ -617,15 +662,9 @@ ret = ap_get_module_config(r->request_config, &dbd_module); if (!ret) { - svr = ap_get_module_config(r->server->module_config, &dbd_module); ret = ap_dbd_open(r->pool, r->server); if (ret) { ap_set_module_config(r->request_config, &dbd_module, ret); - if (!svr->persist) { - apr_pool_cleanup_register(r->pool, svr->conn, dbd_close, - apr_pool_cleanup_null); - } - /* if persist then dbd_open registered cleanup on proc pool */ } } return ret; @@ -635,15 +674,9 @@ svr_cfg *svr; ap_dbd_t *ret = ap_get_module_config(c->conn_config, &dbd_module); if (!ret) { - svr = ap_get_module_config(c->base_server->module_config, &dbd_module); ret = ap_dbd_open(c->pool, c->base_server); if (ret) { ap_set_module_config(c->conn_config, &dbd_module, ret); - if (!svr->persist) { - apr_pool_cleanup_register(c->pool, svr->conn, dbd_close, - apr_pool_cleanup_null); - } - /* if persist then dbd_open registered cleanup on proc pool */ } } return ret; @@ -670,9 +703,7 @@ } static void dbd_hooks(apr_pool_t *pool) { -#if APR_HAS_THREADS ap_hook_child_init((void*)dbd_setup_init, NULL, NULL, APR_HOOK_MIDDLE); -#endif APR_REGISTER_OPTIONAL_FN(ap_dbd_open); APR_REGISTER_OPTIONAL_FN(ap_dbd_close); APR_REGISTER_OPTIONAL_FN(ap_dbd_acquire);