You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jeff Trawick <tr...@bellsouth.net> on 2001/04/24 16:50:17 UTC
[PATCH] get threaded MPM to terminate
With the level of code in CVS, SIGTERM wakes up the main thread in
each child. The main thread then enters pthread_join() to wait for a
worker thread to complete but nothing ever happens because there is
nothing to wake up a worker thread.
This patch moves the workers_may_exit field to the scoreboard and has
the parent process set this field and bombard the pipe of death before
sending SIGTERM to the child processes.
The workers_may_exit field is Paul's new life_status field. Since
this field applies to the entire child process it was moved to a
different place in the scoreboard and prefork.c was updated as
appropriate.
Try to ignore the mpm_trace() stuff in the patch...
Anything good in the following patch is from Greg Ames. The rest is
mine.
Comments?
Index: include/scoreboard.h
===================================================================
RCS file: /home/cvspublic/httpd-2.0/include/scoreboard.h,v
retrieving revision 1.17
diff -u -r1.17 scoreboard.h
--- include/scoreboard.h 2001/04/23 23:14:35 1.17
+++ include/scoreboard.h 2001/04/24 14:13:05
@@ -1,4 +1,3 @@
-
/* ====================================================================
* The Apache Software License, Version 1.1
*
@@ -152,6 +151,9 @@
#define SB_WORKING 0 /* The server is busy and the child is useful. */
#define SB_IDLE_DIE 1 /* The server is idle and the child is superfluous. */
/* The child should check for this and exit gracefully. */
+#define SB_TERM_DIE 2 /* The child is going away on the whim of the parent. */
+#define SB_MAXREQ_DIE 3 /* This child has served enough requests. */
+#define SB_FATAL_DIE 4 /* Some fatal error has occurred. */
/* stuff which is thread/process specific */
typedef struct {
@@ -170,7 +172,6 @@
unsigned long my_bytes_served;
unsigned long conn_bytes;
unsigned short conn_count;
- unsigned short life_status; /* Either SB_WORKING or SB_IDLE_DIE */
apr_time_t start_time;
apr_time_t stop_time;
#ifdef HAVE_TIMES
@@ -191,11 +192,15 @@
* should still be serving requests. */
} global_score;
-/* stuff which the parent generally writes and the children rarely read */
+/* stuff which (mostly) the parent generally writes and the children rarely read
+ * exception: life_status is written to by both parent and children and is
+ * checked often by children
+ */
typedef struct {
pid_t pid;
ap_generation_t generation; /* generation of this child */
int worker_threads;
+ unsigned short life_status; /* Either SB_WORKING or SB_IDLE_DIE */
#ifdef OPTIMIZE_TIMEOUTS
time_t last_rtime; /* time(0) of the last change */
vtime_t last_vtime; /* the last vtime the parent has seen */
Index: server/mpm/prefork/prefork.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/mpm/prefork/prefork.c,v
retrieving revision 1.173
diff -u -r1.173 prefork.c
--- server/mpm/prefork/prefork.c 2001/04/13 19:00:38 1.173
+++ server/mpm/prefork/prefork.c 2001/04/24 14:13:10
@@ -232,7 +232,7 @@
if (pchild) {
apr_pool_destroy(pchild);
}
- ap_scoreboard_image->servers[my_child_num][0].life_status = SB_WORKING;
+ ap_scoreboard_image->parent[my_child_num].life_status = SB_WORKING;
chdir_for_gprof();
exit(code);
}
@@ -373,7 +373,7 @@
static void please_die_gracefully(int sig)
{
/* clean_child_exit(0); */
- ap_scoreboard_image->servers[my_child_num][0].life_status = SB_IDLE_DIE;
+ ap_scoreboard_image->parent[my_child_num].life_status = SB_IDLE_DIE;
if (sig == SIGHUP) {
(void) ap_update_child_status(AP_CHILD_THREAD_FROM_ID(my_child_num),
SERVER_GRACEFUL, (request_rec *) NULL);
@@ -529,7 +529,7 @@
static fd_set main_fds;
#define I_AM_TO_SHUTDOWN() \
-(ap_scoreboard_image->servers[my_child_num][0].life_status != SB_WORKING)
+(ap_scoreboard_image->parent[my_child_num].life_status != SB_WORKING)
int ap_graceful_stop_signalled(void)
{
@@ -840,7 +840,7 @@
apr_signal(SIGQUIT, SIG_DFL);
#endif
apr_signal(SIGTERM, please_die_gracefully);
- ap_scoreboard_image->servers[slot][0].life_status = SB_WORKING;
+ ap_scoreboard_image->parent[slot].life_status = SB_WORKING;
child_main(slot);
}
@@ -891,7 +891,7 @@
apr_signal(SIGHUP, please_die_gracefully);
apr_signal(SIGWINCH, please_die_gracefully);
apr_signal(SIGTERM, please_die_gracefully);
- ap_scoreboard_image->servers[slot][0].life_status = SB_WORKING;
+ ap_scoreboard_image->parent[slot].life_status = SB_WORKING;
child_main(slot);
}
@@ -1254,7 +1254,7 @@
update_scoreboard_global();
for (index = 0; index < ap_daemons_limit; ++index) {
- ap_scoreboard_image->servers[index][0].life_status = SB_IDLE_DIE;
+ ap_scoreboard_image->parent[index].life_status = SB_IDLE_DIE;
}
if (is_graceful) {
Index: server/mpm/threaded/threaded.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/mpm/threaded/threaded.c,v
retrieving revision 1.26
diff -u -r1.26 threaded.c
--- server/mpm/threaded/threaded.c 2001/04/23 23:14:35 1.26
+++ server/mpm/threaded/threaded.c 2001/04/24 14:13:13
@@ -105,11 +105,17 @@
static int min_spare_threads=0;
static int max_spare_threads=0;
static int ap_daemons_limit=0;
-static int workers_may_exit = 0;
static int requests_this_child;
static int num_listensocks = 0;
static apr_socket_t **listensocks;
+static int my_child_num;
+#define CHK_WORKERS_MAY_EXIT() \
+(ap_scoreboard_image->parent[my_child_num].life_status != SB_WORKING && \
+ mpm_trace("workers should exit now"))
+#define SET_WORKERS_MAY_EXIT(reason) \
+ap_scoreboard_image->parent[my_child_num].life_status = (reason)
+
/* The structure used to pass unique initialization info to each thread */
typedef struct {
int pid;
@@ -171,6 +177,13 @@
#define SAFE_ACCEPT(stmt) (stmt)
#endif
+static int mpm_trace(const char *s)
+{
+ ap_log_error(APLOG_MARK, APLOG_EMERG, 0, ap_server_conf, "thread %d/%ld: %s",
+ ap_my_pid, (long)pthread_self(), s);
+ return 1;
+}
+
AP_DECLARE(apr_status_t) ap_mpm_query(int query_code, int *result)
{
switch(query_code){
@@ -284,6 +297,7 @@
static void sig_term(int sig)
{
+ mpm_trace("got SIGTERM");
ap_start_shutdown();
}
@@ -435,11 +449,12 @@
ap_lingering_close(current_conn);
}
}
-/* Sets workers_may_exit if we received a character on the pipe_of_death */
+/* Sets WORKERS_MAY_EXIT if we received a character on the pipe_of_death */
static void check_pipe_of_death(void)
{
+ mpm_trace("got data on pipe of death");
apr_lock_acquire(pipe_of_death_mutex);
- if (!workers_may_exit) {
+ if (!CHK_WORKERS_MAY_EXIT()) {
apr_status_t ret;
char pipe_read_char;
apr_size_t n = 1;
@@ -452,7 +467,7 @@
else {
/* It won the lottery (or something else is very
* wrong). Embrace death with open arms. */
- workers_may_exit = 1;
+ SET_WORKERS_MAY_EXIT(SB_TERM_DIE);
}
}
apr_lock_release(pipe_of_death_mutex);
@@ -487,8 +502,9 @@
/* TODO: Switch to a system where threads reuse the results from earlier
poll calls - manoj */
while (1) {
- workers_may_exit |= (ap_max_requests_per_child != 0) && (requests_this_child <= 0);
- if (workers_may_exit) break;
+ if ((ap_max_requests_per_child != 0) && (requests_this_child <= 0))
+ SET_WORKERS_MAY_EXIT(SB_MAXREQ_DIE);
+ if (CHK_WORKERS_MAY_EXIT()) break;
(void) ap_update_child_status(process_slot, thread_slot, SERVER_READY,
(request_rec *) NULL);
@@ -497,10 +513,10 @@
ap_log_error(APLOG_MARK, APLOG_EMERG, rv, ap_server_conf,
"apr_lock_acquire failed. Attempting to shutdown "
"process gracefully.");
- workers_may_exit = 1;
+ SET_WORKERS_MAY_EXIT(SB_FATAL_DIE);
}
- while (!workers_may_exit) {
+ while (!CHK_WORKERS_MAY_EXIT()) {
apr_status_t ret;
apr_int16_t event;
@@ -514,10 +530,10 @@
* circumstances. Let's try exiting gracefully, for now. */
ap_log_error(APLOG_MARK, APLOG_ERR, ret, (const server_rec *)
ap_server_conf, "apr_poll: (listen)");
- workers_may_exit = 1;
+ SET_WORKERS_MAY_EXIT(SB_FATAL_DIE);
}
- if (workers_may_exit) break;
+ if (CHK_WORKERS_MAY_EXIT()) break;
apr_poll_revents_get(&event, listensocks[0], pollset);
if (event & APR_POLLIN) {
@@ -550,7 +566,7 @@
}
}
got_fd:
- if (!workers_may_exit) {
+ if (!CHK_WORKERS_MAY_EXIT()) {
if ((rv = apr_accept(&csd, sd, ptrans)) != APR_SUCCESS) {
csd = NULL;
ap_log_error(APLOG_MARK, APLOG_ERR, rv, ap_server_conf,
@@ -561,7 +577,7 @@
ap_log_error(APLOG_MARK, APLOG_EMERG, rv, ap_server_conf,
"apr_lock_release failed. Attempting to shutdown "
"process gracefully.");
- workers_may_exit = 1;
+ SET_WORKERS_MAY_EXIT(SB_FATAL_DIE);
}
if (csd != NULL) {
process_socket(ptrans, csd, process_slot, thread_slot);
@@ -574,7 +590,7 @@
ap_log_error(APLOG_MARK, APLOG_EMERG, rv, ap_server_conf,
"apr_lock_release failed. Attempting to shutdown "
"process gracefully.");
- workers_may_exit = 1;
+ SET_WORKERS_MAY_EXIT(SB_FATAL_DIE);
}
break;
}
@@ -611,12 +627,11 @@
apr_thread_t **threads;
apr_threadattr_t *thread_attr;
int i;
- int my_child_num = child_num_arg;
proc_info *my_info = NULL;
ap_listen_rec *lr;
apr_status_t rv;
-
+ my_child_num = child_num_arg;
ap_my_pid = getpid();
apr_pool_create(&pchild, pconf);
@@ -1073,11 +1088,35 @@
restart_pending = shutdown_pending = 0;
server_main_loop(remaining_children_to_start);
-
+ mpm_trace("back from server_main_loop");
if (shutdown_pending) {
+ int i;
+ char char_of_death = '!';
+
/* Time to gracefully shut down:
* Kill child processes, tell them to call child_exit, etc...
*/
+ /* sequence of operations:
+ * 1) set the workers_may_exit flag for each child process
+ * 2) write data on the pipe of death so that the worker threads
+ * are awakened
+ * 3) send SIGTERM to the main threads (blocked in sigwait()) so they
+ * wake up and join up with the exited worker threads
+ */
+ mpm_trace("setting workers_may_exit");
+ for (i = 0; i < ap_daemons_limit; ++i) {
+ ap_scoreboard_image->parent[i].life_status = SB_TERM_DIE;
+ }
+ /* give the children the signal to die */
+ mpm_trace("sending pipe of death signals");
+ for (i = 0; i < ap_daemons_limit;) {
+ if ((rv = apr_file_write(pipe_of_death_out, &char_of_death, &one)) != APR_SUCCESS) {
+ if (APR_STATUS_IS_EINTR(rv)) continue;
+ ap_log_error(APLOG_MARK, APLOG_WARNING, rv, ap_server_conf, "write pipe_of_death");
+ }
+ i++;
+ }
+ mpm_trace("sending SIGTERM");
if (unixd_killpg(getpgrp(), SIGTERM) < 0) {
ap_log_error(APLOG_MARK, APLOG_WARNING, errno, ap_server_conf, "killpg SIGTERM");
}
--
Jeff Trawick | trawickj@bellsouth.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] get threaded MPM to terminate
Posted by Greg Ames <gr...@remulak.net>.
Greg Ames wrote:
>
>
> Let's say we just want to shut down. Assume one thread in one process
> is sitting in apr_pool, waiting for an accept() or the pipe of death.
duh! s/apr_pool/apr_poll/
Re: [PATCH] get threaded MPM to terminate
Posted by rb...@covalent.net.
On Tue, 24 Apr 2001, Greg Ames wrote:
> rbb@covalent.net wrote:
> > Not writing to the pipe_of_death is just a bug.
>
> Agreed. Design bug or code bug? (not that it matters)
code.
> > which is why I am questioning that the algorithm is really wrong, I
> > believe it is a bug, not a logic error.
>
> what's the difference? does anybody care?
There is a big difference. a bug is a problem with the implementation, a
logic error is a problem in the design.
> > Can't this whole thing be solved by having the process that was woken up
> > by the pipe_of_death set workers_may_exit before releasing the poll lock?
>
> I know this patch works, even if the parent only sent one character on
> the pipe of death.
>
> What I haven't pondered very much is the loop sending a character per
> process. I don't know if that's absolutely air tight. It may be.
>
> > BTW, if the parent is signalling a graceful shutdown, the child process's
> > signal handler should never be awoken until all the other threads are
> > gone.
>
> hmmm...how can you tell for sure when all the worker threads are gone,
> unless you are the signal thread and you do a pthread join?
The easiest way, is to have the process itself RAISE the signal, instead
of making the parent process send the signal. Yes, this gets the signal
to the child process before all the threads are gone, but it does keep the
parent out of the loop.
> > The problem I have with this, is that workers_may_exit is specifically a
> > local variable.
>
> yeah, but we're programmers, and we have the technology to change that.
No, you miss my meaning. I'm not saying that currently workers_may_exit
is a local variable, I am saying that it should ALWAYS be a local
variable. The whole point of the variable is that the child process
controls how it shuts itself down. By exposing that variable, you are
giving the parent control over how the child shuts down.
Think of it this way, there are two stages to shutting down the child:
1) notifying the child that it should shutdown
2) actually shutting down.
Once the parent does #1, through the pipe of death, it is up to the child
to determine how best to shutdown.
> > The parent process should not be touching it. It is up
> > to the process how it is going to go away.
>
> Which process decides whether the user wants a graceful shutdown vs.
> quick -n- dirty shutdown vs. a restart?
That is a part of the notifcation. Workers_may_exit is not a part of the
notification, it is a part of the how. Once the child process is told to
die, either gracefully or immediately, the parent should be completely out
of the loop.
What this patch is basically saying, is that the a threaded child process
does not know how to take itself down cleanly. That is bogus. It means
that when MaxRequestsPerChild is hit, we have to do somersaults to
shutdown cleanly.
> > The fact that this patch moves
> > it to the parent's job to set workers_may_exit is incorrect.
>
> I respect your opinion, but disagree. This patch works, and it has a
> really nice side benefit, as well.
>
> I think it's great that those death codes are in the scoreboard for
> admin reasons as well as for sealing up the timing windows. mod_status
> could display them, and the admin would have a better view of what
> Apache is up to internally in our long running download scenarios, for
> example. The ability for admins to see this kind of information will be
> important for the success of any threaded mpm.
What this patch does, is introduce yet another path for the child
processes to shutdown. It separates how a child process dies for
MaxRequestsPerChild from how it dies for a graceful restart. In one case,
the parent process sets a value in shared memory, in the other the threads
determine that they should die.
What should happen, is that in both cases, the child process is notified
(either by a byte from a pipe, or an if condition) that it should die, the
child process sets a local variable to inform the rest of the threads, and
the threads start to die.
By introducing the parent into the how a child process is going to die,
you have made things more complex, and added to the amount of shared
memory required. And, I still haven't seen a good explanation of why the
current design won't work. I do know that Manoj spent about three weeks
removing the race conditions from the original code.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] get threaded MPM to terminate
Posted by Greg Ames <gr...@remulak.net>.
rbb@covalent.net wrote:
>
>
> Not writing to the pipe_of_death is just a bug.
Agreed. Design bug or code bug? (not that it matters)
>
>
> This is the problem with the current logic. Remember, that Manoj and I
> spent a LOT of time
Yes. Thank you both. It was a very good starting point, it was close
to being right, and the Pipe of Death was an elegant and necessary
invention IMO.
> getting this right, and it worked for a very long time,
huh? it had timing windows for a long time. Now it's time to move on,
and eliminate them once and for all.
> which is why I am questioning that the algorithm is really wrong, I
> believe it is a bug, not a logic error.
what's the difference? does anybody care?
>
> Can't this whole thing be solved by having the process that was woken up
> by the pipe_of_death set workers_may_exit before releasing the poll lock?
>
I know this patch works, even if the parent only sent one character on
the pipe of death.
What I haven't pondered very much is the loop sending a character per
process. I don't know if that's absolutely air tight. It may be.
> BTW, if the parent is signalling a graceful shutdown, the child process's
> signal handler should never be awoken until all the other threads are
> gone.
hmmm...how can you tell for sure when all the worker threads are gone,
unless you are the signal thread and you do a pthread join?
>
> The problem I have with this, is that workers_may_exit is specifically a
> local variable.
yeah, but we're programmers, and we have the technology to change that.
> The parent process should not be touching it. It is up
> to the process how it is going to go away.
Which process decides whether the user wants a graceful shutdown vs.
quick -n- dirty shutdown vs. a restart?
> The fact that this patch moves
> it to the parent's job to set workers_may_exit is incorrect.
>
I respect your opinion, but disagree. This patch works, and it has a
really nice side benefit, as well.
I think it's great that those death codes are in the scoreboard for
admin reasons as well as for sealing up the timing windows. mod_status
could display them, and the admin would have a better view of what
Apache is up to internally in our long running download scenarios, for
example. The ability for admins to see this kind of information will be
important for the success of any threaded mpm.
Greg
Re: [PATCH] get threaded MPM to terminate
Posted by rb...@covalent.net.
> Let's say we just want to shut down. Assume one thread in one process
> is sitting in apr_pool, waiting for an accept() or the pipe of death.
> That process is pretty easy to deal with, as long as you remember to
> write to the pipe of death (which we aren't now with the CVS code,
> btw).
Not writing to the pipe_of_death is just a bug.
> But the other processes which are blocked in mutex land are trickier.
> The pipe of death doesn't do anything directly to unblock the mutexes,
> and neither does the signal. However, since the apr_poll process is
> going to be good, wake up, and start to go away, it will eventually
> release the accept mutex to another process. Before this patch, it was
> just looking at workers_may_exit, local to the process. So if the
> signal thread hasn't been dispatched yet, it will just blow past that
This is the problem with the current logic. Remember, that Manoj and I
spent a LOT of time getting this right, and it worked for a very long
time, which is why I am questioning that the algorithm is really wrong, I
believe it is a bug, not a logic error.
Can't this whole thing be solved by having the process that was woken up
by the pipe_of_death set workers_may_exit before releasing the poll lock?
BTW, if the parent is signalling a graceful shutdown, the child process's
signal handler should never be awoken until all the other threads are
gone.
> test and think it should keep on working, and fall into the apr_poll
> forever (unless Netscape/IE5/etc help us out by sending in new
> connections). Remember we had to do the pipe of death earlier to wake
> up the process which first owned the accept mutex.
>
> Having the parent set the new life_status field in the scoreboard, and
> using
> that as workers_may_exit eliminates the race and the dependency on
> enough browsers pounding the server in order for it to shut down.
The problem I have with this, is that workers_may_exit is specifically a
local variable. The parent process should not be touching it. It is up
to the process how it is going to go away. The fact that this patch moves
it to the parent's job to set workers_may_exit is incorrect.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] get threaded MPM to terminate
Posted by Greg Ames <gr...@remulak.net>.
Jeff Trawick wrote:
>
>
> This patch moves the workers_may_exit field to the scoreboard and has
> the parent process set this field and bombard the pipe of death before
> sending SIGTERM to the child processes.
>
> The workers_may_exit field is Paul's new life_status field.
I'll try to answer Ryan's question about why it needs to go into the
scoreboard. Sorry for the long post, but it's a little tricky.
Basically we stole the idea from the prefork synchronization patch,
which a lot of Smart People contributed to. It is to eliminate the race
conditions that exist when the parent signals one or more children.
Let's say we just want to shut down. Assume one thread in one process
is sitting in apr_pool, waiting for an accept() or the pipe of death.
That process is pretty easy to deal with, as long as you remember to
write to the pipe of death (which we aren't now with the CVS code,
btw).
But the other processes which are blocked in mutex land are trickier.
The pipe of death doesn't do anything directly to unblock the mutexes,
and neither does the signal. However, since the apr_poll process is
going to be good, wake up, and start to go away, it will eventually
release the accept mutex to another process. Before this patch, it was
just looking at workers_may_exit, local to the process. So if the
signal thread hasn't been dispatched yet, it will just blow past that
test and think it should keep on working, and fall into the apr_poll
forever (unless Netscape/IE5/etc help us out by sending in new
connections). Remember we had to do the pipe of death earlier to wake
up the process which first owned the accept mutex.
Having the parent set the new life_status field in the scoreboard, and
using
that as workers_may_exit eliminates the race and the dependency on
enough browsers pounding the server in order for it to shut down.
>
> Try to ignore the mpm_trace() stuff in the patch...
>
No! I refuse! That's one of the best parts, especially when gdb doesn't
support threads on Linux, and threads libraries don't work on FreeBSD.
But yeah, it would be nice to spruce it up a bit so you could turn it on
and off from a config
directive, or better yet, the command line.
> Anything good in the following patch is from Greg Ames. The rest is
> mine.
>
uhhh, thanks, but I don't deserve that much credit. Several of us
brainstormed on it, and as I said, we stole ideas from the prefork
patch.
Greg
Re: [PATCH] get threaded MPM to terminate
Posted by rb...@covalent.net.
> Mostly I deserve credit for pissing Ryan off enough to get his creative
> juices flowing in a good direction. Apparently that's a useful thing to
> do once in a while. :-)
Ha Ha. These days, the only way I can find the time to invest in this
stuff is to get riled up. Without that, I have three or four other
projects that take my attention away from new-httpd. :-)
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] get threaded MPM to terminate
Posted by Greg Ames <gr...@remulak.net>.
rbb@covalent.net wrote:
>
> I now have a patch that compiles cleanly, and should completely remove the
> pipe_of_death. The basic idea is that everything is done through signals,
> using the SIGWAIT logic. As for waking threads up from apr_poll, that is
> done by connecting to the socket from the server itself, and then
> immediately closing the socket.
>
>
> The idea for the patch came in a conversation with Greg Ames earlier
> today, so he deserves the share the credit for the patch. :-)
>
Mostly I deserve credit for pissing Ryan off enough to get his creative
juices flowing in a good direction. Apparently that's a useful thing to
do once in a while. :-)
There are a couple of things I really like about this idea:
* If we minimize the number of different control paths into the child
processes, we minimize race conditions, minimize path length in
worker_thread, and maximize reliability. With the pipe of death gone,
there will be one control path into the child, namely signals.
* (drum roll...) SINGLE_LISTEN_UNSERIALIZED_ACCEPT becomes possible once
more. This means we have a potential gain in scalability in the
threaded mpm, as well as shrinking worker_thread path length some more.
Look out Zeus, iPlanet, and IIS! oh yeah, we still have a problem with
malloc/free, but we kinda sorta have a direction to take care of that.
Yes, there are some messy details to work out to get S_L_U_A working
reliably, but it's worth it.
Greg
Re: [PATCH] get threaded MPM to terminate
Posted by Jeff Trawick <tr...@bellsouth.net>.
Greg Ames <gr...@remulak.net> writes:
> If you do an "apachectl graceful" first, that seems to work fine (patch
> or no steenkin' patch). But then if you follow it up with "apachectl
> restart" or "apachectl stop", this one strange process or thread won't
> die. "ps ax -O wchan" says it is in "unix_a", whatever that is. It is
> the first thing listed under the parent after you start apache. And the
> parent bails out on a restart sometimes :-(
The "unix_a" process is cgid... "unix_a" is apparently the AF_UNIX
accept() entry point.
cgid seems to be left hanging in a number of situations. I wonder if
there are cases where the pool for that process isn't getting cleaned
up before the parent bails out.
--
Jeff Trawick | trawickj@bellsouth.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] get threaded MPM to terminate
Posted by Jeff Trawick <tr...@bellsouth.net>.
Greg Ames <gr...@remulak.net> writes:
> Ryan or Jeff - what's up with a missing "workers_may_exit=1" on line
> 732? That's in make_child with the current CVS; no comprende.
I suspect that Ryan meant to add such a line right after the call to
apr_signal_thread() a few lines back (after the current line 710).
You still need to wake up the worker threads. If you do it with the
pipe of death then the first/only thread per process which reads from
the pipe of death will set workers_may_exit anyway.
--
Jeff Trawick | trawickj@bellsouth.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] get threaded MPM to terminate
Posted by rb...@covalent.net.
> Ryan or Jeff - what's up with a missing "workers_may_exit=1" on line
> 732? That's in make_child with the current CVS; no comprende.
That may explain the problem I am having with my patch. :-)
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] get threaded MPM to terminate
Posted by Greg Ames <gr...@remulak.net>.
Greg Ames wrote:
>
>
> In the mean time, I'll trim down this patch to the most essential piece
> - writing to the pipe of death - and commit it. Please speak up ASAP if
> you have a problem with this.
>
Well, I have a problem with this.
Just writing to the pipe of death isn't enough apparently. I did that
in the shutdown leg, then noticed that "apachectl restart" didn't work
right. That looked easy - no pipe of death in that section of code - so
I did pipe of death stuff in the restart leg as well, but it was still
flaky.
If you do an "apachectl graceful" first, that seems to work fine (patch
or no steenkin' patch). But then if you follow it up with "apachectl
restart" or "apachectl stop", this one strange process or thread won't
die. "ps ax -O wchan" says it is in "unix_a", whatever that is. It is
the first thing listed under the parent after you start apache. And the
parent bails out on a restart sometimes :-(
My trimmed down patch might be a slight improvement over what is in CVS
at the moment but it is nowhere near the solution our users need. So
I'm not committing it as it stands now.
Ryan or Jeff - what's up with a missing "workers_may_exit=1" on line
732? That's in make_child with the current CVS; no comprende.
Greg
Re: [PATCH] get threaded MPM to terminate
Posted by "Victor J. Orlikowski" <v....@gte.net>.
> cane! (Victor, is it a coincidence that you decided to work from home
> today??)
>
Most certainly :)
Victor
--
Victor J. Orlikowski
======================
v.j.orlikowski@gte.net
orlikowski@apache.org
vjo@us.ibm.com
Re: [PATCH] get threaded MPM to terminate
Posted by Greg Ames <gr...@remulak.net>.
Rodent of Unusual Size wrote:
>
> rbb@covalent.net wrote:
> >
> > I now have a patch that compiles cleanly, and should completely
> > remove the pipe_of_death. The basic idea is that everything is
> > done through signals, using the SIGWAIT logic. As for waking
> > threads up from apr_poll, that is done by connecting to the socket
> > from the server itself, and then immediately closing the socket.
>
> Ugh. Ick. -1. If the system's stack is hosed, or the network
> is being shut down (think OS/390), this can cause problems.
<sigh>
You wouldn't believe the flack I took on this one. Both Jeff and the
Rodent were after me. ISTR a snide remark about being a spy from a
small company in the Bay area...sheesh. I think Ken was considering
beating me with a stick, and we're not talking chopsticks, it was his
cane! (Victor, is it a coincidence that you decided to work from home
today??)
We can solve this specific OS/390 issue pretty simply -
clean_child_exit(APEXIT_CHILDFATAL). Believe me, we won't have any
trouble popping the accept()s or poll()s when the stack is down. OS/390
users don't expect network apps to stay up across stack bounces anyway.
I'll do that in a bit, unless Jeff beats me to it.
But there are other complications, unfortunately. I hope we can work
thru enough of the issues so that we could get rid of the pipe of death
for some set of users, but it's hard to say at this point.
In the mean time, I'll trim down this patch to the most essential piece
- writing to the pipe of death - and commit it. Please speak up ASAP if
you have a problem with this.
Greg
Re: [PATCH] get threaded MPM to terminate
Posted by Greg Ames <gr...@remulak.net>.
"Roy T. Fielding" wrote:
>
> > > And why does each worker grab a mutex before checking the pipe of death?
> > > All that a worker does is check to see if there is a character on the
> > > pipe and, if so, it tells all of the workers to exit. So why does it
> > > matter if more than one worker gets inside that exclusion zone? *shrug*
> >
> > yet another good question. I'll take a crack at it:
> >
> > when the parent decides to shut down or restart, it writes one character
> > per process on the PoD. If multiple worker threads within a given
> > process are allowed to each eat a character, some other process may not
> > be woken up . This stall only occurs under very light loads (e.g.
> > developers running tests).
>
> Wouldn't the same result be achieved by simply closing the pipe for
> writing? Then when the children attempt to read they get an EOF,
> regardless of how many attempt the read. This is assuming that the
> parent opens a unique pipe of death per graceful restart, which
> seems to be the case.
>
hmmmm...sounds reasonable, and sounds like less code too. Code that's
not there can't break, and can't eat CPU either. I like it!
Greg (who hasn't checked the unique pipe of death per restart situation)
Re: [PATCH] get threaded MPM to terminate
Posted by "Roy T. Fielding" <fi...@ebuilt.com>.
> > And why does each worker grab a mutex before checking the pipe of death?
> > All that a worker does is check to see if there is a character on the
> > pipe and, if so, it tells all of the workers to exit. So why does it
> > matter if more than one worker gets inside that exclusion zone? *shrug*
>
> yet another good question. I'll take a crack at it:
>
> when the parent decides to shut down or restart, it writes one character
> per process on the PoD. If multiple worker threads within a given
> process are allowed to each eat a character, some other process may not
> be woken up . This stall only occurs under very light loads (e.g.
> developers running tests).
Wouldn't the same result be achieved by simply closing the pipe for
writing? Then when the children attempt to read they get an EOF,
regardless of how many attempt the read. This is assuming that the
parent opens a unique pipe of death per graceful restart, which
seems to be the case.
....Roy
Re: [PATCH] get threaded MPM to terminate
Posted by Greg Ames <gr...@remulak.net>.
"Roy T. Fielding" wrote:
>
>
> The reason I have no confidence in the pipe of death as a solution is
> simply that I haven't seen it work yet under stressful conditions,
that's why I asked people to beat up on threaded, esp.
shutdown/restart. I'm hearing good reports so far (knock on wood) about
shutdown/restart. Due to that and due to gaining an understanding of
the design because of my recent foray into threaded, I'm fairly
confident in the Pipe of Death.
That said, I'd prefer to see the PoD disappear for performance reasons,
if we can work thru the issues. But IMO the first priority is making a
stable, generally usable version of threaded available.
> and
> when I look at the code it just seems like something is wrong. Like,
> why is it that workers_may_exit is never set to zero?
it's init'ed to zero...
> Is it merely
> because only the workers ever set it to 1, and once that happens it is
> on the road to exiting the process in any case?
yessir
>
> And why does each worker grab a mutex before checking the pipe of death?
> All that a worker does is check to see if there is a character on the
> pipe and, if so, it tells all of the workers to exit. So why does it
> matter if more than one worker gets inside that exclusion zone? *shrug*
yet another good question. I'll take a crack at it:
when the parent decides to shut down or restart, it writes one character
per process on the PoD. If multiple worker threads within a given
process are allowed to each eat a character, some other process may not
be woken up . This stall only occurs under very light loads (e.g.
developers running tests).
This mutexing is outside of the hot path, so *shrug* here too. I just
committed a comment about "why the mutex" for posterity, so if it's
wrong, please jump all over me :-)
Greg
Re: [PATCH] get threaded MPM to terminate
Posted by "Roy T. Fielding" <fi...@ebuilt.com>.
On Fri, Apr 27, 2001 at 06:12:56PM -0700, dean gaudet wrote:
> On Wed, 25 Apr 2001, Roy T. Fielding wrote:
>
> > replacement works better than what we have now in CVS. The claim
> > that the pipe of death is somehow better than 1.3 signals is just wrong.
>
> if you use signals then you have a requirment that all libraries linked
> with httpd be signal safe.
Sure, but that is a probability issue -- if the library is poorly written
then it is possible that a request will get hosed in mid-process during
a graceful restart. There is no real difference between that and the
fact that we don't read-lock every file while it is being sent. Services
that need stronger integrity (like big database back-ends storing transaction
records) will need to be signal-safe.
Anyway, what I meant was that until the pipe of death is proven to work
at least as reliably as signals, I don't think it is reasonable for anyone
to make blanket vetos on "regressing" the method to something which we
do know will work, albeit with the problem you mention.
The reason I have no confidence in the pipe of death as a solution is
simply that I haven't seen it work yet under stressful conditions, and
when I look at the code it just seems like something is wrong. Like,
why is it that workers_may_exit is never set to zero? Is it merely
because only the workers ever set it to 1, and once that happens it is
on the road to exiting the process in any case? Hmmm, maybe I'm just
forgetting that there is always a separate parent process that is
creating children, even on winnt and threaded.
And why does each worker grab a mutex before checking the pipe of death?
All that a worker does is check to see if there is a character on the
pipe and, if so, it tells all of the workers to exit. So why does it
matter if more than one worker gets inside that exclusion zone? *shrug*
....Roy
Re: [PATCH] get threaded MPM to terminate
Posted by dean gaudet <dg...@arctic.org>.
On Wed, 25 Apr 2001, Roy T. Fielding wrote:
> replacement works better than what we have now in CVS. The claim
> that the pipe of death is somehow better than 1.3 signals is just wrong.
if you use signals then you have a requirment that all libraries linked
with httpd be signal safe.
good luck.
-dean
Re: [PATCH] get threaded MPM to terminate
Posted by "Roy T. Fielding" <fi...@ebuilt.com>.
On Wed, Apr 25, 2001 at 02:06:24PM -0400, Rodent of Unusual Size wrote:
> rbb@covalent.net wrote:
> >
> > I now have a patch that compiles cleanly, and should completely
> > remove the pipe_of_death. The basic idea is that everything is
> > done through signals, using the SIGWAIT logic. As for waking
> > threads up from apr_poll, that is done by connecting to the socket
> > from the server itself, and then immediately closing the socket.
>
> Ugh. Ick. -1. If the system's stack is hosed, or the network
> is being shut down (think OS/390), this can cause problems. Or
> think of a protocol module that is not even talking to the network,
> but is doing some other kind of job. Never send your control
> functions over the data channel; they need to be out of band from
> the data.
>
> We got away from the 1.3 signal module for well-known reasons.
> The pipe_of_death is cross-platform safe and provides the necessary
> granularity, but interferes with SINGLE_LISTEN_UNSERIALIZED_ACCEPT.
> Any replacement for pipe_of_death must not introduce any regressions;
> using the data channel will do so for the reasons stated above.
> Some other mechanism may have the advantages of pipe_of_death
> without its drawbacks, but this is not it.
Ummm, speaking as someone who has very little confidence that the
pipe of death works reliably at all, the only requirement is that the
replacement works better than what we have now in CVS. The claim
that the pipe of death is somehow better than 1.3 signals is just wrong.
It may be more portable, but that is why we have #ifdefs.
....Roy
Re: [PATCH] get threaded MPM to terminate
Posted by Rodent of Unusual Size <Ke...@Golux.Com>.
rbb@covalent.net wrote:
>
> I now have a patch that compiles cleanly, and should completely
> remove the pipe_of_death. The basic idea is that everything is
> done through signals, using the SIGWAIT logic. As for waking
> threads up from apr_poll, that is done by connecting to the socket
> from the server itself, and then immediately closing the socket.
Ugh. Ick. -1. If the system's stack is hosed, or the network
is being shut down (think OS/390), this can cause problems. Or
think of a protocol module that is not even talking to the network,
but is doing some other kind of job. Never send your control
functions over the data channel; they need to be out of band from
the data.
We got away from the 1.3 signal module for well-known reasons.
The pipe_of_death is cross-platform safe and provides the necessary
granularity, but interferes with SINGLE_LISTEN_UNSERIALIZED_ACCEPT.
Any replacement for pipe_of_death must not introduce any regressions;
using the data channel will do so for the reasons stated above.
Some other mechanism may have the advantages of pipe_of_death
without its drawbacks, but this is not it.
--
#ken P-)}
Ken Coar <http://Golux.Com/coar/>
Apache Software Foundation <http://www.apache.org/>
"Apache Server for Dummies" <http://Apache-Server.Com/>
"Apache Server Unleashed" <http://ApacheUnleashed.Com/>
Re: [PATCH] get threaded MPM to terminate
Posted by rb...@covalent.net.
I now have a patch that compiles cleanly, and should completely remove the
pipe_of_death. The basic idea is that everything is done through signals,
using the SIGWAIT logic. As for waking threads up from apr_poll, that is
done by connecting to the socket from the server itself, and then
immediately closing the socket.
This is completely untested, and I am too tired of looking at computers to
continue. I will test the patch tomorrow and hopefully post tomorrow.
This should solve all the problems discussed today.
The idea for the patch came in a conversation with Greg Ames earlier
today, so he deserves the share the credit for the patch. :-)
Ryan
On 24 Apr 2001, Jeff Trawick wrote:
> "Bill Stoddard" <bi...@wstoddard.com> writes:
>
> > I believe Ryan is right here...
>
> I agree that there is no race condition. I don't agree with the
> desire to keep it out of the scoreboard but I suspect all that will
> change anyway :)
>
> I'll trim the patch down to just writing to the pipe of death.
>
> --
> Jeff Trawick | trawickj@bellsouth.net | PGP public key at web site:
> http://www.geocities.com/SiliconValley/Park/9289/
> Born in Roswell... married an alien...
>
>
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] get threaded MPM to terminate
Posted by rb...@covalent.net.
On 24 Apr 2001, Jeff Trawick wrote:
> "Bill Stoddard" <bi...@wstoddard.com> writes:
>
> > I believe Ryan is right here...
>
> I agree that there is no race condition. I don't agree with the
> desire to keep it out of the scoreboard but I suspect all that will
> change anyway :)
>
> I'll trim the patch down to just writing to the pipe of death.
Actually, Greg Ames and I spoke on the phone, and I belive the best idea
is to actually remove the pipe of death. I am going to work on the patch
to do this right now. I'll post a status update tonight.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] get threaded MPM to terminate
Posted by Jeff Trawick <tr...@bellsouth.net>.
"Bill Stoddard" <bi...@wstoddard.com> writes:
> I believe Ryan is right here...
I agree that there is no race condition. I don't agree with the
desire to keep it out of the scoreboard but I suspect all that will
change anyway :)
I'll trim the patch down to just writing to the pipe of death.
--
Jeff Trawick | trawickj@bellsouth.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] get threaded MPM to terminate
Posted by Bill Stoddard <bi...@wstoddard.com>.
>
> > On 24 Apr 2001, Jeff Trawick wrote:
> >
> > > > Why is this field going into the scoreboard? This field is really private
> > > > to the child process, and doesn't need to go into shared memory.
> > > >
> > > > Can't the problem describes above be solved with a simple
> > > >
> > > > workers_may_exit = 1;
> > > >
> > > > at line 732 of threaded.c?
> > >
> > > You're at least close to right.
> > >
> > > You still have the problem that there is nothing to wake up the worker
> > > threads; thus we need to write to the pipe of death as done in the
> > > patch.
>
> I believe Ryan is right here...
>
> All but one worker should be blocked on the accept mutex (find SAFE_ACCEPT() in
> threaded.c). One worker is blocked in apr_poll(). When the parent writes to the pipe
of
> death, the thread in apr_poll is awakened and it is able to intuit if the pipe of death
> was written to or not, so it calls check_pipe_of_death(). If the pipe_of_death() was
> written to, workers_may_exit is set. Then the thread releases the accept mutex and
exits.
> The release of the accept mutex allows another thread to enter the locked region. The
very
> first thing it does is check workers_may_exit, sees that it is set, releases the accept
> mutex and exits. This pattern should repeat untill all the workers have exited at which
> point the main thread pthread_join call fails allowing the main thread to exit.
>
> I don't think we need the workers_may_exit in shared memory because the accept_mutex
> guarantees no workers will sneak into the apr_poll() call.
BTW, this last point was a revelation to me up until 10 minutes ago. I just read the code
:-). I thought there was a race between setting workers_may_exit and workers going into
apr_poll.
Bill
Re: [PATCH] get threaded MPM to terminate
Posted by Bill Stoddard <bi...@wstoddard.com>.
> On 24 Apr 2001, Jeff Trawick wrote:
>
> > > Why is this field going into the scoreboard? This field is really private
> > > to the child process, and doesn't need to go into shared memory.
> > >
> > > Can't the problem describes above be solved with a simple
> > >
> > > workers_may_exit = 1;
> > >
> > > at line 732 of threaded.c?
> >
> > You're at least close to right.
> >
> > You still have the problem that there is nothing to wake up the worker
> > threads; thus we need to write to the pipe of death as done in the
> > patch.
I believe Ryan is right here...
All but one worker should be blocked on the accept mutex (find SAFE_ACCEPT() in
threaded.c). One worker is blocked in apr_poll(). When the parent writes to the pipe of
death, the thread in apr_poll is awakened and it is able to intuit if the pipe of death
was written to or not, so it calls check_pipe_of_death(). If the pipe_of_death() was
written to, workers_may_exit is set. Then the thread releases the accept mutex and exits.
The release of the accept mutex allows another thread to enter the locked region. The very
first thing it does is check workers_may_exit, sees that it is set, releases the accept
mutex and exits. This pattern should repeat untill all the workers have exited at which
point the main thread pthread_join call fails allowing the main thread to exit.
I don't think we need the workers_may_exit in shared memory because the accept_mutex
guarantees no workers will sneak into the apr_poll() call.
Bill
Re: [PATCH] get threaded MPM to terminate
Posted by rb...@covalent.net.
On 24 Apr 2001, Jeff Trawick wrote:
> > Why is this field going into the scoreboard? This field is really private
> > to the child process, and doesn't need to go into shared memory.
> >
> > Can't the problem describes above be solved with a simple
> >
> > workers_may_exit = 1;
> >
> > at line 732 of threaded.c?
>
> You're at least close to right.
>
> You still have the problem that there is nothing to wake up the worker
> threads; thus we need to write to the pipe of death as done in the
> patch.
>
> You need to guarantee that the flag checked by worker threads is set
> before they wake up and potentially go back to sleep. Therefore the
> parent process needs to set it before writing on the pipe of death.
> Since the parent process is writing it and the child process is
> looking at it then it must be in the scoreboard.
>
> (Where did I go astray here?)
The parent should not know anything about workers_may_exit. If the parent
sends a message through pipe_of_death, then the thread that gets that
message should be setting workers_may_exit.
This removes the parent from the equation at all.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] get threaded MPM to terminate
Posted by Jeff Trawick <tr...@bellsouth.net>.
> Why is this field going into the scoreboard? This field is really private
> to the child process, and doesn't need to go into shared memory.
>
> Can't the problem describes above be solved with a simple
>
> workers_may_exit = 1;
>
> at line 732 of threaded.c?
You're at least close to right.
You still have the problem that there is nothing to wake up the worker
threads; thus we need to write to the pipe of death as done in the
patch.
You need to guarantee that the flag checked by worker threads is set
before they wake up and potentially go back to sleep. Therefore the
parent process needs to set it before writing on the pipe of death.
Since the parent process is writing it and the child process is
looking at it then it must be in the scoreboard.
(Where did I go astray here?)
--
Jeff Trawick | trawickj@bellsouth.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] get threaded MPM to terminate
Posted by rb...@covalent.net.
On Tue, 24 Apr 2001, Jeff Trawick wrote:
> With the level of code in CVS, SIGTERM wakes up the main thread in
> each child. The main thread then enters pthread_join() to wait for a
> worker thread to complete but nothing ever happens because there is
> nothing to wake up a worker thread.
>
> This patch moves the workers_may_exit field to the scoreboard and has
> the parent process set this field and bombard the pipe of death before
> sending SIGTERM to the child processes.
>
> The workers_may_exit field is Paul's new life_status field. Since
> this field applies to the entire child process it was moved to a
> different place in the scoreboard and prefork.c was updated as
> appropriate.
>
> Try to ignore the mpm_trace() stuff in the patch...
>
> Anything good in the following patch is from Greg Ames. The rest is
> mine.
>
> Comments?
>
Why is this field going into the scoreboard? This field is really private
to the child process, and doesn't need to go into shared memory.
Can't the problem describes above be solved with a simple
workers_may_exit = 1;
at line 732 of threaded.c?
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------