Mailing-List: contact cvs-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: neutral (asf.osuosl.org: local policy)
Message-ID: <4344864B.3050301@rowe-clan.net>
Date: Wed, 05 Oct 2005 21:04:59 -0500
From: "William A. Rowe, Jr." <wrowe@rowe-clan.net>
User-Agent: Mozilla Thunderbird 1.0.7-1.1.fc3 (X11/20050929)
MIME-Version: 1.0
To: dev@httpd.apache.org
CC: cvs@httpd.apache.org
Subject: Re: svn commit: r306495 - in /httpd/httpd/trunk: include/ap_mmn.h
 include/http_core.h modules/http/http_core.c server/core.c
 server/core_filters.c
 server/protocol.c
References: <20051006012948.7408.qmail@minotaur.apache.org>
In-Reply-To: <20051006012948.7408.qmail@minotaur.apache.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Before I consider backporting for 2.1-dev, I'd very much appreciate
if some of the event mpm ap_process_http_async_connection developers
would confirm that the mechanisms behave the way I expect them to.

IIUC, the event pollset will handle the core timeout, and the
keep_alive_timeout on their own, and the common code to read-between
the request line and headers, headers and body follow the normal course
of processing as in the non-async model.

Bill

wrowe@apache.org wrote:
> Author: wrowe
> Date: Wed Oct  5 18:29:42 2005
> New Revision: 306495
> 
> URL: http://svn.apache.org/viewcvs?rev=306495&view=rev
> Log:
> 
>   NET_TIME, as a standalone feature, was a horrid idea.
> 
>   The core filter will NOT operate correctly across platforms 
>   (even between Linux/Solaris) without setting up the conn->timeout,
>   so always apply the timeout when establishing the core filter.
> 
>   The keep-alive-timeout is entirely an HTTP-ism, and needs to
>   move to the http protocol handler.  Note #1; this isn't triggered
>   in the event mpm, but the event mpm introspects s->keep_alive_timeout
>   directly adding it to the pollset, so this is a non-sequitor.
> 
>   Finally, once the headers are read, the named virtual host may
>   have a different (more/less permissive) timeout for the remainder
>   of the request body.  This http-centric patch picks up that subtle
>   detail and can switch to a named-vhost timeout.
> 
> Modified:
>     httpd/httpd/trunk/include/ap_mmn.h
>     httpd/httpd/trunk/include/http_core.h
>     httpd/httpd/trunk/modules/http/http_core.c
>     httpd/httpd/trunk/server/core.c
>     httpd/httpd/trunk/server/core_filters.c
>     httpd/httpd/trunk/server/protocol.c
> 
> Modified: httpd/httpd/trunk/include/ap_mmn.h
> URL: http://svn.apache.org/viewcvs/httpd/httpd/trunk/include/ap_mmn.h?rev=306495&r1=306494&r2=306495&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/include/ap_mmn.h (original)
> +++ httpd/httpd/trunk/include/ap_mmn.h Wed Oct  5 18:29:42 2005
> @@ -106,12 +106,13 @@
>   * 20050708.0 (2.1.7-dev) Bump MODULE_MAGIC_COOKIE to "AP22"!
>   * 20050708.1 (2.1.7-dev) add proxy request_status hook (minor)
>   * 20050919.0 (2.1.8-dev) mod_ssl ssl_ext_list optional function added
> -  */
> + * 20051005.0 (2.1.8-dev) NET_TIME filter eliminated
> + */
>  
>  #define MODULE_MAGIC_COOKIE 0x41503232UL /* "AP22" */
>  
>  #ifndef MODULE_MAGIC_NUMBER_MAJOR
> -#define MODULE_MAGIC_NUMBER_MAJOR 20050919
> +#define MODULE_MAGIC_NUMBER_MAJOR 20051005
>  #endif
>  #define MODULE_MAGIC_NUMBER_MINOR 0                     /* 0...n */
>  
> 
> Modified: httpd/httpd/trunk/include/http_core.h
> URL: http://svn.apache.org/viewcvs/httpd/httpd/trunk/include/http_core.h?rev=306495&r1=306494&r2=306495&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/include/http_core.h (original)
> +++ httpd/httpd/trunk/include/http_core.h Wed Oct  5 18:29:42 2005
> @@ -600,9 +600,6 @@
>  AP_CORE_DECLARE_NONSTD(const char *) ap_limit_section(cmd_parms *cmd, void *dummy, const char *arg);
>  
>  /* Core filters; not exported. */
> -int ap_net_time_filter(ap_filter_t *f, apr_bucket_brigade *b,
> -                       ap_input_mode_t mode, apr_read_type_e block,
> -                       apr_off_t readbytes);
>  int ap_core_input_filter(ap_filter_t *f, apr_bucket_brigade *b,
>                           ap_input_mode_t mode, apr_read_type_e block,
>                           apr_off_t readbytes);
> @@ -641,7 +638,6 @@
>  extern AP_DECLARE_DATA ap_filter_rec_t *ap_subreq_core_filter_handle;
>  extern AP_DECLARE_DATA ap_filter_rec_t *ap_core_output_filter_handle;
>  extern AP_DECLARE_DATA ap_filter_rec_t *ap_content_length_filter_handle;
> -extern AP_DECLARE_DATA ap_filter_rec_t *ap_net_time_filter_handle;
>  extern AP_DECLARE_DATA ap_filter_rec_t *ap_core_input_filter_handle;
>  
>  /**
> 
> Modified: httpd/httpd/trunk/modules/http/http_core.c
> URL: http://svn.apache.org/viewcvs/httpd/httpd/trunk/modules/http/http_core.c?rev=306495&r1=306494&r2=306495&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http/http_core.c (original)
> +++ httpd/httpd/trunk/modules/http/http_core.c Wed Oct  5 18:29:42 2005
> @@ -181,11 +181,13 @@
>   
>          if (ap_graceful_stop_signalled())
>              break;
> -        /* Go straight to select() to wait for the next request */
> +
>          if (!csd) {
>              csd = ap_get_module_config(c->conn_config, &core_module);
>          }
>          apr_socket_opt_set(csd, APR_INCOMPLETE_READ, 1);
> +        apr_socket_timeout_set(csd, c->base_server->keep_alive_timeout);
> +        /* Go straight to select() to wait for the next request */
>      }
>   
>      return OK;
> 
> Modified: httpd/httpd/trunk/server/core.c
> URL: http://svn.apache.org/viewcvs/httpd/httpd/trunk/server/core.c?rev=306495&r1=306494&r2=306495&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/core.c (original)
> +++ httpd/httpd/trunk/server/core.c Wed Oct  5 18:29:42 2005
> @@ -91,7 +91,6 @@
>  AP_DECLARE_DATA ap_filter_rec_t *ap_subreq_core_filter_handle;
>  AP_DECLARE_DATA ap_filter_rec_t *ap_core_output_filter_handle;
>  AP_DECLARE_DATA ap_filter_rec_t *ap_content_length_filter_handle;
> -AP_DECLARE_DATA ap_filter_rec_t *ap_net_time_filter_handle;
>  AP_DECLARE_DATA ap_filter_rec_t *ap_core_input_filter_handle;
>  
>  /* magic pointer for ErrorDocument xxx "default" */
> @@ -3759,10 +3758,6 @@
>      }
>      else {
>          req_cfg->bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
> -        if (!r->prev) {
> -            ap_add_input_filter_handle(ap_net_time_filter_handle,
> -                                       NULL, r, r->connection);
> -        }
>      }
>  
>      ap_set_module_config(r->request_config, &core_module, req_cfg);
> @@ -3821,10 +3816,9 @@
>  static int core_pre_connection(conn_rec *c, void *csd)
>  {
>      core_net_rec *net = apr_palloc(c->pool, sizeof(*net));
> -
> -#ifdef AP_MPM_DISABLE_NAGLE_ACCEPTED_SOCK
>      apr_status_t rv;
>  
> +#ifdef AP_MPM_DISABLE_NAGLE_ACCEPTED_SOCK
>      /* The Nagle algorithm says that we should delay sending partial
>       * packets in hopes of getting more data.  We don't want to do
>       * this; we are not telnet.  There are bad interactions between
> @@ -3841,6 +3835,20 @@
>                        "apr_socket_opt_set(APR_TCP_NODELAY)");
>      }
>  #endif
> +
> +    /* The core filter requires the timeout mode to be set, which
> +     * incidentally sets the socket to be nonblocking.  If this
> +     * is not initialized correctly, Linux - for example - will
> +     * be initially blocking, while Solaris will be non blocking
> +     * and any initial read will fail.
> +     */
> +    rv = apr_socket_timeout_set(csd, c->base_server->timeout);
> +    if (rv != APR_SUCCESS) {
> +        /* expected cause is that the client disconnected already */
> +        ap_log_cerror(APLOG_MARK, APLOG_DEBUG, rv, c,
> +                      "apr_socket_timeout_set");
> +    }
> +
>      net->c = c;
>      net->in_ctx = NULL;
>      net->out_ctx = NULL;
> @@ -3887,9 +3895,6 @@
>      ap_core_input_filter_handle =
>          ap_register_input_filter("CORE_IN", ap_core_input_filter,
>                                   NULL, AP_FTYPE_NETWORK);
> -    ap_net_time_filter_handle =
> -        ap_register_input_filter("NET_TIME", ap_net_time_filter,
> -                                 NULL, AP_FTYPE_PROTOCOL);
>      ap_content_length_filter_handle =
>          ap_register_output_filter("CONTENT_LENGTH", ap_content_length_filter,
>                                    NULL, AP_FTYPE_PROTOCOL);
> 
> Modified: httpd/httpd/trunk/server/core_filters.c
> URL: http://svn.apache.org/viewcvs/httpd/httpd/trunk/server/core_filters.c?rev=306495&r1=306494&r2=306495&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/core_filters.c (original)
> +++ httpd/httpd/trunk/server/core_filters.c Wed Oct  5 18:29:42 2005
> @@ -58,41 +58,6 @@
>  
>  #define AP_MIN_SENDFILE_BYTES           (256)
>  
> -typedef struct net_time_filter_ctx {
> -    apr_socket_t *csd;
> -    int           first_line;
> -} net_time_filter_ctx_t;
> -
> -int ap_net_time_filter(ap_filter_t *f, apr_bucket_brigade *b,
> -                       ap_input_mode_t mode, apr_read_type_e block,
> -                       apr_off_t readbytes)
> -{
> -    net_time_filter_ctx_t *ctx = f->ctx;
> -    int keptalive = f->c->keepalive == AP_CONN_KEEPALIVE;
> -
> -    if (!ctx) {
> -        f->ctx = ctx = apr_palloc(f->r->pool, sizeof(*ctx));
> -        ctx->first_line = 1;
> -        ctx->csd = ap_get_module_config(f->c->conn_config, &core_module);        
> -    }
> -
> -    if (mode != AP_MODE_INIT && mode != AP_MODE_EATCRLF) {
> -        if (ctx->first_line) {
> -            apr_socket_timeout_set(ctx->csd, 
> -                                   keptalive
> -                                      ? f->c->base_server->keep_alive_timeout
> -                                      : f->c->base_server->timeout);
> -            ctx->first_line = 0;
> -        }
> -        else {
> -            if (keptalive) {
> -                apr_socket_timeout_set(ctx->csd, f->c->base_server->timeout);
> -            }
> -        }
> -    }
> -    return ap_get_brigade(f->next, b, mode, block, readbytes);
> -}
> -
>  /**
>   * Remove all zero length buckets from the brigade.
>   */
> 
> Modified: httpd/httpd/trunk/server/protocol.c
> URL: http://svn.apache.org/viewcvs/httpd/httpd/trunk/server/protocol.c?rev=306495&r1=306494&r2=306495&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Wed Oct  5 18:29:42 2005
> @@ -832,6 +832,8 @@
>      const char *expect;
>      int access_status;
>      apr_bucket_brigade *tmp_bb;
> +    apr_socket_t *csd;
> +    apr_interval_time_t cur_timeout;
>  
>      apr_pool_create(&p, conn->pool);
>      apr_pool_tag(p, "request");
> @@ -892,6 +894,17 @@
>          return NULL;
>      }
>  
> +    /* We may have been in keep_alive_timeout mode, so toggle back
> +     * to the normal timeout mode as we fetch the header lines,
> +     * as necessary.
> +     */
> +    csd = ap_get_module_config(conn->conn_config, &core_module);
> +    apr_socket_timeout_get(csd, &cur_timeout);
> +    if (cur_timeout != conn->base_server->timeout) {
> +        apr_socket_timeout_set(csd, conn->base_server->timeout);
> +        cur_timeout = conn->base_server->timeout;
> +    }
> +
>      if (!r->assbackwards) {
>          ap_get_mime_headers_core(r, tmp_bb);
>          if (r->status != HTTP_REQUEST_TIME_OUT) {
> @@ -941,6 +954,14 @@
>       * now read. may update status.
>       */
>      ap_update_vhost_from_headers(r);
> +
> +    /* Toggle to the Host:-based vhost's timeout mode to fetch the 
> +     * request body and send the response body, if needed.
> +     */
> +    if (cur_timeout != r->server->timeout) {
> +        apr_socket_timeout_set(csd, r->server->timeout);
> +        cur_timeout = r->server->timeout;
> +    }
>  
>      /* we may have switched to another server */
>      r->per_dir_config = r->server->lookup_defaults;
> 
> 
> 
>