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 2016/12/15 19:06:55 UTC
Re: svn commit: r1738464 - /httpd/httpd/trunk/server/mpm/event/event.c
I have been looking at this area a little bit from the perspective of
killing the keepalive conns as early as possible during graceful
process exit.
This might have had a bad interaction with the event wakeup patch
here. Now we can sit in poll() for significant time after graceful
shutdown is signaled (immediately after the poll call that will be
EINTR'ed). This will happen before the keepalive conns are moved to
lingering close.
I am not completely sure how 2.4.23 and 2.4.24 will differ here in
practice but that is also a concern. I think something nearly as
simple as copying the 2nd block with
process_keepalive_queue(0); /* kill'em all \m/ */
Into the block where poll returned EINTR may be close to what we need.
Another unexplored option might be to refer to short poll intervals
during shutdown, from the perspective of how these two features are
disturbing eachother.
On Sun, Apr 10, 2016 at 4:35 PM, <sf...@apache.org> wrote:
> Author: sf
> Date: Sun Apr 10 20:35:18 2016
> New Revision: 1738464
>
> URL: http://svn.apache.org/viewvc?rev=1738464&view=rev
> Log:
> Terminate keep-alive connections when dying
>
> When shutting down a process gracefully, terminate keep-alive connections so
> that we don't get any new requests which may keep the dying process alive
> longer.
>
>
> Modified:
> httpd/httpd/trunk/server/mpm/event/event.c
>
> Modified: httpd/httpd/trunk/server/mpm/event/event.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1738464&r1=1738463&r2=1738464&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/event/event.c Sun Apr 10 20:35:18 2016
> @@ -2075,11 +2075,12 @@ static void * APR_THREAD_FUNC listener_t
> /* If all workers are busy, we kill older keep-alive connections so that they
> * may connect to another process.
> */
> - if (workers_were_busy && *keepalive_q->total) {
> - ap_log_error(APLOG_MARK, APLOG_TRACE1, 0, ap_server_conf,
> - "All workers are busy, will close %d keep-alive "
> - "connections",
> - *keepalive_q->total);
> + if ((workers_were_busy || dying) && *keepalive_q->total) {
> + if (!dying)
> + ap_log_error(APLOG_MARK, APLOG_TRACE1, 0, ap_server_conf,
> + "All workers are busy, will close %d keep-alive "
> + "connections",
> + *keepalive_q->total);
> process_timeout_queue(keepalive_q, 0,
> start_lingering_close_nonblocking);
> }
>
>
--
Eric Covener
covener@gmail.com
Re: svn commit: r1738464 -
/httpd/httpd/trunk/server/mpm/event/event.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
Actually, it is.
https://svn.apache.org/viewvc?view=revision&revision=1772334
> On Dec 15, 2016, at 2:47 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> On Thu, Dec 15, 2016 at 8:06 PM, Eric Covener <co...@gmail.com> wrote:
>> I have been looking at this area a little bit from the perspective of
>> killing the keepalive conns as early as possible during graceful
>> process exit.
>>
>> This might have had a bad interaction with the event wakeup patch
>> here.
>
> Hmm, the wakeup patch isn't in 2.4.24/x already no? (not proposed, by
> me at least, too much changes in events already...).
>
>> Now we can sit in poll() for significant time after graceful
>> shutdown is signaled (immediately after the poll call that will be
>> EINTR'ed). This will happen before the keepalive conns are moved to
>> lingering close.
>>
>> I am not completely sure how 2.4.23 and 2.4.24 will differ here in
>> practice but that is also a concern. I think something nearly as
>> simple as copying the 2nd block with
>>
>> process_keepalive_queue(0); /* kill'em all \m/ */
>>
>> Into the block where poll returned EINTR may be close to what we need.
>> Another unexplored option might be to refer to short poll intervals
>> during shutdown, from the perspective of how these two features are
>> disturbing eachother.
>
> If we want to kill keepalive connections early on restart/shutdown I'd rather :
>
> Index: server/mpm/event/event.c
> ===================================================================
> --- server/mpm/event/event.c (revision 1774260)
> +++ server/mpm/event/event.c (working copy)
> @@ -1865,6 +1865,7 @@ static void * APR_THREAD_FUNC listener_thread(apr_
> if (terminate_mode == ST_UNGRACEFUL
> || apr_atomic_read32(&connection_count) == 0)
> break;
> + process_keepalive_queue(0); /* kill'em all \m/ */
> }
>
> if (conns_this_child <= 0)
> _
>
> so that we don't trigger it on any possible poll() EINTR.
>
> Does it work for you?
Re: svn commit: r1738464 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Eric Covener <co...@gmail.com>.
On Thu, Dec 15, 2016 at 3:15 PM, Yann Ylavic <yl...@gmail.com> wrote:
> Ah ok, so you did not reproduced this with vanilla 2.4.x, right?
Right, I was on trunk and had just assumed this was backported (I knew
the sf stuff was).
>
> BTW, I had issues while testing both sf's changes (now in 2.4.x), the
> wakeup ones and REUSEPORT altogether, with heavy requests/gracefuls
> scenario.
Interesting. I think my very simple case is more the opposite -- low
activity in the process during shutdown that is kind of degenerate.
Re: svn commit: r1738464 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Dec 15, 2016 at 8:54 PM, Eric Covener <co...@gmail.com> wrote:
> On Thu, Dec 15, 2016 at 2:47 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> Hmm, the wakeup patch isn't in 2.4.24/x already no? (not proposed, by
>> me at least, too much changes in events already...).
>
>
> You're right, my bad. I will followup in the release thread that it's
> a red herring.
Ah ok, so you did not reproduced this with vanilla 2.4.x, right?
BTW, I had issues while testing both sf's changes (now in 2.4.x), the
wakeup ones and REUSEPORT altogether, with heavy requests/gracefuls
scenario.
But I couldn't terminate my tests and diagnostic (cleared box by a
colleague, and then lack of time).
I'll restart these tests as soon as possible, hoping that you found
the culprit already ;)
Re: svn commit: r1738464 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Eric Covener <co...@gmail.com>.
On Thu, Dec 15, 2016 at 2:47 PM, Yann Ylavic <yl...@gmail.com> wrote:
> Hmm, the wakeup patch isn't in 2.4.24/x already no? (not proposed, by
> me at least, too much changes in events already...).
You're right, my bad. I will followup in the release thread that it's
a red herring.
--
Eric Covener
covener@gmail.com
Re: svn commit: r1738464 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Eric Covener <co...@gmail.com>.
On Thu, Dec 15, 2016 at 3:07 PM, Eric Covener <co...@gmail.com> wrote:
> On Thu, Dec 15, 2016 at 2:47 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> If we want to kill keepalive connections early on restart/shutdown I'd rather :
>>
>> Index: server/mpm/event/event.c
>> ===================================================================
>> --- server/mpm/event/event.c (revision 1774260)
>> +++ server/mpm/event/event.c (working copy)
>> @@ -1865,6 +1865,7 @@ static void * APR_THREAD_FUNC listener_thread(apr_
>> if (terminate_mode == ST_UNGRACEFUL
>> || apr_atomic_read32(&connection_count) == 0)
>> break;
>> + process_keepalive_queue(0); /* kill'em all \m/ */
>> }
>>
>> if (conns_this_child <= 0)
>> _
>>
>> so that we don't trigger it on any possible poll() EINTR.
>>
>> Does it work for you?
>
> Seems to do the trick and terminate earlier / makes my temporary
> traces more sensible
I meant closes the keepalive connections earlier, not terminates the
process any earlier!
Re: svn commit: r1738464 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Eric Covener <co...@gmail.com>.
On Thu, Dec 15, 2016 at 2:47 PM, Yann Ylavic <yl...@gmail.com> wrote:
> If we want to kill keepalive connections early on restart/shutdown I'd rather :
>
> Index: server/mpm/event/event.c
> ===================================================================
> --- server/mpm/event/event.c (revision 1774260)
> +++ server/mpm/event/event.c (working copy)
> @@ -1865,6 +1865,7 @@ static void * APR_THREAD_FUNC listener_thread(apr_
> if (terminate_mode == ST_UNGRACEFUL
> || apr_atomic_read32(&connection_count) == 0)
> break;
> + process_keepalive_queue(0); /* kill'em all \m/ */
> }
>
> if (conns_this_child <= 0)
> _
>
> so that we don't trigger it on any possible poll() EINTR.
>
> Does it work for you?
Seems to do the trick and terminate earlier / makes my temporary
traces more sensible
--
Eric Covener
covener@gmail.com
Re: svn commit: r1738464 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Dec 15, 2016 at 8:06 PM, Eric Covener <co...@gmail.com> wrote:
> I have been looking at this area a little bit from the perspective of
> killing the keepalive conns as early as possible during graceful
> process exit.
>
> This might have had a bad interaction with the event wakeup patch
> here.
Hmm, the wakeup patch isn't in 2.4.24/x already no? (not proposed, by
me at least, too much changes in events already...).
> Now we can sit in poll() for significant time after graceful
> shutdown is signaled (immediately after the poll call that will be
> EINTR'ed). This will happen before the keepalive conns are moved to
> lingering close.
>
> I am not completely sure how 2.4.23 and 2.4.24 will differ here in
> practice but that is also a concern. I think something nearly as
> simple as copying the 2nd block with
>
> process_keepalive_queue(0); /* kill'em all \m/ */
>
> Into the block where poll returned EINTR may be close to what we need.
> Another unexplored option might be to refer to short poll intervals
> during shutdown, from the perspective of how these two features are
> disturbing eachother.
If we want to kill keepalive connections early on restart/shutdown I'd rather :
Index: server/mpm/event/event.c
===================================================================
--- server/mpm/event/event.c (revision 1774260)
+++ server/mpm/event/event.c (working copy)
@@ -1865,6 +1865,7 @@ static void * APR_THREAD_FUNC listener_thread(apr_
if (terminate_mode == ST_UNGRACEFUL
|| apr_atomic_read32(&connection_count) == 0)
break;
+ process_keepalive_queue(0); /* kill'em all \m/ */
}
if (conns_this_child <= 0)
_
so that we don't trigger it on any possible poll() EINTR.
Does it work for you?