httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject Re: mod_fcgid and streaming large request bodies
Date Fri, 27 Dec 2013 23:42:09 GMT
On Fri, Dec 27, 2013 at 6:14 PM, Justin Erenkrantz <justin@erenkrantz.com>wrote:

> On Fri, Dec 27, 2013 at 8:49 AM, Justin Erenkrantz
> <justin@erenkrantz.com> wrote:
> > Anyway, I'm diving into the code a bit - but, I figured it might be
> useful
> > to see if anyone else has any thoughts about how to handle large request
> > bodies with mod_fcgid.
>
> Here's a first-cut patch that compiles at least.  Cheers.  -- justin
>
> Add FcgidStreamRequestBody directive to send request body as it arrives
> without
> storing it in memory or on disk.
>

It would be quite valuable if there is a limit on how much can be pre-read
(0 for pure streaming).  Pre-reading the request body reduces the number of
application processes or threads required, and they are usually fatter
(sometimes dramatically so) than httpd workers.  While the current
implementation is truly ridiculous, there's some goodness that could be
kept.  I'll try to look through this in the next few days and see if it is
practical to keep the best of both versions.  If no round tuits are
available we should at least proceed with this, perhaps changing the
directive syntax to allow a hybrid implementation in the future with no
legacy cruft.  (Wild suggestion: FcgiPreReadRequestBody
Off|On|Unlimited|nnnnK, where On is <default>K)


