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
