You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2007/06/12 22:20:39 UTC
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
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
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
Posted by Paul Querna <ch...@force-elite.com>.
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