Re: svn commit: r1899777 - /httpd/httpd/trunk/server/mpm/event/event.c
On 4/12/22 2:08 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Tue Apr 12 12:08:02 2022
> New Revision: 1899777
>
> URL: http://svn.apache.org/viewvc?rev=1899777&view=rev
> Log:
> mpm_event: Fix accounting of active/total processes on ungraceful restart.
>
> Children processes terminated by ap_{reclaim,relieve}_child_processes() were
> were not un-accounted for total_daemons and active_daemons, which was done in
> server_main_loop() only. This led to perform_idle_server_maintenance()
> thinking
> it was over the limit of children processes and never create new ones.
>
> Have this accounting right in event_note_child_{started,stopped}() which is
> called both at runtime and reload time.
>
> * server/mpm/event/event.c(struct event_retained_data):
> Rename field max_daemons_limit to max_daemon_used to better describe what
> it's about and to align with AP_MPMQ_MAX_DAEMON_USED.
>
> * server/mpm/event/event.c(event_note_child_stopped):
> Renamed from event_note_child_killed() to clarify that it's not only called
> when a child is killed (i.e. on restart) but whenever a child has stopped.
>
> * server/mpm/event/event.c(event_note_child_stopped):
> Move decrementing {active,total}_daemons and marking child's threads as
> SERVER_DEAD from server_main_loop() so that it's done both at runtime and
> reload time. Log the current number/state of daemons at APLOG_DEBUG level
> for each child stopped.
>
> * server/mpm/event/event.c(event_note_child_started):
> Move incrementing {active,total}_daemons from make_child() for symmetry,
> given that make_child() calls event_note_child_started(). Log the current
> number/state of daemons at APLOG_DEBUG level for each child started.
>
> * server/mpm/event/event.c(perform_idle_server_maintenance):
> Fix possible miscounting of retained->max_daemon_used accross the multiple
> calls to perform_idle_server_maintenance() if ListenCoresBucketsRatio > 0.
> Pass an int *max_daemon_used which starts at zero and is bumped consistently
> for all the buckets, while retained->max_daemon_used is updated only after
> all the buckets have been maintained.
>
> * server/mpm/event/event.c(perform_idle_server_maintenance):
> Use event_note_child_stopped() to handle exited children processes.
>
>
> Fixes: BZ 66004
>
>
> 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=1899777&r1=1899776&r2=1899777&view=diff
> ==
> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/event/event.c Tue Apr 12 12:08:02 2022
> @@ -3447,9 +3480,11 @@ static void server_main_loop(int remaini
> continue;
> }
>
> +max_daemon_used = 0;
> for (i = 0; i < num_buckets; i++) {
> -perform_idle_server_maintenance(i);
> +perform_idle_server_maintenance(i, &max_daemon_used);
> }
> +retained->max_daemon_used = max_daemon_used;
Shouldn't we do the above only when max_daemon_used > retained->max_daemon_used
? Otherwise we might shrink
retained->max_daemon_used if make_child increased it?
Regards
Rüdiger
Faster start children after fatal signals?
While looking at PR65769 I stumbled across the below in event.c (same in worker.c) 3460/* Don't perform idle maintenance when a child dies, 3461 * only do it when there's a timeout. Remember only a 3462 * finite number of children can die, and it's pretty 3463 * pathological for a lot to die suddenly. 3464 */ 3465continue; In case several processes or even all die by a segfault we would wait until we have processed all that processes plus one second until we start new processes. Shouldn't we perform an perform_idle_server_maintenance in case the process died because of a fatal signal? Regards Rüdiger
Re: svn commit: r1899648 - in /httpd/httpd/trunk: changes-entries/core_response_buckets.txt include/mod_core.h modules/http/http_core.c modules/http/http_filters.c modules/http/http_protocol.c server/
On 4/7/22 12:41 PM, ic...@apache.org wrote:
> Author: icing
> Date: Thu Apr 7 10:41:46 2022
> New Revision: 1899648
>
> URL: http://svn.apache.org/viewvc?rev=1899648&view=rev
> Log:
> *) core/mod_http: use RESPONSE meta buckets and a new HTTP/1.x specific
> filter to send responses through the output filter chain.
> Specifically: the HTTP_HEADER output filter and
> ap_send_interim_response()
> create a RESPONSE bucket and no longer are concerned with HTTP/1.x
> serialization.
> A new HTTP1_RESPONSE_OUT transcode filter writes the proper HTTP/1.x
> bytes when dealing with a RESPONSE bucket. That filter installs itself
> on the pre_read_request hook when the connection has protocol 'http/1.1'.
>
>
> Added:
> httpd/httpd/trunk/changes-entries/core_response_buckets.txt
> Modified:
> httpd/httpd/trunk/include/mod_core.h
> httpd/httpd/trunk/modules/http/http_core.c
> httpd/httpd/trunk/modules/http/http_filters.c
> httpd/httpd/trunk/modules/http/http_protocol.c
> httpd/httpd/trunk/server/protocol.c
>
> Modified: httpd/httpd/trunk/modules/http/http_filters.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1899648&r1=1899647&r2=1899648&view=diff
> ==
> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
> +++ httpd/httpd/trunk/modules/http/http_filters.c Thu Apr 7 10:41:46 2022
> @@ -1364,62 +1178,124 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
> return rv;
> }
> }
> -
> -for (e = APR_BRIGADE_FIRST(b);
> - e != APR_BRIGADE_SENTINEL(b);
> - e = APR_BUCKET_NEXT(e))
> -{
> -if (AP_BUCKET_IS_ERROR(e) && !eb) {
> -eb = e->data;
> -continue;
> -}
> -if (APR_BUCKET_IS_EOS(e)) {
> -if (!eos) eos = e;
> -continue;
> -}
> -/*
> - * If we see an EOC bucket it is a signal that we should get out
> - * of the way doing nothing.
> +else {
> +/* Determine if it is time to insert the response bucket for
> + * the request. Request handlers just write content or an EOS
> + * and that needs to take the current state of request_rec to
> + * send on status and headers as a response bucket.
> + *
> + * But we also send interim responses (as response buckets)
> + * through this filter and that must not trigger generating
> + * an additional response bucket.
> + *
> + * Waiting on a DATA/ERROR/EOS bucket alone is not enough,
> + * unfortunately, as some handlers trigger response generation
> + * by just writing a FLUSH (see mod_lua's websocket for example).
> */
> -if (AP_BUCKET_IS_EOC(e)) {
> -ap_remove_output_filter(f);
> -return ap_pass_brigade(f->next, b);
> +for (e = APR_BRIGADE_FIRST(b);
> + e != APR_BRIGADE_SENTINEL(b) && !trigger;
> + e = APR_BUCKET_NEXT(e))
> +{
> +if (AP_BUCKET_IS_RESPONSE(e)) {
> +/* remember the last one if there are many. */
> +respb = e;
> +}
> +else if (APR_BUCKET_IS_FLUSH(e)) {
> +/* flush without response bucket triggers */
> +if (!respb) trigger = e;
> +}
> +else if (APR_BUCKET_IS_EOS(e)) {
> +trigger = e;
> +}
> +else if (AP_BUCKET_IS_ERROR(e)) {
> +/* Need to handle this below via ap_die() */
> +break;
> +}
> +else {
> +/* First content bucket, always triggering the response.*/
> +trigger = e;
> +}
Why don't we remove ourselves any longer if we hit an EOC bucket and pass stuff
on immediately?
If we want to handle this HTTP protocol agnostic here we need to ensure that
the EOC bucket
triggers the immediate removal of the ap_h1_response_out_filter once the
ap_h1_response_out_filter
sees it without putting any headers on the wire.
I am not sure what the appropriate behaviour would be in the HTTP > 1.1 case.
> }
> -}
>
> -if (!ctx->headers_sent && !check_headers(r)) {
> -/* We may come back here from ap_die() below,
> - * so clear anything from this response.
> - */
> -apr_table_clear(r->headers_out);
> -apr_table_clear(r->err_headers_out);
> -apr_brigade_cleanup(b);
> +if (respb) {
> +ap_bucket_response *resp = respb->data;
> +if (resp->status >= 200 || resp->status == 101) {
Why using the literal 101 instead of HTTP_SWITCHING_PROTOCOLS?
> +/* Someone is passing the final response, remember it
> + * so we no longer generate one. */
> +ctx->final_status = resp->status;
>
Re: svn commit: r1899777 - /httpd/httpd/trunk/server/mpm/event/event.c
On Wed, Apr 13, 2022 at 4:22 PM Ruediger Pluem wrote:
>
> On 4/12/22 2:08 PM, yla...@apache.org wrote:
>
> > @@ -3447,9 +3480,11 @@ static void server_main_loop(int remaini
> > continue;
> > }
> >
> > +max_daemon_used = 0;
> > for (i = 0; i < num_buckets; i++) {
> > -perform_idle_server_maintenance(i);
> > +perform_idle_server_maintenance(i, &max_daemon_used);
> > }
> > +retained->max_daemon_used = max_daemon_used;
>
> Shouldn't we do the above only when max_daemon_used >
> retained->max_daemon_used ? Otherwise we might shrink
> retained->max_daemon_used if make_child increased it?
You are right that retained->max_daemon_used can be shrinked with
r1899777, it should be better now with the follow up r1899812.
But here we need to set it unconditionally (not only when <
max_daemon_used) because retained->max_daemon_used needs to decrease
too when "high" children are stopped (e.g. MaxSpareThreads), it can't
increase forever.
Regards;
Yann.
Re: Faster start children after fatal signals?
On Wed, Apr 13, 2022 at 4:30 PM Ruediger Pluem wrote: > > While looking at PR65769 I stumbled across the below in event.c (same in > worker.c) > > 3460/* Don't perform idle maintenance when a child dies, > 3461 * only do it when there's a timeout. Remember only a > 3462 * finite number of children can die, and it's pretty > 3463 * pathological for a lot to die suddenly. > 3464 */ > 3465continue; > > In case several processes or even all die by a segfault we would wait until > we have processed all that processes plus one second > until we start new processes. Shouldn't we perform an > perform_idle_server_maintenance in case the process died because of a fatal > signal? +1
