httpd-cvs mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From chr...@apache.org
Subject svn commit: r909672 - in /httpd/mod_fcgid/trunk: CHANGES-FCGID modules/fcgid/fcgid_bridge.c modules/fcgid/mod_fcgid.c
Date Sat, 13 Feb 2010 00:49:29 GMT
Author: chrisd
Date: Sat Feb 13 00:49:29 2010
New Revision: 909672

URL: http://svn.apache.org/viewvc?rev=909672&view=rev
Log:
Allow processes to be reused within multiple phases of a request
by releasing them into the free list as soon as possible after reading
their response headers.  This applies to processes being used in
the FCGI_AUTHORIZER role, as well as those returning error codes or
redirections.

Thus a single process may act as both an authorizer and a responder for
a single request.  It may also handle ErrorDocument redirections,
e.g., if it responds with a 401 status code while authorizing, the same
process may then handle that error and respond with a custom login page.

Modified:
    httpd/mod_fcgid/trunk/CHANGES-FCGID
    httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c
    httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c

Modified: httpd/mod_fcgid/trunk/CHANGES-FCGID
URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/CHANGES-FCGID?rev=909672&r1=909671&r2=909672&view=diff
==============================================================================
--- httpd/mod_fcgid/trunk/CHANGES-FCGID [utf8] (original)
+++ httpd/mod_fcgid/trunk/CHANGES-FCGID [utf8] Sat Feb 13 00:49:29 2010
@@ -1,6 +1,10 @@
                                                          -*- coding: utf-8 -*-
 Changes with mod_fcgid 2.3.6
 
+  *) Allow processes to be reused within multiple phases of a request
+     by releasing them into the free list as soon as possible.
+     [Chris Darroch]
+
   *) Fix lookup of process command lines when using FcgidWrapper or
      access control directives, including within .htaccess files.
      [Chris Darroch]

Modified: httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c
URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c?rev=909672&r1=909671&r2=909672&view=diff
==============================================================================
--- httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c (original)
+++ httpd/mod_fcgid/trunk/modules/fcgid/fcgid_bridge.c Sat Feb 13 00:49:29 2010
@@ -169,7 +169,13 @@
         ctx->buffer = NULL;
     }
 
-    proc_close_ipc(&ctx->ipc);
+    /* proc_close_ipc() and ipc_handle_cleanup() do their own sanity
+     * checks, but we'll do our own anyway
+     */
+    if (ctx->ipc.ipc_handle_info) {
+        proc_close_ipc(&ctx->ipc);
+        ctx->ipc.ipc_handle_info = NULL;
+    }
 
     if (ctx->procnode) {
         ++ctx->procnode->requests_handled;
@@ -282,22 +288,93 @@
 }
 
 static int
