You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by je...@apache.org on 2002/05/01 09:15:40 UTC
cvs commit: httpd-2.0/server/mpm/worker worker.c
jerenkrantz 02/05/01 00:15:40
Modified: . CHANGES
server/mpm/worker worker.c
Log:
Close sockets on worker MPM when doing a graceless restart. This should
resolve some segfaults see when doing such restarts.
(Justin tweaked the palloc/memset in favor of calloc.)
Submitted by: Aaron Bannert
Reviewed by: Greg Ames, Sander Striker, Justin Erenkrantz
Revision Changes Path
1.750 +3 -0 httpd-2.0/CHANGES
Index: CHANGES
===================================================================
RCS file: /home/cvs/httpd-2.0/CHANGES,v
retrieving revision 1.749
retrieving revision 1.750
diff -u -r1.749 -r1.750
--- CHANGES 30 Apr 2002 17:10:11 -0000 1.749
+++ CHANGES 1 May 2002 07:15:39 -0000 1.750
@@ -1,5 +1,8 @@
Changes with Apache 2.0.37
+ *) Close sockets on worker MPM when doing a graceless restart.
+ [Aaron Bannert]
+
*) Reverted a minor optimization in mod_ssl.c that used the vhost ID
as the session id context rather that a MD5 hash of that vhost ID,
because it caused very long vhost id's to be unusable with mod_ssl.
1.121 +21 -0 httpd-2.0/server/mpm/worker/worker.c
Index: worker.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm/worker/worker.c,v
retrieving revision 1.120
retrieving revision 1.121
diff -u -r1.120 -r1.121
--- worker.c 28 Apr 2002 22:13:32 -0000 1.120
+++ worker.c 1 May 2002 07:15:39 -0000 1.121
@@ -252,6 +252,21 @@
*/
#define LISTENER_SIGNAL SIGHUP
+/* An array of socket descriptors in use by each thread used to
+ * perform a non-graceful (forced) shutdown of the server. */
+static apr_socket_t **worker_sockets;
+
+static void close_worker_sockets(void)
+{
+ int i;
+ for (i = 0; i < ap_threads_per_child; i++) {
+ if (worker_sockets[i]) {
+ apr_socket_close(worker_sockets[i]);
+ worker_sockets[i] = NULL;
+ }
+ }
+}
+
static void wakeup_listener(void)
{
listener_may_exit = 1;
@@ -301,6 +316,7 @@
workers_may_exit = 1;
ap_queue_interrupt_all(worker_queue);
ap_queue_info_term(worker_queue_info);
+ close_worker_sockets(); /* forcefully kill all current connections */
}
}
@@ -912,7 +928,9 @@
}
continue;
}
+ worker_sockets[thread_slot] = csd;
process_socket(ptrans, csd, process_slot, thread_slot, bucket_alloc);
+ worker_sockets[thread_slot] = NULL;
requests_this_child--; /* FIXME: should be synchronized - aaron */
apr_pool_clear(ptrans);
last_ptrans = ptrans;
@@ -1001,6 +1019,9 @@
"ap_queue_info_create() failed");
clean_child_exit(APEXIT_CHILDFATAL);
}
+
+ worker_sockets = apr_pcalloc(pchild, ap_threads_per_child
+ * sizeof(apr_socket_t *));
loops = prev_threads_created = 0;
while (1) {
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Posted by Aaron Bannert <aa...@clove.org>.
> > safe_free()-like stuff does, but at least it is a compromise. OTOH,
> > as Cliff brought up, we'll get a SEGV if apr_palloc() returns NULL.
>
> Whoa, slow down. I said that was an argument someone had once posed, but
> I also said I thought it was ridiculous. We just don't need to worry
> about palloc returning NULL. None of the rest of Apache does.
Fair enough. [I didn't mean to imply that you thought it was a concern,
just that you remembered someone bringing it up.]
-aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Posted by Cliff Woolley <jw...@virginia.edu>.
On Wed, 1 May 2002, Aaron Bannert wrote:
> - Does this work on ... (linux)?
Yes, it seems to. Something else is screwy, but as a whole, it kind of
works at least, and this piece in particular seems to be doing fine.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Posted by Aaron Bannert <aa...@clove.org>.
> Because we have to keep the old API working, and because duplicating code
> everywhere is a bad thing.
How is it duplicated? This is new code.
> And while we are on the topic, anything that is posted to the mailing
> list is open for others to commit to the code base. That is how we work.
> People here are expected to be part-time volunteers, so if one person does
> 60% of the work and posts it, others should feel free to do the other 40%
> and commit the sucker while the originator is sleeping. The only necessary
> part is that it be appropriately attributed in Submitted By.
If you go back and read my original post, you'll realize that the reason
I posted it was so that I could get some feedback. If I thought it only
needed one +1 I wouldn't have posted it. I don't think 2 days (partly
over a weekend) is enough time for everyone to review a patch with such
ugly side-effects.
Furthermore, this patch still has outstanding issues which I think
warrant discussion:
- Will the log message confuse people? Greg suggests we drop the level down
during forcefull restart/shutdown.
- Does this work on all platforms (linux)?
- Have we beaten this enough to call it release quality?
> In this case, there is no excuse for sitting on a bug fix just because
> there are stylistic issues about a patch. The appropriate thing to do
> is remove the style changes and commit the fix.
I don't think anyone was sitting on it for stylistic issues, where did
you get that from? Besides, the commit message is inaccurate and partly
incorrect.
-aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Posted by Cliff Woolley <jw...@virginia.edu>.
On Wed, 1 May 2002, Aaron Bannert wrote:
> safe_free()-like stuff does, but at least it is a compromise. OTOH,
> as Cliff brought up, we'll get a SEGV if apr_palloc() returns NULL.
Whoa, slow down. I said that was an argument someone had once posed, but
I also said I thought it was ridiculous. We just don't need to worry
about palloc returning NULL. None of the rest of Apache does.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Posted by "Roy T. Fielding" <fi...@apache.org>.
On Wednesday, May 1, 2002, at 01:49 PM, Aaron Bannert wrote:
>> And, consider my position on your calloc change in this patch as a
>> veto. If you want to remove calloc, then you should do so throughout
>> the code rather than in sporadic places that may make maintaining the
>> code a nightmare if we were to fix calloc. But, that is an issue
>> that is now open in APR's STATUS.
>
> What exactly are you vetoing? My use of apr_palloc()+memset() is
> technically superior to apr_pcalloc(). What is your technical reason
> for forcing me to use apr_pcalloc()?
Umm, no it isn't. The reason is that it makes the code harder to
understand.
>> If the end result of the calloc vote is that we should remove calloc,
>> then feel free to do a search and replace across the entire tree.
>> Until then, let's please remain consistent to our API. -- justin
>
> I'm really not sure about the macro yet. On the one hand it's too
> late to remove apr_pcalloc(). The macro stinks the same way #define
> safe_free()-like stuff does, but at least it is a compromise. OTOH,
> as Cliff brought up, we'll get a SEGV if apr_palloc() returns NULL. I
> don't see how:
>
> foo = apr_palloc(p, sizeof(*foo));
> memset(foo, 0, sizeof(*foo));
>
> is any less desirable than:
>
> foo = apr_pcalloc(p, sizeof(*foo));
>
> It is quite obvious how the memory is being initialized in both cases,
> and for the reasons discussed earlier it is obvious why the first would
> be optimal.
Because we have to keep the old API working, and because duplicating code
everywhere is a bad thing. The arguments have already been made. I don't
even understand why people are voting on the macro -- just commit it.
Let's save the arguments for things where actual disagreement is useful.
And while we are on the topic, anything that is posted to the mailing
list is open for others to commit to the code base. That is how we work.
People here are expected to be part-time volunteers, so if one person does
60% of the work and posts it, others should feel free to do the other 40%
and commit the sucker while the originator is sleeping. The only necessary
part is that it be appropriately attributed in Submitted By.
In this case, there is no excuse for sitting on a bug fix just because
there are stylistic issues about a patch. The appropriate thing to do
is remove the style changes and commit the fix.
....Roy
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Posted by Aaron Bannert <aa...@clove.org>.
> And, consider my position on your calloc change in this patch as a
> veto. If you want to remove calloc, then you should do so throughout
> the code rather than in sporadic places that may make maintaining the
> code a nightmare if we were to fix calloc. But, that is an issue
> that is now open in APR's STATUS.
What exactly are you vetoing? My use of apr_palloc()+memset() is
technically superior to apr_pcalloc(). What is your technical reason
for forcing me to use apr_pcalloc()?
> If the end result of the calloc vote is that we should remove calloc,
> then feel free to do a search and replace across the entire tree.
> Until then, let's please remain consistent to our API. -- justin
I'm really not sure about the macro yet. On the one hand it's too
late to remove apr_pcalloc(). The macro stinks the same way #define
safe_free()-like stuff does, but at least it is a compromise. OTOH,
as Cliff brought up, we'll get a SEGV if apr_palloc() returns NULL. I
don't see how:
foo = apr_palloc(p, sizeof(*foo));
memset(foo, 0, sizeof(*foo));
is any less desirable than:
foo = apr_pcalloc(p, sizeof(*foo));
It is quite obvious how the memory is being initialized in both cases,
and for the reasons discussed earlier it is obvious why the first would
be optimal.
-aaron
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Posted by Justin Erenkrantz <je...@apache.org>.
On Wed, May 01, 2002 at 12:22:13PM -0700, Aaron Bannert wrote:
> I did not post this so that you could commit it before I thought it had
> been properly reviewed. On top of that you modified my original patch
> in a way that you knew I would disagree with. Please do not do that again.
Sorry. My sincere apologies. My thought was that you were
intending to commit it once it had proper review (we had three +1s
for it). What we had before segfaulted, so I felt that it was best
to stop segfaulting. Again, I was wrong - sorry...
And, consider my position on your calloc change in this patch as a
veto. If you want to remove calloc, then you should do so throughout
the code rather than in sporadic places that may make maintaining the
code a nightmare if we were to fix calloc. But, that is an issue
that is now open in APR's STATUS.
If the end result of the calloc vote is that we should remove calloc,
then feel free to do a search and replace across the entire tree.
Until then, let's please remain consistent to our API. -- justin
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Posted by Aaron Bannert <aa...@clove.org>.
On Wed, May 01, 2002 at 07:15:40AM -0000, Justin Erenkrantz wrote:
> jerenkrantz 02/05/01 00:15:40
>
> Modified: . CHANGES
> server/mpm/worker worker.c
> Log:
> Close sockets on worker MPM when doing a graceless restart. This should
> resolve some segfaults see when doing such restarts.
>
> (Justin tweaked the palloc/memset in favor of calloc.)
>
> Submitted by: Aaron Bannert
I did not post this so that you could commit it before I thought it had
been properly reviewed. On top of that you modified my original patch
in a way that you knew I would disagree with. Please do not do that again.
-aaron
Re: Roll of 2.0.36, WAS: RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
Posted by Jeff Trawick <tr...@attglobal.net>.
"Sander Striker" <st...@apache.org> writes:
> Hi,
>
> I was going to roll 2.0.36, but I want to wait for this last
> worker change. Unfortunately I don't have the time to pursue
> the issue now, so if someone does, please feel free to take
> care of this annoying beast.
I'll start working on it now. It shouldn't be hard to add a join for
graceless termination.
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
Re: Roll of 2.0.36, WAS: RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
Posted by Eli Marmor <ma...@netmask.it>.
Sander Striker wrote:
> I was going to roll 2.0.36, but I want to wait for this last
> worker change. Unfortunately I don't have the time to pursue
> the issue now, so if someone does, please feel free to take
> care of this annoying beast.
BTW: Is there any problem with the CVS version of mod_cache?
Accroding to the latest "nightly build log" of Chuck:
> Making in httpd-2.0-nightly
> In file included from mod_cache.c:61:
> mod_cache.h:213: syntax error before `apr_atomic_t'
> In file included from cache_storage.c:61:
> mod_cache.h:213: syntax error before `apr_atomic_t'
> In file included from cache_util.c:61:
> mod_cache.h:213: syntax error before `apr_atomic_t'
--
Eli Marmor
marmor@netmask.it
CTO, Founder
Netmask (El-Mar) Internet Technologies Ltd.
__________________________________________________________
Tel.: +972-9-766-1020 8 Yad-Harutzim St.
Fax.: +972-9-766-1314 P.O.B. 7004
Mobile: +972-50-23-7338 Kfar-Saba 44641, Israel
Roll of 2.0.36, WAS: RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
Posted by Sander Striker <st...@apache.org>.
Hi,
I was going to roll 2.0.36, but I want to wait for this last
worker change. Unfortunately I don't have the time to pursue
the issue now, so if someone does, please feel free to take
care of this annoying beast.
Sander
> From: Sander Striker [mailto:striker@apache.org]
> Sent: 01 May 2002 15:11
>> From: trawick@rdu88-250-035.nc.rr.com
>> [mailto:trawick@rdu88-250-035.nc.rr.com]On Behalf Of Jeff Trawick
>> Sent: 01 May 2002 14:53
[...]
>> (My apologies for not keeping up with the discussions over the past
>> days.)
>>
>> I don't see (yet) which segfault this would fix. Maybe I was hoping
>> for a fix to the wrong problem...
>>
>> If this was to be combined with a change to do a thread-join on the
>> workers even for graceless termination, then I can see how the
>> segfault caused by cleaning up a pool that the worker threads depend
>> on would be avoided. (But of course a thread-join on a worker could
>> hang for a while, depending on what a 3rd-party module is doing.)
>>
>> This change alone doesn't do much for the race between the main thread
>> cleaning up pchild and the worker threads getting dispatched and
>> realizing that they can exit and finally finishing their use of data
>> in subpools of pchild and doing the pthread_exit().
>
> Grmpf. Ofcourse, you are correct. We need the thread_join as a patch
> job. After 2.0.36 I am afraid we have to implement apr_thread_cancel...
>
> Sander
>
RE: cvs commit: httpd-2.0/server/mpm/worker worker.c
Posted by Sander Striker <st...@apache.org>.
> From: trawick@rdu88-250-035.nc.rr.com
> [mailto:trawick@rdu88-250-035.nc.rr.com]On Behalf Of Jeff Trawick
> Sent: 01 May 2002 14:53
> jerenkrantz@apache.org writes:
>
> > jerenkrantz 02/05/01 00:15:40
> >
> > Modified: . CHANGES
> > server/mpm/worker worker.c
> > Log:
> > Close sockets on worker MPM when doing a graceless restart. This should
> > resolve some segfaults see when doing such restarts.
>
> (My apologies for not keeping up with the discussions over the past
> days.)
>
> I don't see (yet) which segfault this would fix. Maybe I was hoping
> for a fix to the wrong problem...
>
> If this was to be combined with a change to do a thread-join on the
> workers even for graceless termination, then I can see how the
> segfault caused by cleaning up a pool that the worker threads depend
> on would be avoided. (But of course a thread-join on a worker could
> hang for a while, depending on what a 3rd-party module is doing.)
>
> This change alone doesn't do much for the race between the main thread
> cleaning up pchild and the worker threads getting dispatched and
> realizing that they can exit and finally finishing their use of data
> in subpools of pchild and doing the pthread_exit().
Grmpf. Ofcourse, you are correct. We need the thread_join as a patch
job. After 2.0.36 I am afraid we have to implement apr_thread_cancel...
Sander
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Posted by Jeff Trawick <tr...@attglobal.net>.
jerenkrantz@apache.org writes:
> jerenkrantz 02/05/01 00:15:40
>
> Modified: . CHANGES
> server/mpm/worker worker.c
> Log:
> Close sockets on worker MPM when doing a graceless restart. This should
> resolve some segfaults see when doing such restarts.
(My apologies for not keeping up with the discussions over the past
days.)
I don't see (yet) which segfault this would fix. Maybe I was hoping
for a fix to the wrong problem...
If this was to be combined with a change to do a thread-join on the
workers even for graceless termination, then I can see how the
segfault caused by cleaning up a pool that the worker threads depend
on would be avoided. (But of course a thread-join on a worker could
hang for a while, depending on what a 3rd-party module is doing.)
This change alone doesn't do much for the race between the main thread
cleaning up pchild and the worker threads getting dispatched and
realizing that they can exit and finally finishing their use of data
in subpools of pchild and doing the pthread_exit().
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Posted by Jeff Trawick <tr...@attglobal.net>.
jerenkrantz@apache.org writes:
> jerenkrantz 02/05/01 00:15:40
>
> Modified: . CHANGES
> server/mpm/worker worker.c
> Log:
> Close sockets on worker MPM when doing a graceless restart. This should
> resolve some segfaults see when doing such restarts.
(My apologies for not keeping up with the discussions over the past
days.)
I don't see (yet) which segfault this would fix. Maybe I was hoping
for a fix to the wrong problem...
If this was to be combined with a change to do a thread-join on the
workers even for graceless termination, then I can see how the
segfault caused by cleaning up a pool that the worker threads depend
on would be avoided. (But of course a thread-join on a worker could
hang for a while, depending on what a 3rd-party module is doing.)
This change alone doesn't do much for the race between the main thread
cleaning up pchild and the worker threads getting dispatched and
realizing that they can exit and finally finishing their use of data
in subpools of pchild and doing the pthread_exit().
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...