httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: Multi-protocol patch, part 1
Date Tue, 17 Apr 2001 06:21:29 GMT
On Mon, Apr 16, 2001 at 09:07:02PM -0700, rbb@covalent.net wrote:
>...
> What this post basically does, is it breaks part of the conn_rec out and
> into an HTTP specific structure.  I am not saying that this is everything
> that can be pulled out of the conn_rec, but it is a good first step.
> 
> Comments?

It no workee. (timeout handling; see below)

>...
> --- modules/http/http_core.c	2001/04/11 23:37:16	1.270
> +++ modules/http/http_core.c	2001/04/17 03:53:22
> @@ -295,6 +295,35 @@
>      return OK;
>  }
> 
> +static int http_create_request(request_rec *r)
> +{
> +    http_conn_rec *conn = ap_get_module_config(r->connection->conn_config, &http_module);

Nit. Please use "hconn" to distinguish this from the "old" conn. It's hard
to read, when I keep thing conn_rec.

> +    conn = apr_palloc(r->pool, sizeof(*conn));

Maybe use pcalloc here? Certainly, for future-proofing.

> +    ap_set_module_config(r->connection->conn_config, &http_module, conn);
> +
> +    if (!r->main && !(r->prev || r->next)) {
> +        conn->keptalive = r->connection->keepalive == 1;
> +        r->connection->keepalive    = 0;
> +
> +        apr_setsocketopt(r->connection->client_socket, APR_SO_TIMEOUT,
> +                         (int)(conn->keptalive
> +                         ? r->server->keep_alive_timeout * APR_USEC_PER_SEC
> +                         : r->server->timeout * APR_USEC_PER_SEC));
> +
> +        if (conn->keptalive) {
> +            apr_setsocketopt(r->connection->client_socket,
> +                             APR_SO_TIMEOUT,
> +                             (int)(r->server->timeout * APR_USEC_PER_SEC));
> +        }

Take a look at server/protocol.c, where this stuff came from. In between the
two sets of the timeout, you'll see it goes to the socket to read the
request line. Basically, it sets the timeout to <something>, reads a line,
then resets it to <something>.

In the above code, it simply sets it a couple times and exits. Not very
effective :-)

> +        conn->keptalive = 0;

See note below about this variable.

>...
> --- modules/http/mod_core.h	2001/02/28 15:24:07	1.6
> +++ modules/http/mod_core.h	2001/04/17 03:53:23

Hrm. It would be nice to rename this to mod_http.h before we get too
attached to the mod_core.h name.  (totally unrelated to this patch, but just
raising it while I'm thinking of it)

> @@ -70,6 +70,15 @@
>   * @package mod_core private header file
>   */
> 
> +typedef struct http_conn_rec http_conn_rec;

If you're introducing a new structure, then please namespace-protect it. No
sense digging a deeper hole :-)

> +struct http_conn_rec {
> +    /** Did we use HTTP Keep-Alive? */
> +    unsigned keptalive:1;

See below about this variable.

However, please note that we can/should simply toss the darned bit fields
from conn_rec, and certainly this one.

>...
> --- modules/loggers/mod_log_config.c	2001/03/09 20:30:32	1.52
> +++ modules/loggers/mod_log_config.c	2001/04/17 03:53:23
> @@ -191,6 +191,7 @@
>  #include "http_core.h"          /* For REMOTE_NAME */
>  #include "http_log.h"
>  #include "http_protocol.h"
> +#include "mod_core.h"
> 
>  #if APR_HAVE_UNISTD_H
>  #include <unistd.h>
> @@ -527,13 +528,19 @@
>  }
>  static const char *log_connection_status(request_rec *r, char *a)
>  {
> +#ifdef AP_HTTP_ENABLED
> +    http_conn_rec *hconn = ap_get_module_config(r->connection->conn_config,
> +                                                &http_module);
> +#endif

Oof! It would be nice to keep the http_conn_rec private. This kinda sucks.

Maybe for another day, though.

>...
> --- server/protocol.c	2001/04/11 23:37:16	1.12
> +++ server/protocol.c	2001/04/17 03:53:26
> @@ -556,9 +556,6 @@
>      r->connection      = conn;
>      r->server          = conn->base_server;
> 
> -    conn->keptalive    = conn->keepalive == 1;

Okay... about conn->keptalive.

I just searched the whole httpd-2.0 repository. This value is defined in the
conn_rec, but used *only* in this function.

Rather than shift it from conn_rec to http_conn_rec, let's just switch it to
a local variable here(!)

> -    conn->keepalive    = 0;
> -
>      r->user            = NULL;
>      r->ap_auth_type    = NULL;
> 
> @@ -584,11 +581,6 @@
>      r->output_filters  = conn->output_filters;
>      r->input_filters   = conn->input_filters;
> 
> -    apr_setsocketopt(conn->client_socket, APR_SO_TIMEOUT,
> -                     (int)(conn->keptalive
> -                     ? r->server->keep_alive_timeout * APR_USEC_PER_SEC
> -                     : r->server->timeout * APR_USEC_PER_SEC));
> -
>      ap_add_output_filter("BYTERANGE", NULL, r, r->connection);
>      ap_add_output_filter("CONTENT_LENGTH", NULL, r, r->connection);
>      ap_add_output_filter("HTTP_HEADER", NULL, r, r->connection);
> @@ -604,10 +596,6 @@
>          }
>          return NULL;
>      }
> -    if (r->connection->keptalive) {

This is a longer way of saying conn->keptalive. But that is a long way of
referring the local variable "keptalive".

> -        apr_setsocketopt(r->connection->client_socket, APR_SO_TIMEOUT,
> -                         (int)(r->server->timeout * APR_USEC_PER_SEC));

This little block of code says, "it we set the timeout to something other
than server->timeout (above), then set it back."

I'm not sure that we can extract this to mod_http. Maybe conn->keepalives,
but I think this patch needs a bit of rethink, regarding this timeout
handling code.

> -    }
>      if (!r->assbackwards) {
>          get_mime_headers(r);
>          if (r->status != HTTP_REQUEST_TIME_OUT) {
> @@ -645,8 +633,6 @@
> 
>      /* we may have switched to another server */
>      r->per_dir_config = r->server->lookup_defaults;
> -
> -    conn->keptalive = 0;        /* We now have a request to play with */

Local var again.


There may be some other review comments, but I concentrated mostly trying to
describe the above issue w.r.t timeouts and stuff. Happy to review an
updated patch, of course!

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Mime
View raw message