+handle_request_ipc(request_rec *r, int role,
+                   apr_bucket_brigade *output_brigade,
+                   fcgid_bucket_ctx *bucket_ctx, const char **location_ptr)
+{
+    int cond_status;
+    apr_status_t rv;
+    apr_bucket_brigade *brigade_stdout;
+    char sbuf[MAX_STRING_LEN];
+    const char *location;
+
+    /* Write output_brigade to fastcgi server */
+    if ((rv = proc_write_ipc(&bucket_ctx->ipc,
+                             output_brigade)) != APR_SUCCESS) {
+        bucket_ctx->has_error = 1;
+        return HTTP_INTERNAL_SERVER_ERROR;
+    }
+
+    /* Create brigade */
+    brigade_stdout =
+        apr_brigade_create(r->pool, r->connection->bucket_alloc);
+    APR_BRIGADE_INSERT_TAIL(brigade_stdout,
+                            ap_bucket_fcgid_header_create(r->connection->
+                                                          bucket_alloc,
+                                                          bucket_ctx));
+
+    /* Check the script header first; return immediately on error. */
+    if ((cond_status =
+         ap_scan_script_header_err_core(r, sbuf, getsfunc_fcgid_BRIGADE,
+                                        brigade_stdout)) >= 400) {
+        return cond_status;
+    }
+
+    /* Check redirect */
+    location = apr_table_get(r->headers_out, "Location");
+
+    if (location && location[0] == '/' && r->status == 200) {
+        /* This redirect needs to be a GET no matter what the original 
+         * method was. 
+         */
+        r->method = apr_pstrdup(r->pool, "GET");
+        r->method_number = M_GET;
+
+        /* We already read the message body (if any), so don't allow 
+         * the redirected request to think it has one.  We can ignore 
+         * Transfer-Encoding, since we used REQUEST_CHUNKED_ERROR. 
+         */
+        apr_table_unset(r->headers_in, "Content-Length");
+
+        *location_ptr = location;
+        return HTTP_OK;
+    }
+    else if (location && r->status == 200) {
+        /* XX Note that if a script wants to produce its own Redirect 
+         * body, it now has to explicitly *say* "Status: 302" 
+         */
+        return HTTP_MOVED_TEMPORARILY;
+    }
+
+    /* Now pass to output filter */
+    if (role == FCGI_RESPONDER
+        && (rv = ap_pass_brigade(r->output_filters,
+                                 brigade_stdout)) != APR_SUCCESS) {
+        if (!APR_STATUS_IS_ECONNABORTED(rv)) {
+            ap_log_rerror(APLOG_MARK, APLOG_WARNING, rv, r,
+                          "mod_fcgid: ap_pass_brigade failed in "
+                          "handle_request_ipc function");
+        }
+
+        return HTTP_INTERNAL_SERVER_ERROR;
+    }
+
+    return cond_status;
+}
+
+static int
 handle_request(request_rec * r, int role, fcgid_cmd_conf *cmd_conf,
                apr_bucket_brigade * output_brigade)
 {
-    apr_pool_t *request_pool = r->main ? r->main->pool : r->pool;
     fcgid_command fcgi_request;
     fcgid_bucket_ctx *bucket_ctx;
     int i, j, cond_status;
-    apr_status_t rv;
-    apr_bucket_brigade *brigade_stdout;
-    char sbuf[MAX_STRING_LEN];
-    const char *location;
+    const char *location = NULL;
 
-    bucket_ctx = apr_pcalloc(request_pool, sizeof(*bucket_ctx));
+    bucket_ctx = apr_pcalloc(r->pool, sizeof(*bucket_ctx));
 
     bucket_ctx->ipc.request = r;
-    apr_pool_cleanup_register(request_pool, bucket_ctx,
+    apr_pool_cleanup_register(r->pool, bucket_ctx,
                               bucket_ctx_cleanup, apr_pool_cleanup_null);
 
     /* Try to get a connected ipc handle */
@@ -357,63 +434,36 @@
         apr_time_now();
     bucket_ctx->procnode->diewhy = FCGID_DIE_KILLSELF;
 
-    /* Write output_brigade to fastcgi server */
-    if ((rv =
-         proc_write_ipc(&bucket_ctx->ipc, output_brigade)) != APR_SUCCESS) {
-        bucket_ctx->has_error = 1;
-        return HTTP_INTERNAL_SERVER_ERROR;
-    }
-
-    /* Create brigade */
-    brigade_stdout =
-        apr_brigade_create(request_pool, r->connection->bucket_alloc);
-    APR_BRIGADE_INSERT_TAIL(brigade_stdout,
-                            ap_bucket_fcgid_header_create(r->connection->
-                                                          bucket_alloc,
-                                                          bucket_ctx));
-    /*APR_BRIGADE_INSERT_TAIL(brigade_stdout, apr_bucket_flush_create(r->connection->bucket_alloc));
*/
-
-    /* Check the script header first. If got error, return immediately */
-    if ((cond_status = ap_scan_script_header_err_core
-         (r, sbuf, getsfunc_fcgid_BRIGADE, brigade_stdout)) >= 400)
-        return cond_status;
-
-    /* Check redirect */
-    location = apr_table_get(r->headers_out, "Location");
+    cond_status = handle_request_ipc(r, role, output_brigade,
+                                     bucket_ctx, &location);
 
-    if (location && location[0] == '/' && r->status == 200) {
-        /* This redirect needs to be a GET no matter what the original 
-         * method was. 
-         */
-        r->method = apr_pstrdup(r->pool, "GET");
-        r->method_number = M_GET;
+    /* Release the process ASAP.  This may already have been done in
+     * ap_pass_brigade() by fcgid_header_bucket_read(), but not in the
+     * case where handle_request_ipc() returned early without reading
+     * the body of the HTTP response.  This could be because of an error,
+     * or because of a role or a status code which permits us to ignore
+     * the message body.
+     *
+     * As an example, when handling a request in the FCGI_AUTHORIZER role,
+     * we don't read through to the end of the response from the process,
+     * we just read the HTTP headers.  That means each phase of the
+     * request handling sequence (e.g., authentication, authorization, etc.)
+     * will require its own process unless we make sure to always release
+     * any process we acquired regardless of whether we're reading the
+     * response body.
+     *
+     * As another example, if we perform or cause an internal redirection
+     * (for instance, by returning an error code that invokes a script
+     * handler in ap_die() because of an ErrorDocument configuration), then
+     * we must also release the process we acquired here so that it is
+     * potentially available during the next handling phase.
+     */
 
-        /* We already read the message body (if any), so don't allow 
-         * the redirected request to think it has one. We can ignore 
-         * Transfer-Encoding, since we used REQUEST_CHUNKED_ERROR. 
-         */
-        apr_table_unset(r->headers_in, "Content-Length");
+    apr_pool_cleanup_run(r->pool, bucket_ctx, bucket_ctx_cleanup);
 
+    /* Perform internal redirection if necessary */
+    if (location) {
         ap_internal_redirect_handler(location, r);
-        return HTTP_OK;
-    }
-    else if (location && r->status == 200) {
-        /* XX Note that if a script wants to produce its own Redirect 
-         * body, it now has to explicitly *say* "Status: 302" 
-         */
-        return HTTP_MOVED_TEMPORARILY;
-    }
-
-    /* Now pass to output filter */
-    if (role == FCGI_RESPONDER && (rv =
-                                   ap_pass_brigade(r->output_filters,
-                                                   brigade_stdout)) !=
-        APR_SUCCESS) {
-        if (!APR_STATUS_IS_ECONNABORTED(rv)) {
-            ap_log_rerror(APLOG_MARK, APLOG_WARNING, rv, r,
-                          "mod_fcgid: ap_pass_brigade failed in handle_request function");
-        }
-        return HTTP_INTERNAL_SERVER_ERROR;
     }
 
     /* Retrun condition status */
@@ -610,16 +660,15 @@
 
 int bridge_request(request_rec * r, int role, fcgid_cmd_conf *cmd_conf)
 {
-    apr_pool_t *request_pool = r->main ? r->main->pool : r->pool;
     apr_bucket_brigade *output_brigade;
     apr_bucket *bucket_eos;
-    char **envp = ap_create_environment(request_pool,
+    char **envp = ap_create_environment(r->pool,
                                         r->subprocess_env);
     int rc;
 
     /* Create brigade for the request to fastcgi server */
     output_brigade =
-        apr_brigade_create(request_pool, r->connection->bucket_alloc);
+        apr_brigade_create(r->pool, r->connection->bucket_alloc);
 
     /* Build the begin request and environ request, append them to output_brigade */
     if (!build_begin_block
@@ -632,7 +681,7 @@
     }
 
     if (role == FCGI_RESPONDER) {
-        rc = add_request_body(r, request_pool, output_brigade);
+        rc = add_request_body(r, r->pool, output_brigade);
         if (rc) {
             return rc;
         }

Modified: httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c
URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c?rev=909672&r1=909671&r2=909672&view=diff
==============================================================================
--- httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c (original)
+++ httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c Sat Feb 13 00:49:29 2010
@@ -144,7 +144,6 @@
     cgi_exec_info_t e_info;
     const char *command;
     const char **argv;
-    apr_pool_t *p;
     apr_status_t rv;
     int http_retcode;
     fcgid_cmd_conf *wrapper_conf;
@@ -169,7 +168,6 @@
     e_info.bb = NULL;
     e_info.ctx = NULL;
     e_info.next = NULL;
-    p = r->main ? r->main->pool : r->pool;
 
     wrapper_conf = get_wrapper_info(r->filename, r);
 
@@ -185,7 +183,7 @@
     /* Build the command line */
     if (wrapper_conf) {
         if ((rv =
-             default_build_command(&command, &argv, r, p,
+             default_build_command(&command, &argv, r, r->pool,
                                    &e_info)) != APR_SUCCESS) {
             ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
                           "mod_fcgid: don't know how to spawn wrapper child process: %s",
@@ -193,7 +191,7 @@
             return HTTP_INTERNAL_SERVER_ERROR;
         }
     } else {
-        if ((rv = cgi_build_command(&command, &argv, r, p,
+        if ((rv = cgi_build_command(&command, &argv, r, r->pool,
                                     &e_info)) != APR_SUCCESS) {
             ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
                           "mod_fcgid: don't know how to spawn child process: %s",



Mime
View raw message