httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: svn commit: r546328 - in /httpd/httpd/trunk: CHANGES include/httpd.h modules/http/http_core.c modules/ssl/ssl_engine_io.c server/core.c server/mpm/experimental/event/event.c
Date Tue, 12 Jun 2007 20:20:39 GMT


On 06/12/2007 02:32 AM, pquerna@apache.org wrote:
> Author: pquerna
> Date: Mon Jun 11 17:32:24 2007
> New Revision: 546328
> 
> URL: http://svn.apache.org/viewvc?view=rev&rev=546328
> Log:
> Add a clogging_input_filters variable to the conn_rec, enabling the Event MPM to know
when its running with an input filter that buffers its own data, like mod_ssl.
> 
> Modified:
>     httpd/httpd/trunk/CHANGES
>     httpd/httpd/trunk/include/httpd.h
>     httpd/httpd/trunk/modules/http/http_core.c
>     httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
>     httpd/httpd/trunk/server/core.c
>     httpd/httpd/trunk/server/mpm/experimental/event/event.c
> 

> 
> Modified: httpd/httpd/trunk/include/httpd.h
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/httpd.h?view=diff&rev=546328&r1=546327&r2=546328
> ==============================================================================
> --- httpd/httpd/trunk/include/httpd.h (original)
> +++ httpd/httpd/trunk/include/httpd.h Mon Jun 11 17:32:24 2007
> @@ -1081,6 +1081,11 @@
>      int data_in_input_filters;
>      /** Is there data pending in the output filters? */
>      int data_in_output_filters;
> +

Nitpicking and style police: Empty lines should be completely empty and should not contain
spaces.

> +    /** Are there any filters that clogg/buffer the input stream, breaking
> +     *  the event mpm.
> +     */
> +    int clogging_input_filters;
>  };

I am missing a minor bump since this is a change of the public API.

>  
>  /** 
> 
> Modified: httpd/httpd/trunk/modules/http/http_core.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_core.c?view=diff&rev=546328&r1=546327&r2=546328
> ==============================================================================
> --- httpd/httpd/trunk/modules/http/http_core.c (original)
> +++ httpd/httpd/trunk/modules/http/http_core.c Mon Jun 11 17:32:24 2007
> @@ -119,11 +119,17 @@
>      return DEFAULT_HTTP_PORT;
>  }
>  
> +static int ap_process_http_connection(conn_rec *c);
> +

Nitpicking and style: I think you should either declare a prototype at the top of the file
just before all
function implementations start or you can just move ap_process_http_connection here (which
of course
may make backports of other future changes to this file harder and thus might not be the best
solution).

>  static int ap_process_http_async_connection(conn_rec *c)
>  {
>      request_rec *r;
>      conn_state_t *cs = c->cs;
>  
> +    if (c->clogging_input_filters) {
> +        return ap_process_http_connection(c);
> +    }
> + 

Nitpicking and style police: Empty lines should be completely empty and should not contain
spaces.

>      AP_DEBUG_ASSERT(cs->state == CONN_STATE_READ_REQUEST_LINE);
>  
>      while (cs->state == CONN_STATE_READ_REQUEST_LINE) {
> 
> Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c?view=diff&rev=546328&r1=546327&r2=546328
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Mon Jun 11 17:32:24 2007
> @@ -1665,6 +1665,9 @@
>      filter_ctx->pbioWrite       = BIO_new(&bio_filter_out_method);
>      filter_ctx->pbioWrite->ptr  = (void *)bio_filter_out_ctx_new(filter_ctx, c);
>  
> +    /* We insert a clogging input filter. Let the core know. */
> +    c->clogging_input_filters = 1;
> + 

Nitpicking and style police: Empty lines should be completely empty and should not contain
spaces.

>      ssl_io_input_add_filter(filter_ctx, c, ssl);
>  
>      SSL_set_bio(ssl, filter_ctx->pbioRead, filter_ctx->pbioWrite);
> 

> Modified: httpd/httpd/trunk/server/mpm/experimental/event/event.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/experimental/event/event.c?view=diff&rev=546328&r1=546327&r2=546328
> ==============================================================================
> --- httpd/httpd/trunk/server/mpm/experimental/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/experimental/event/event.c Mon Jun 11 17:32:24 2007
> @@ -620,6 +620,16 @@
>          pt = cs->pfd.client_data;
>      }
>  
> +    if (c->clogging_input_filters && !c->aborted) {
> +        /* Since we have an input filter which 'cloggs' the input stream,
> +         * like mod_ssl, lets just do the normal read from input filters,
> +         * like the Worker MPM does.
> +         */
> +        ap_run_process_connection(c);
> +        ap_lingering_close(c);
> +        return 0;

I am not that deep into the event MPM code, but if we get into the lingering close state of
an async connection
(c->clogging_input_filters = 0 && cs->state == CONN_STATE_LINGER) we do the
following:

        ap_lingering_close(c);
        apr_pool_clear(p);
        ap_push_pool(worker_queue_info, p);
        return 1;

I fear that we will leak pools and memory if we don't clear them and push them back in the
queue.
To be honest I have no idea why we do a return 1 in this situtaion. It seems wrong to me and
should
be return 0 instead.

Another alternative would be to do

cs->state = CONN_STATE_LINGER

instead of

> +        ap_lingering_close(c);
> +        return 0;



Regards

RĂ¼diger

Mime
View raw message