> * modules/fcgid/mod_fcgid.c
>   (fcgid_cmds): Add FcgidStreamRequestBody directive.
> * modules/fcgid/fcgid_conf.h
>   (fcgid_server_conf): Add stream_request_body/stream_request_body_set
> flags.
> * modules/fcgid/fcgid_conf.c
>   (DEFAULT_STREAM_REQUEST_BODY): Set to 0.
>   (create_fcgid_server_config): Init stream_request_body flag.
>   (merge_fcgid_server_config): Merge new stream_request_body flag.
>   (set_stream_request_body): New config helper.
> * modules/fcgid/fcgid_bridge.c
>   (add_request_body): Add a forward declaration to reduce diff (for now);
>   take fcgid_bucket_ctx param; if stream_request_body is set, don't copy
>   bucket and then call proc_write_ipc to send out that brigade and clear it
>   out before next loop iteration.
>   (bridge_request): Delay reading the request body until later.
>   (handle_request_ipc): Move add_request_body call to here and append the
> EOS
>   bucket before we do the final write of the request body.
>
> Index: modules/fcgid/fcgid_bridge.c
> ===================================================================
> --- modules/fcgid/fcgid_bridge.c (revision 1553671)
> +++ modules/fcgid/fcgid_bridge.c (working copy)
> @@ -287,6 +287,10 @@ static int getsfunc_fcgid_BRIGADE(char *buf, int l
>      return done;
>  }
>
> +static int add_request_body(request_rec *r, apr_pool_t *request_pool,
> +                            apr_bucket_brigade *output_brigade,
> +                            fcgid_bucket_ctx *bucket_ctx);
> +
>  static int
>  handle_request_ipc(request_rec *r, int role,
>                     apr_bucket_brigade *output_brigade,
> @@ -295,9 +299,21 @@ handle_request_ipc(request_rec *r, int role,
>      int cond_status;
>      apr_status_t rv;
>      apr_bucket_brigade *brigade_stdout;
> +    apr_bucket *bucket_eos;
>      char sbuf[MAX_STRING_LEN];
>      const char *location;
>
> +    if (role == FCGI_RESPONDER) {
> +        rv = add_request_body(r, r->pool, output_brigade, bucket_ctx);
> +        if (rv) {
> +            return rv;
> +        }
> +    }
> +
> +    /* The eos bucket now */
> +    bucket_eos = apr_bucket_eos_create(r->connection->bucket_alloc);
> +    APR_BRIGADE_INSERT_TAIL(output_brigade, bucket_eos);
> +
>      /* Write output_brigade to fastcgi server */
>      if ((rv = proc_write_ipc(&bucket_ctx->ipc,
>                               output_brigade)) != APR_SUCCESS) {
> @@ -304,6 +320,7 @@ handle_request_ipc(request_rec *r, int role,
>          bucket_ctx->has_error = 1;
>          return HTTP_INTERNAL_SERVER_ERROR;
>      }
> +    apr_brigade_cleanup(output_brigade);
>
>      /* Create brigade */
>      brigade_stdout =
> @@ -522,7 +539,8 @@ handle_request(request_rec * r, int role, fcgid_cm
>  }
>
>  static int add_request_body(request_rec *r, apr_pool_t *request_pool,
> -                            apr_bucket_brigade *output_brigade)
> +                            apr_bucket_brigade *output_brigade,
> +                            fcgid_bucket_ctx *bucket_ctx)
>  {
>      apr_bucket *bucket_input, *bucket_header;
>      apr_file_t *fd = NULL;
> @@ -563,8 +581,6 @@ static int add_request_body(request_rec *r, apr_po
>              return HTTP_INTERNAL_SERVER_ERROR;
>          }
>
> -
> -
>          while ((bucket_input = APR_BRIGADE_FIRST(input_brigade)) !=
> APR_BRIGADE_SENTINEL(input_brigade)) {
>              const char *data;
>              apr_size_t len;
> @@ -615,7 +631,9 @@ static int add_request_body(request_rec *r, apr_po
>                  return HTTP_INTERNAL_SERVER_ERROR;
>              }
>
> -            if (request_size > sconf->max_mem_request_len) {
> +            if (sconf->stream_request_body) {
> +                bucket_stdin = bucket_input;
> +            } else if (request_size > sconf->max_mem_request_len) {
>                  apr_size_t wrote_len;
>                  static const char *fd_key = "fcgid_fd";
>
> @@ -701,6 +719,16 @@ static int add_request_body(request_rec *r, apr_po
>              }
>              APR_BRIGADE_INSERT_TAIL(output_brigade, bucket_header);
>              APR_BRIGADE_INSERT_TAIL(output_brigade, bucket_stdin);
> +
> +            if (sconf->stream_request_body) {
> +                /* 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;
> +                }
> +                apr_brigade_cleanup(output_brigade);
> +            }
>          }
>
>          apr_brigade_cleanup(input_brigade);
> @@ -731,7 +759,6 @@ static int add_request_body(request_rec *r, apr_po
>  int bridge_request(request_rec * r, int role, fcgid_cmd_conf *cmd_conf)
>  {
>      apr_bucket_brigade *output_brigade;
> -    apr_bucket *bucket_eos;
>      char **envp = ap_create_environment(r->pool,
>                                          r->subprocess_env);
>      int rc;
> @@ -750,17 +777,6 @@ int bridge_request(request_rec * r, int role, fcgi
>          return HTTP_INTERNAL_SERVER_ERROR;
>      }
>
> -    if (role == FCGI_RESPONDER) {
> -        rc = add_request_body(r, r->pool, output_brigade);
> -        if (rc) {
> -            return rc;
> -        }
> -    }
> -
> -    /* The eos bucket now */
> -    bucket_eos = apr_bucket_eos_create(r->connection->bucket_alloc);
> -    APR_BRIGADE_INSERT_TAIL(output_brigade, bucket_eos);
> -
>      /* Bridge the request */
>      return handle_request(r, role, cmd_conf, output_brigade);
>  }
> Index: modules/fcgid/fcgid_conf.c
> ===================================================================
> --- modules/fcgid/fcgid_conf.c (revision 1553671)
> +++ modules/fcgid/fcgid_conf.c (working copy)
> @@ -57,6 +57,7 @@
>   */
>  #define DEFAULT_MAX_REQUEST_LEN (1024*128)
>  #define DEFAULT_MAX_MEM_REQUEST_LEN (1024*64)
> +#define DEFAULT_STREAM_REQUEST_BODY 0
>  #define DEFAULT_WRAPPER_KEY "ALL"
>  #define WRAPPER_FLAG_VIRTUAL "virtual"
>
> @@ -97,6 +98,7 @@ void *create_fcgid_server_config(apr_pool_t * p, s
>      config->ipc_connect_timeout = DEFAULT_IPC_CONNECT_TIMEOUT;
>      config->max_mem_request_len = DEFAULT_MAX_MEM_REQUEST_LEN;
>      config->max_request_len = DEFAULT_MAX_REQUEST_LEN;
> +    config->stream_request_body = DEFAULT_STREAM_REQUEST_BODY;
>      config->max_requests_per_process = DEFAULT_MAX_REQUESTS_PER_PROCESS;
>      config->output_buffersize = DEFAULT_OUTPUT_BUFFERSIZE;
>      config->max_class_process_count = DEFAULT_MAX_CLASS_PROCESS_COUNT;
> @@ -158,6 +160,7 @@ void *merge_fcgid_server_config(apr_pool_t * p, vo
>      MERGE_SCALAR(base, local, merged, ipc_connect_timeout);
>      MERGE_SCALAR(base, local, merged, max_mem_request_len);
>      MERGE_SCALAR(base, local, merged, max_request_len);
> +    MERGE_SCALAR(base, local, merged, stream_request_body);
>      MERGE_SCALAR(base, local, merged, max_requests_per_process);
>      MERGE_SCALAR(base, local, merged, output_buffersize);
>      MERGE_SCALAR(base, local, merged, max_class_process_count);
> @@ -407,6 +410,17 @@ const char *set_max_mem_request_len(cmd_parms * cm
>      return NULL;
>  }
>
> +const char *set_stream_request_body(cmd_parms * cmd, void *dummy, int arg)
> +{
> +    server_rec *s = cmd->server;
> +    fcgid_server_conf *config =
> +        ap_get_module_config(s->module_config, &fcgid_module);
> +
> +    config->stream_request_body = arg;
> +    config->stream_request_body_set = 1;
> +    return NULL;
> +}
> +
>  const char *set_spawn_score(cmd_parms * cmd, void *dummy, const char *arg)
>  {
>      server_rec *s = cmd->server;
> Index: modules/fcgid/fcgid_conf.h
> ===================================================================
> --- modules/fcgid/fcgid_conf.h (revision 1553671)
> +++ modules/fcgid/fcgid_conf.h (working copy)
> @@ -91,6 +91,8 @@ typedef struct {
>      int ipc_connect_timeout_set;
>      int max_mem_request_len;
>      int max_mem_request_len_set;
> +    int stream_request_body;
> +    int stream_request_body_set;
>      apr_off_t max_request_len;
>      int max_request_len_set;
>      int max_requests_per_process;
> @@ -259,6 +261,8 @@ const char *set_php_fix_pathinfo_enable(cmd_parms
>  const char *set_max_requests_per_process(cmd_parms * cmd, void *dummy,
>                                           const char *arg);
>
> +const char *set_stream_request_body(cmd_parms * cmd, void *config, int
> arg);
> +
>  #ifdef WIN32
>  const char *set_win32_prevent_process_orphans(cmd_parms *cmd, void *dummy,
>                                                int arg);
> Index: modules/fcgid/mod_fcgid.c
> ===================================================================
> --- modules/fcgid/mod_fcgid.c (revision 1553671)
> +++ modules/fcgid/mod_fcgid.c (working copy)
> @@ -784,6 +784,10 @@ static const command_rec fcgid_cmds[] = {
>      AP_INIT_TAKE1("FcgidMaxRequestsPerProcess",
> set_max_requests_per_process,
>                    NULL, RSRC_CONF,
>                    "Max requests handled by each fastcgi application"),
> +    AP_INIT_FLAG("FcgidStreamRequestBody",
> +                 set_stream_request_body, NULL,
> +                 ACCESS_CONF | OR_FILEINFO,
> +                 "Set to 'on' to allow request body to be streamed to
> FastCGI app"),
>      AP_INIT_TAKE1("FcgidOutputBufferSize", set_output_buffersize, NULL,
>                    RSRC_CONF,
>                    "CGI output buffer size"),
>



-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Mime
View raw message