httpd-cvs mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From n..@apache.org
Subject svn commit: r395981 - in /httpd/httpd/branches/2.2.x: CHANGES STATUS modules/database/mod_dbd.c modules/database/mod_dbd.h
Date Fri, 21 Apr 2006 18:30:04 GMT
Author: niq
Date: Fri Apr 21 11:30:02 2006
New Revision: 395981

URL: http://svn.apache.org/viewcvs?rev=395981&view=rev
Log:
Backport mod_dbd updates

Modified:
    httpd/httpd/branches/2.2.x/CHANGES
    httpd/httpd/branches/2.2.x/STATUS
    httpd/httpd/branches/2.2.x/modules/database/mod_dbd.c
    httpd/httpd/branches/2.2.x/modules/database/mod_dbd.h

Modified: httpd/httpd/branches/2.2.x/CHANGES
URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/2.2.x/CHANGES?rev=395981&r1=395980&r2=395981&view=diff
==============================================================================
--- httpd/httpd/branches/2.2.x/CHANGES [utf-8] (original)
+++ httpd/httpd/branches/2.2.x/CHANGES [utf-8] Fri Apr 21 11:30:02 2006
@@ -1,6 +1,13 @@
                                                         -*- coding: utf-8 -*-
 Changes with Apache 2.2.2
 
+  *) mod_dbd: Update defaults, improve error reporting.
+     [Chris Darroch <chrisd pearsoncmg com>, Nick Kew]
+
+  *) mod_dbd: Create own pool and mutex to avoid problem use of
+     process pool in request processing.
+     [Chris Darroch <chrisd pearsoncmg com>]
+
   *) HTML-escape the Expect error message.  Not classed as security as
      an attacker has no way to influence the Expect header a victim will
      send to a target site.  Reported by Thiago Zaninotti

Modified: httpd/httpd/branches/2.2.x/STATUS
URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/2.2.x/STATUS?rev=395981&r1=395980&r2=395981&view=diff
==============================================================================
--- httpd/httpd/branches/2.2.x/STATUS (original)
+++ httpd/httpd/branches/2.2.x/STATUS Fri Apr 21 11:30:02 2006
@@ -124,13 +124,6 @@
               jim, does your agreement with niq also include his +1 in the case
               the comment is fixed?
 
-    * mod_dbd: When threaded, create a private pool in child_init
-      so we're not using the process pool later.  Also improve
-      configuration defaults and merge function.
-      http://svn.apache.org/viewcvs.cgi?rev=348026&view=rev
-      http://svn.apache.org/viewcvs.cgi?rev=354400&view=rev
-      +1 niq, trawick, pquerna
-
 PATCHES PROPOSED TO BACKPORT FROM TRUNK:
 
     * Backport mod_authz_dbd (it's had some testing now)

Modified: httpd/httpd/branches/2.2.x/modules/database/mod_dbd.c
URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/2.2.x/modules/database/mod_dbd.c?rev=395981&r1=395980&r2=395981&view=diff
==============================================================================
--- httpd/httpd/branches/2.2.x/modules/database/mod_dbd.c (original)
+++ httpd/httpd/branches/2.2.x/modules/database/mod_dbd.c Fri Apr 21 11:30:02 2006
@@ -1,4 +1,4 @@
-/* Copyright 2003-5 The Apache Software Foundation or its licensors, as
+/* Copyright 2003-6 The Apache Software Foundation or its licensors, as
  * applicable.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
@@ -34,6 +34,11 @@
 
 /************ svr cfg: manage db connection pool ****************/
 
+#define NMIN_SET     0x1
+#define NKEEP_SET    0x2
+#define NMAX_SET     0x4
+#define EXPTIME_SET  0x8
+
 typedef struct dbd_prepared {
     const char *label;
     const char *query;
@@ -45,6 +50,8 @@
     int persist;
     dbd_prepared *prepared;
 #if APR_HAS_THREADS
+    apr_thread_mutex_t *mutex;
+    apr_pool_t *pool;
     apr_reslist_t *dbpool;
     int nmin;
     int nkeep;
@@ -53,6 +60,7 @@
 #else
     ap_dbd_t *conn;
 #endif
+    unsigned int set;
 } svr_cfg;
 
 typedef enum { cmd_name, cmd_params, cmd_persist,
@@ -94,33 +102,45 @@
     case cmd_params:
         svr->params = val;
         break;
-    case cmd_persist:
-        ISINT(val);
-        svr->persist = atoi(val);
-        break;
 #if APR_HAS_THREADS
     case cmd_min:
         ISINT(val);
         svr->nmin = atoi(val);
+        svr->set |= NMIN_SET;
         break;
     case cmd_keep:
         ISINT(val);
         svr->nkeep = atoi(val);
+        svr->set |= NKEEP_SET;
         break;
     case cmd_max:
         ISINT(val);
         svr->nmax = atoi(val);
+        svr->set |= NMAX_SET;
         break;
     case cmd_exp:
         ISINT(val);
         svr->exptime = atoi(val);
+        svr->set |= EXPTIME_SET;
         break;
 #endif
     }
     return NULL;
 }
