Mailing List Archive

On Tue, Mar 12, 2024 at 1:06?PM Eric Covener <covener@gmail.com> wrote:
>
> On Mon, Mar 11, 2024 at 8:28?PM <covener@apache.org> wrote:
> >
> > Author: covener
> > Date: Tue Mar 12 00:28:34 2024
> > New Revision: 1916243
> >
> > URL: http://svn.apache.org/viewvc?rev=1916243&view=rev
> > Log:
> > use graceful exit if lister started
> >
> > 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=1916243&r1=1916242&r2=1916243&view=diff
> > ==============================================================================
> > --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> > +++ httpd/httpd/trunk/server/mpm/event/event.c Tue Mar 12 00:28:34 2024
> > @@ -2749,7 +2749,7 @@ static void *APR_THREAD_FUNC start_threa
> > APLOGNO(03104)
> > "ap_thread_create: unable to create worker thread");
> > /* let the parent decide how bad this really is */
> > - signal_threads(ST_UNGRACEFUL);
> > + signal_threads(listener_started ? ST_GRACEFUL : ST_UNGRACEFUL);
> > clean_child_exit(APEXIT_CHILDSICK);
> > }
>
> Maybe this option is silly, if we are going to nearly immediately
> clear pchild and call exit().

Maybe it could be:
if (threads_created) {
resource_shortage = 1;
signal_threads(ST_GRACEFUL);
break;
}
clean_child_exit(APEXIT_CHILDSICK);
?


Regards;
Yann.

On Tue, Mar 12, 2024 at 1:46?PM Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
> Maybe it could be:

We should probably prevent the listener from starting too, like:

> if (threads_created) {
> resource_shortage = 1;
> signal_threads(ST_GRACEFUL);
listener_started = 1; /* don't start it if not already */
> break;
> }
> clean_child_exit(APEXIT_CHILDSICK);
> ?
>
>
> Regards;
> Yann.

On Tue, Mar 12, 2024 at 8:48?AM Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
> On Tue, Mar 12, 2024 at 1:06?PM Eric Covener <covener@gmail.com> wrote:
> >
> > On Mon, Mar 11, 2024 at 8:28?PM <covener@apache.org> wrote:
> > >
> > > Author: covener
> > > Date: Tue Mar 12 00:28:34 2024
> > > New Revision: 1916243
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1916243&view=rev
> > > Log:
> > > use graceful exit if lister started
> > >
> > > 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=1916243&r1=1916242&r2=1916243&view=diff
> > > ==============================================================================
> > > --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> > > +++ httpd/httpd/trunk/server/mpm/event/event.c Tue Mar 12 00:28:34 2024
> > > @@ -2749,7 +2749,7 @@ static void *APR_THREAD_FUNC start_threa
> > > APLOGNO(03104)
> > > "ap_thread_create: unable to create worker thread");
> > > /* let the parent decide how bad this really is */
> > > - signal_threads(ST_UNGRACEFUL);
> > > + signal_threads(listener_started ? ST_GRACEFUL : ST_UNGRACEFUL);
> > > clean_child_exit(APEXIT_CHILDSICK);
> > > }
> >
> > Maybe this option is silly, if we are going to nearly immediately
> > clear pchild and call exit().
>
> 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.

> resource_shortage = 1;
> signal_threads(ST_GRACEFUL);
> break;

I think this would have us still outer looping to keep trying to create threads?

> }
> 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?

--
Eric Covener
covener@gmail.com

On Tue, Mar 12, 2024 at 3:03?PM Eric Covener <covener@gmail.com> wrote:
>
> On Tue, Mar 12, 2024 at 8:48?AM Yann Ylavic <ylavic.dev@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.

On Tue, Mar 12, 2024 at 10:19?AM Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
> On Tue, Mar 12, 2024 at 3:03?PM Eric Covener <covener@gmail.com> wrote:
> >
> > On Tue, Mar 12, 2024 at 8:48?AM Yann Ylavic <ylavic.dev@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()?

Yes, I see what you mean now. I will try to force some pthread_create
errors w/ the patch soon.

On Tue, Mar 12, 2024 at 3:30?PM Eric Covener <covener@gmail.com> wrote:
>
> On Tue, Mar 12, 2024 at 10:19?AM Yann Ylavic <ylavic.dev@gmail.com> wrote:
> >
> > On Tue, Mar 12, 2024 at 3:03?PM Eric Covener <covener@gmail.com> wrote:
> > >
> > > On Tue, Mar 12, 2024 at 8:48?AM Yann Ylavic <ylavic.dev@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()?
>
> Yes, I see what you mean now. I will try to force some pthread_create
> errors w/ the patch soon.

