httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Querna <c...@force-elite.com>
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 Wed, 13 Jun 2007 02:41:36 GMT
Ruediger Pluem wrote:
>> 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.

Fixed all of the empty line issues in r546632.

>> +    /** 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.

Added minor bump to trunk in r546650.


>>  
>> +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).

Yes, I was trying to avoid moving functions around.  I've moved the decl
up to the top in r546632.

>> 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;

Here is what the return value does:
        rv = process_socket(ptrans, csd, cs, process_slot, thread_slot);
        if (!rv) {
            requests_this_child--;
        }

It is a bad system, and all it does is keep the stats right for the
number of connections handled.

Return 1 means the client was not completely handled, and that the
socket is still being 'worked on' by someone.

Return 0 means its completely done.

And yes, requests_this_child counts down, because it is crazy.


> 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

I've changed to this in r546715.

Thanks Again,

-Paul


Mime
View raw message