+static const char *dbd_param_flag(cmd_parms *cmd, void *cfg, int flag)
+{
+    svr_cfg *svr = (svr_cfg*) ap_get_module_config
+        (cmd->server->module_config, &dbd_module);
+
+    switch ((long) cmd->info) {
+    case cmd_persist:
+        svr->persist = flag;
+        break;
+    }
+    return NULL;
+}
 DBD_DECLARE_NONSTD(void) ap_dbd_prepare(server_rec *s, const char *query,
-                                const char *label)
+                                        const char *label)
 {
     svr_cfg *svr = ap_get_module_config(s->module_config, &dbd_module);
     dbd_prepared *prepared = apr_pcalloc(s->process->pool, sizeof(dbd_prepared));
@@ -140,53 +160,52 @@
                   "SQL Driver"),
     AP_INIT_TAKE1("DBDParams", dbd_param, (void*)cmd_params, RSRC_CONF,
                   "SQL Driver Params"),
-    AP_INIT_TAKE1("DBDPersist", dbd_param, (void*)cmd_persist, RSRC_CONF,
-                  "Use persistent connection/pool"),
+    AP_INIT_FLAG("DBDPersist", dbd_param_flag, (void*)cmd_persist, RSRC_CONF,
+                 "Use persistent connection/pool"),
     AP_INIT_TAKE2("DBDPrepareSQL", dbd_prepare, NULL, RSRC_CONF,
                   "Prepared SQL statement, label"),
 #if APR_HAS_THREADS
     AP_INIT_TAKE1("DBDMin", dbd_param, (void*)cmd_min, RSRC_CONF,
                   "Minimum number of connections"),
+    /* XXX: note that mod_proxy calls this "smax" */
     AP_INIT_TAKE1("DBDKeep", dbd_param, (void*)cmd_keep, RSRC_CONF,
                   "Maximum number of sustained connections"),
     AP_INIT_TAKE1("DBDMax", dbd_param, (void*)cmd_max, RSRC_CONF,
                   "Maximum number of connections"),
+    /* XXX: note that mod_proxy calls this "ttl" (time to live) */
     AP_INIT_TAKE1("DBDExptime", dbd_param, (void*)cmd_exp, RSRC_CONF,
                   "Keepalive time for idle connections"),
 #endif
     {NULL}
 };
