Re: svn commit: r1916243 - /httpd/httpd/trunk/server/mpm/event/event.c

[Yann Ylavic]

On Tue, Mar 12, 2024 at 3:03 PM Eric Covener <cove...@gmail.com> wrote:
>
> On Tue, Mar 12, 2024 at 8:48 AM Yann Ylavic <ylavic....@gmail.com> wrote:
> >
> > Maybe it could be:
> >     if (threads_created) {
>
> not listener_started?
>
> threads_started>0  could just mean we had no scoreboard issues but
> pthread_create failed on anything but the first thread.

Don't we want the workers to gracefully stop whenever at least one was
created, or we may deadlock in clean_child_exit()?

>
> >         resource_shortage = 1;
> >         signal_threads(ST_GRACEFUL);
> >         break;
>
> I think this would have us still outer looping to keep trying to create 
> threads?

Yes, exiting start_threads() is better I think, we should not create
more threads anyway.

>
> >     }
> >     clean_child_exit(APEXIT_CHILDSICK);
>
> >> We should probably prevent the listener from starting too, like:
>
> Could be confusing, maybe we can instead dodge creating the listener
> if resource_shortage=1?

So something like the attached patch?


Regards;
Yann.

Index: server/mpm/event/event.c
===================================================================
--- server/mpm/event/event.c	(revision 1916254)
+++ server/mpm/event/event.c	(working copy)
@@ -2748,8 +2748,18 @@ static void *APR_THREAD_FUNC start_threads(apr_thr
                 ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf,
                              APLOGNO(03104)
                              "ap_thread_create: unable to create worker thread");
-                /* let the parent decide how bad this really is */
-                signal_threads(listener_started ? ST_GRACEFUL : ST_UNGRACEFUL);
+                /* Let the parent decide how bad this really is by returning
+                 * APEXIT_CHILDSICK. If threads were created already let them
+                 * stop cleanly first to avoid deadlocks in clean_child_exit(),
+                 * just stop creating new ones here (but set resource_shortage
+                 * to return APEXIT_CHILDSICK still when the child exists).
+                 */
+                if (threads_created) {
+                    resource_shortage = 1;
+                    signal_threads(ST_GRACEFUL);
+                    apr_thread_exit(thd, APR_SUCCESS);
+                    return NULL;
+                }
                 clean_child_exit(APEXIT_CHILDSICK);
             }
             threads_created++;