Possibly we want this bit too:
rv = ap_thread_create(&threads[i], thread_attr,
worker_thread, my_info, pruntime);
if (rv != APR_SUCCESS) {
+ ap_update_child_status_from_indexes(my_child_num, i,
+ SERVER_DEAD, NULL);

to restore SERVER_DEAD for the thread we could not create..

On Tue, Mar 12, 2024 at 10:30?AM Eric Covener <covener@gmail.com> wrote:
>
> On Tue, Mar 12, 2024 at 10:19?AM Yann Ylavic <ylavic.dev@gmail.com> wrote:
> >
> > On Tue, Mar 12, 2024 at 3:03?PM Eric Covener <covener@gmail.com> wrote:
> > >
> > > On Tue, Mar 12, 2024 at 8:48?AM Yann Ylavic <ylavic.dev@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()?
>
> Yes, I see what you mean now. I will try to force some pthread_create
> errors w/ the patch soon.

It seems like if there is no listener yet, the
signal_threads(ST_GRACEFUL) will fail to interrupt the queue (because
it defers to the woken up listener)
Maybe we do it inline before apr_thread_exit/



--
Eric Covener
covener@gmail.com

below + POD wakeup

Did not force the path yet where the listener is started (or fold in
the scoreboard change )

On Tue, Mar 12, 2024 at 10:54?AM Eric Covener <covener@gmail.com> wrote:
>
> On Tue, Mar 12, 2024 at 10:30?AM Eric Covener <covener@gmail.com> wrote:
> >
> > On Tue, Mar 12, 2024 at 10:19?AM Yann Ylavic <ylavic.dev@gmail.com> wrote:
> > >
> > > On Tue, Mar 12, 2024 at 3:03?PM Eric Covener <covener@gmail.com> wrote:
> > > >
> > > > On Tue, Mar 12, 2024 at 8:48?AM Yann Ylavic <ylavic.dev@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()?
> >
> > Yes, I see what you mean now. I will try to force some pthread_create
> > errors w/ the patch soon.
>
> It seems like if there is no listener yet, the
> signal_threads(ST_GRACEFUL) will fail to interrupt the queue (because
> it defers to the woken up listener)
> Maybe we do it inline before apr_thread_exit/
>
>
>
> --
> Eric Covener
> covener@gmail.com



--
Eric Covener
covener@gmail.com

I should add, this against a very divergent $bigco fork so patch may
not work well.

On Tue, Mar 12, 2024 at 11:07?AM Eric Covener <covener@gmail.com> wrote:
>
> below + POD wakeup
>
> Did not force the path yet where the listener is started (or fold in
> the scoreboard change )
>
> On Tue, Mar 12, 2024 at 10:54?AM Eric Covener <covener@gmail.com> wrote:
> >
> > On Tue, Mar 12, 2024 at 10:30?AM Eric Covener <covener@gmail.com> wrote:
> > >
> > > On Tue, Mar 12, 2024 at 10:19?AM Yann Ylavic <ylavic.dev@gmail.com> wrote:
> > > >
> > > > On Tue, Mar 12, 2024 at 3:03?PM Eric Covener <covener@gmail.com> wrote:
> > > > >
> > > > > On Tue, Mar 12, 2024 at 8:48?AM Yann Ylavic <ylavic.dev@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()?
> > >
> > > Yes, I see what you mean now. I will try to force some pthread_create
> > > errors w/ the patch soon.
> >
> > It seems like if there is no listener yet, the
> > signal_threads(ST_GRACEFUL) will fail to interrupt the queue (because
> > it defers to the woken up listener)
> > Maybe we do it inline before apr_thread_exit/
> >
> >
> >
> > --
> > Eric Covener
> > covener@gmail.com
>
>
>
> --
> Eric Covener
> covener@gmail.com



--
Eric Covener
covener@gmail.com

On Tue, Mar 12, 2024 at 4:08?PM Eric Covener <covener@gmail.com> wrote:
>
> below + POD wakeup
>
> Did not force the path yet where the listener is started (or fold in
> the scoreboard change )

+1, maybe ap_queue_term() rather than ap_queue_interrupt_all().