-#define DEFAULT_NMIN 0
-#define DEFAULT_NMAX 5
-#define DEFAULT_NKEEP 1
-#define DEFAULT_EXPTIME 120
-#define COND_PARAM(x,val) \
-    if (cfg->x == val) {              \
-        cfg->x = ((svr_cfg*)base)->x; \
-    }
-#define COND_PARAM0(x) COND_PARAM(x,0)
-#define COND_PARAM1(x) COND_PARAM(x,-1)
-static void *dbd_merge(apr_pool_t *pool, void *base, void *add) {
-    svr_cfg *cfg = apr_pmemdup(pool, add, sizeof(svr_cfg));
-    COND_PARAM0(name);
-    COND_PARAM0(params);
-    COND_PARAM1(persist);
+static void *dbd_merge(apr_pool_t *pool, void *BASE, void *ADD) {
+    svr_cfg *base = (svr_cfg*) BASE;
+    svr_cfg *add = (svr_cfg*) ADD;
+    svr_cfg *cfg = apr_pcalloc(pool, sizeof(svr_cfg));
+    cfg->name = strcmp(add->name, "") ? add->name : base->name;
+    cfg->params = strcmp(add->params, "") ? add->params : base->params;
+    cfg->persist = (add->persist == -1) ? base->persist : add->persist;
 #if APR_HAS_THREADS
-    COND_PARAM(nmin, DEFAULT_NMIN);
-    COND_PARAM(nkeep, DEFAULT_NKEEP);
-    COND_PARAM(nmax, DEFAULT_NMAX);
-    COND_PARAM(exptime, DEFAULT_EXPTIME);
+    cfg->nmin = (add->set&NMIN_SET) ? add->nmin : base->nmin;
+    cfg->nkeep = (add->set&NKEEP_SET) ? add->nkeep : base->nkeep;
+    cfg->nmax = (add->set&NMAX_SET) ? add->nmax : base->nmax;
+    cfg->exptime = (add->set&EXPTIME_SET) ? add->exptime : base->exptime;
 #endif
-    return cfg;
+    cfg->set = add->set | base->set;
+    return (void*) cfg;
 }
-#undef COND_PARAM
-#undef COND_PARAM0
-#undef COND_PARAM1
+/* A default nmin of >0 will help with generating meaningful
+ * startup error messages if the database is down.
+ */
+#define DEFAULT_NMIN 1
+#define DEFAULT_NKEEP 2
+#define DEFAULT_NMAX 10
+#define DEFAULT_EXPTIME 300
 static void *dbd_cfg(apr_pool_t *p, server_rec *x)
 {
     svr_cfg *svr = (svr_cfg*) apr_pcalloc(p, sizeof(svr_cfg));
-    svr->persist = -1;
     svr->name = svr->params = ""; /* don't risk segfault on misconfiguration */
+    svr->persist = -1;
 #if APR_HAS_THREADS
     svr->nmin = DEFAULT_NMIN;
     svr->nkeep = DEFAULT_NKEEP;
@@ -224,75 +243,168 @@
 {
     svr_cfg *svr = (svr_cfg*) params;
     ap_dbd_t *rec = apr_pcalloc(pool, sizeof(ap_dbd_t));
-    apr_status_t rv = apr_dbd_get_driver(pool, svr->name, &rec->driver);
+    apr_status_t rv;
+
+    /* this pool is mostly so dbd_close can destroy the prepared stmts */
+    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");
+    }
 
-/* Error-checking get_driver isn't necessary now (because it's done at
- * config-time).  But in case that changes in future ...
+/* 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
+ * necessary now, but in case that changes in the future ...
  */
+    rv = apr_dbd_get_driver(rec->pool, svr->name, &rec->driver);
     switch (rv) {
     case APR_ENOTIMPL:
-        ap_log_perror(APLOG_MARK, APLOG_CRIT, 0, pool,
+        ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, rec->pool,
                       "DBD: driver for %s not available", svr->name);
         return rv;
     case APR_EDSOOPEN:
-        ap_log_perror(APLOG_MARK, APLOG_CRIT, 0, pool,
+        ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, rec->pool,
                       "DBD: can't find driver for %s", svr->name);
         return rv;
     case APR_ESYMNOTFOUND:
-        ap_log_perror(APLOG_MARK, APLOG_CRIT, 0, pool,
+        ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, rec->pool,
                       "DBD: driver for %s is invalid or corrupted", svr->name);
         return rv;
     default:
-        ap_log_perror(APLOG_MARK, APLOG_CRIT, 0, pool,
+        ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, rec->pool,
                       "DBD: mod_dbd not compatible with apr in get_driver");
         return rv;
     case APR_SUCCESS:
         break;
     }
 
-    rv = apr_dbd_open(rec->driver, pool, svr->params, &rec->handle);
+    rv = apr_dbd_open(rec->driver, rec->pool, svr->params, &rec->handle);
     switch (rv) {
     case APR_EGENERAL:
-        ap_log_perror(APLOG_MARK, APLOG_CRIT, 0, pool,
-                      "DBD: Can't connect to %s[%s]", svr->name, svr->params);
+        ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, rec->pool,
+                      "DBD: Can't connect to %s", svr->name);
         return rv;
     default:
-        ap_log_perror(APLOG_MARK, APLOG_CRIT, 0, pool,
+        ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, rec->pool,
                       "DBD: mod_dbd not compatible with apr in open");
         return rv;
     case APR_SUCCESS:
         break;
     }
     *db = rec;
-    rv = dbd_prepared_init(pool, svr, rec);
+    rv = dbd_prepared_init(rec->pool, svr, rec);
+    if (rv != APR_SUCCESS) {
+        ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, rec->pool,
+                      "DBD: failed to initialise prepared SQL statements");
+    }
+    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)
 {
-    ap_dbd_t *rec = sql;
-    return apr_dbd_close(rec->driver, rec->handle);
+    return dbd_close(sql);
 }
 
