You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Eric Covener <co...@gmail.com> on 2017/12/20 15:51:04 UTC
Re: svn commit: r1818804 - in /httpd/httpd/trunk: CHANGES server/mpm/event/event.c
On Wed, Dec 20, 2017 at 9:49 AM, <yl...@apache.org> wrote:
> Author: ylavic
> Date: Wed Dec 20 14:49:17 2017
> New Revision: 1818804
>
> URL: http://svn.apache.org/viewvc?rev=1818804&view=rev
> Log:
> mpm_event: close connections not reported as handled by any module.
>
> This avoids losing track of them and leaking scoreboard entries.
> PR 61551.
>
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/server/mpm/event/event.c
>
> Modified: httpd/httpd/trunk/CHANGES
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1818804&r1=1818803&r2=1818804&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Wed Dec 20 14:49:17 2017
> @@ -1,6 +1,10 @@
> -*- coding: utf-8 -*-
> Changes with Apache 2.5.1
>
> + *) mpm_event: close connections not reported as handled by any module to
> + avoid losing track of them and leaking scoreboard entries. PR 61551.
> + [Yann Ylavic]
> +
> *) mod_dumpio: do nothing below log level TRACE7. [Yann Ylavic]
>
> *) mod_md: reverses most of v1.0.5 optimization of post_config init, so that
>
> Modified: httpd/httpd/trunk/server/mpm/event/event.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1818804&r1=1818803&r2=1818804&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/event/event.c Wed Dec 20 14:49:17 2017
> @@ -1049,20 +1049,30 @@ static void process_socket(apr_thread_t
> * otherwise write, should set the sense appropriately.
> */
> apr_atomic_inc32(&clogged_count);
> - ap_run_process_connection(c);
> - if (cs->pub.state != CONN_STATE_SUSPENDED) {
> + rc = ap_run_process_connection(c);
> + if (rc || cs->pub.state != CONN_STATE_SUSPENDED) {
> cs->pub.state = CONN_STATE_LINGER;
> }
> apr_atomic_dec32(&clogged_count);
> }
> else if (cs->pub.state == CONN_STATE_READ_REQUEST_LINE) {
> read_request:
> - ap_run_process_connection(c);
> + rc = ap_run_process_connection(c);
>
> - /* state will be updated upon return
> - * fall thru to either wait for readability/timeout or
> - * do lingering close
> + /* State will be updated upon successful return: fall thru to either
> + * wait for readability/timeout, do write completion or lingering
> + * close. But forcibly close the connection if the run failed (handler
> + * raised an error for it) or the state is something unexpected at the
> + * MPM level (meaning that no module handled it and we can't do much
> + * here; note that if a handler wants mpm_event to keep POLLIN for the
> + * rest of the request line it should use CHECK_REQUEST_LINE_READABLE
> + * and not simply return OK with the initial READ_REQUEST_LINE state).
> */
> + if (rc || (cs->pub.state != CONN_STATE_CHECK_REQUEST_LINE_READABLE
> + && cs->pub.state != CONN_STATE_WRITE_COMPLETION
> + && cs->pub.state != CONN_STATE_SUSPENDED)) {
> + cs->pub.state = CONN_STATE_LINGER;
> + }
> }
is the "rc" check just covering further bases? Should we trace or set
a note here?
--
Eric Covener
covener@gmail.com
Re: svn commit: r1818804 - in /httpd/httpd/trunk: CHANGES server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Dec 20, 2017 at 6:03 PM, Eric Covener <co...@gmail.com> wrote:
> On Wed, Dec 20, 2017 at 11:28 AM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Wed, Dec 20, 2017 at 4:51 PM, Eric Covener <co...@gmail.com> wrote:
>>>
>>> is the "rc" check just covering further bases? Should we trace or set
>>> a note here?
>>
>> For me "rc" would catch at least the DECLINED case (no hook at all),
>> though it may not possibly happen with default builtin http module
>> (does --with-http=shared work?).
>> I also think that it's easier for a module which asks for the MPM to
>> finish the connection, to simply return an error in a
>> process_connection hook than something like OK + CONN_STATE_LINGER (+-
>> c->aborted = 1), .
>
> Maybe the header could be clarified one way or another.
Done (hopefully) in r1818951, does it work for you?
>
>> I'd be fine with a c->notes, but wouldn't that be useful for a
>> pre_close_connection hook only?
>> Or do you mean an ap_log_cerror() at some specific level (which I'd be
>> fine with too)?
>
> Yes, I guess notes are not so useful for such an early failure. Maybe
> ap_log_cerror() at some traceN level would be good.
Also in r1818951.
Regards,
Yann.
Re: svn commit: r1818804 - in /httpd/httpd/trunk: CHANGES server/mpm/event/event.c
Posted by Eric Covener <co...@gmail.com>.
On Wed, Dec 20, 2017 at 11:28 AM, Yann Ylavic <yl...@gmail.com> wrote:
> On Wed, Dec 20, 2017 at 4:51 PM, Eric Covener <co...@gmail.com> wrote:
>>
>> is the "rc" check just covering further bases? Should we trace or set
>> a note here?
>
> For me "rc" would catch at least the DECLINED case (no hook at all),
> though it may not possibly happen with default builtin http module
> (does --with-http=shared work?).
> I also think that it's easier for a module which asks for the MPM to
> finish the connection, to simply return an error in a
> process_connection hook than something like OK + CONN_STATE_LINGER (+-
> c->aborted = 1), .
Maybe the header could be clarified one way or another.
> I'd be fine with a c->notes, but wouldn't that be useful for a
> pre_close_connection hook only?
> Or do you mean an ap_log_cerror() at some specific level (which I'd be
> fine with too)?
Yes, I guess notes are not so useful for such an early failure. Maybe
ap_log_cerror() at some traceN level would be good.
Re: svn commit: r1818804 - in /httpd/httpd/trunk: CHANGES server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Dec 20, 2017 at 4:51 PM, Eric Covener <co...@gmail.com> wrote:
>
> is the "rc" check just covering further bases? Should we trace or set
> a note here?
For me "rc" would catch at least the DECLINED case (no hook at all),
though it may not possibly happen with default builtin http module
(does --with-http=shared work?).
I also think that it's easier for a module which asks for the MPM to
finish the connection, to simply return an error in a
process_connection hook than something like OK + CONN_STATE_LINGER (+-
c->aborted = 1), .
I'd be fine with a c->notes, but wouldn't that be useful for a
pre_close_connection hook only?
Or do you mean an ap_log_cerror() at some specific level (which I'd be
fine with too)?