-static apr_status_t dbd_setup(apr_pool_t *pool, server_rec *s)
+static apr_status_t dbd_setup(apr_pool_t *pool, svr_cfg *svr)
 {
-    svr_cfg *svr = ap_get_module_config(s->module_config, &dbd_module);
-    apr_status_t rv = apr_reslist_create(&svr->dbpool, svr->nmin, svr->nkeep,
-                                         svr->nmax, svr->exptime,
-                                         dbd_construct, dbd_destruct,
-                                         svr, pool);
+    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
+     */
+    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(pool, svr->dbpool,
+        apr_pool_cleanup_register(svr->pool, svr->dbpool,
                                   (void*)apr_reslist_destroy,
                                   apr_pool_cleanup_null);
     }
     else {
-        ap_log_perror(APLOG_MARK, APLOG_CRIT, 0, pool,
-                      "DBD Pool: failed to initialise");
+        ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, svr->pool,
+                      "DBD: failed to initialise");
+        apr_pool_destroy(svr->pool);
+        svr->pool = NULL;
     }
+
     return rv;
 }
+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);
+    apr_status_t rv;
+
+    if (!svr->persist) {
+        return APR_SUCCESS;
+    }
+
+    rv = dbd_setup(pool, svr);
+    if (rv == APR_SUCCESS) {
+        return rv;
+    }
+
+    /* we failed, so create a mutex so that subsequent competing callers
+     * to ap_dbd_open can serialize themselves while they retry
+     */
+    rv = apr_thread_mutex_create(&svr->mutex, APR_THREAD_MUTEX_DEFAULT, pool);
+    if (rv != APR_SUCCESS) {
+        ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, pool,
+                      "DBD: Failed to create thread mutex");
+    }
+    return rv;
+}
+static apr_status_t dbd_setup_lock(apr_pool_t *pool, server_rec *s)
+{
+    svr_cfg *svr = ap_get_module_config(s->module_config, &dbd_module);
+    apr_status_t rv, rv2 = APR_SUCCESS;
 
+    /* several threads could be here at the same time, all trying to
+     * initialize the reslist because dbd_setup_init failed to do so
+     */
+    if (!svr->mutex) {
+        /* we already logged an error when the mutex couldn't be created */
+        return APR_EGENERAL;
+    }
+
+    rv = apr_thread_mutex_lock(svr->mutex);
+    if (rv != APR_SUCCESS) {
+        ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, pool,
+                      "DBD: Failed to acquire thread mutex");
+        return rv;
+    }
+
+    if (!svr->dbpool) {
+        rv2 = dbd_setup(s->process->pool, svr);
+    }
+
+    rv = apr_thread_mutex_unlock(svr->mutex);
+    if (rv != APR_SUCCESS) {
+        ap_log_perror(APLOG_MARK, APLOG_CRIT, rv, pool,
+                      "DBD: Failed to release thread mutex");
+        if (rv2 == APR_SUCCESS) {
+            rv2 = rv;
+        }
+    }
+    return rv2;
+}
 #endif
 
 
@@ -304,7 +416,7 @@
 {
     svr_cfg *svr = ap_get_module_config(s->module_config, &dbd_module);
     if (!svr->persist) {
-        apr_dbd_close(sql->driver, sql->handle);
+        dbd_close((void*) sql);
     }
 #if APR_HAS_THREADS
     else {
@@ -312,11 +424,6 @@
     }
 #endif
 }
-static apr_status_t dbd_close(void *CONN)
-{
-    ap_dbd_t *conn = CONN;
-    return apr_dbd_close(conn->driver, conn->handle);
-}
 #define arec ((ap_dbd_t*)rec)
 #if APR_HAS_THREADS
 DBD_DECLARE_NONSTD(ap_dbd_t*) ap_dbd_open(apr_pool_t *pool, server_rec *s)
@@ -333,12 +440,13 @@
     }
 
     if (!svr->dbpool) {
-        if (dbd_setup(s->process->pool, s) != APR_SUCCESS) {
+        if (dbd_setup_lock(pool, s) != APR_SUCCESS) {
             return NULL;
         }
     }
-    if (apr_reslist_acquire(svr->dbpool, &rec) != APR_SUCCESS) {
-        ap_log_perror(APLOG_MARK, APLOG_ERR, 0, pool,
+    rv = apr_reslist_acquire(svr->dbpool, &rec);
+    if (rv != APR_SUCCESS) {
+        ap_log_perror(APLOG_MARK, APLOG_ERR, rv, pool,
                       "Failed to acquire DBD connection from pool!");
         return NULL;
     }
@@ -348,7 +456,7 @@
         if (!errmsg) {
             errmsg = "(unknown)";
         }
-        ap_log_perror(APLOG_MARK, APLOG_ERR, 0, pool,
+        ap_log_perror(APLOG_MARK, APLOG_ERR, rv, pool,
                       "DBD[%s] Error: %s", svr->name, errmsg );
         apr_reslist_invalidate(svr->dbpool, rec);
         return NULL;
@@ -378,7 +486,7 @@
             if (!errmsg) {
                 errmsg = "(unknown)";
             }
-            ap_log_perror(APLOG_MARK, APLOG_ERR, 0, pool,
+            ap_log_perror(APLOG_MARK, APLOG_ERR, rv, pool,
                           "DBD[%s] Error: %s", svr->name, errmsg);
             svr->conn = NULL;
         }
@@ -493,7 +601,7 @@
 static void dbd_hooks(apr_pool_t *pool)
 {
 #if APR_HAS_THREADS
-    ap_hook_child_init((void*)dbd_setup, NULL, NULL, APR_HOOK_MIDDLE);
+    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);

Modified: httpd/httpd/branches/2.2.x/modules/database/mod_dbd.h
URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/2.2.x/modules/database/mod_dbd.h?rev=395981&r1=395980&r2=395981&view=diff
==============================================================================
--- httpd/httpd/branches/2.2.x/modules/database/mod_dbd.h (original)
+++ httpd/httpd/branches/2.2.x/modules/database/mod_dbd.h Fri Apr 21 11:30:02 2006
@@ -1,5 +1,4 @@
-/* Copyright 2003-5 WebThing Ltd
- * Copyright 2005 The Apache Software Foundation or its licensors, as
+/* Copyright 2003-6 The Apache Software Foundation or its licensors, as
  * applicable.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
@@ -17,7 +16,7 @@
 
 /**
  * @file  mod_dbd.h
- * @brief Database Acces Extension Module for Apache
+ * @brief Database Access Extension Module for Apache
  *
  * Overview of what this is and does:
  * http://www.apache.org/~niq/dbd.html
@@ -61,6 +60,7 @@
     apr_dbd_t *handle;
     const apr_dbd_driver_t *driver;
     apr_hash_t *prepared;
+    apr_pool_t *pool;
 } ap_dbd_t;
 
 /* Export functions to access the database */



Mime
View raw message