You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ben Laurie <be...@algroup.co.uk> on 1998/05/18 21:47:57 UTC

Apology

OK, I screwed up. I hadn't realised what an impact that change made. I
am now fixing it for Unix, and I agree it isn't obvious what needs doing
(well, it is to me, but I can see the problem).

It is regrettable that the interface to the child-spawner needs to
change, but the kludge of adding stuff to the request_rec was just not
acceptable (to me, at least - I'm willing to be overruled, though - but
not without some argument!). I can't see any other way around it that
isn't a kludge. Note that to make this fix truly correct we must
eliminated the non-BUFF methods of spawning children (I imagine this is
also needed for NSPR - Dean?).

Sorry to all I snapped at.

Cheers,

Ben.

-- 
Ben Laurie            |Phone: +44 (181) 735 0686|  Apache Group member
Freelance Consultant  |Fax:   +44 (181) 735 0689|http://www.apache.org
and Technical Director|Email: ben@algroup.co.uk |
A.L. Digital Ltd,     |Apache-SSL author    http://www.apache-ssl.org/
London, England.      |"Apache: TDG" http://www.ora.com/catalog/apache

Re: Apology

Posted by Brian Behlendorf <br...@hyperreal.org>.
At 07:01 PM 5/19/98 +0100, Ben Laurie wrote:
>> Did the MMN get bumped?
>
>Nope, I forgot that bit again. Coming RSN.

Whenever the MMN in httpd.h gets bumped, the file mmn.txt in apache-devsite
also needs to be increased by the same person, since the person increasing
the MMN is the one who best understands the change being made.

>>  Did docs/new_features_1_3.html#api get touched?
>> Or docs/upgrading_to_1_3.html?
>
>I didn't know they should be.

For similar reasons as above.  Otherwise writing and maintaining
third-party modules becomes impossible.  

Yes, there's a lot of overhead to an API change.  That's not a problem in
my view since API changes should be made as sparingly as possible.

	Brian


--=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--
pure chewing satisfaction                                  brian@apache.org
                                                        brian@hyperreal.org

Re: Apology

Posted by Ben Laurie <be...@algroup.co.uk>.
Dean Gaudet wrote:
> 
> On Tue, 19 May 1998, Ben Laurie wrote:
> 
> > Yeah, that is a possible argument, but if you poke around, you'll
> > discover that there are sometimes two different request_recs involved,
> > for example, from mod_cgi:
> >
> >     /*
> >      * we spawn out of r->main if it's there so that we can avoid
> >      * waiting for free_proc_chain to cleanup in the middle of an
> >      * SSI request -djg
> >      */
> >     if (!ap_spawn_child_err_buff(r->main ? r->main->pool : r->pool,
> > cgi_child,
> >                                (void *) &cld,
> >                                kill_after_timeout,
> >                                &script_out, &script_in, &script_err)) {
> >       ap_log_error(APLOG_MARK, APLOG_ERR, r->server,
> >                   "couldn't spawn child process: %s", r->filename);
> >       return SERVER_ERROR;
> >     }
> 
> Yeah, this is because of something that Roy keeps saying is wrong -- that
> we send kill -TERM, wait 3 seconds and then send kill -9.  I think in the
> past I've said "I like the -9 for the reliability of my server when
> there's clueless users developing CGIs".  I changed my mind though... and
> in apache-nspr it just closes all the pipes and sends a single -TERM and
> then forgets about it.  Completely.  There isn't even a cleanup registered
> for it.

Possibly it should be a configurable option.

Cheers,

Ben.

-- 
Ben Laurie            |Phone: +44 (181) 735 0686|  Apache Group member
Freelance Consultant  |Fax:   +44 (181) 735 0689|http://www.apache.org
and Technical Director|Email: ben@algroup.co.uk |
A.L. Digital Ltd,     |Apache-SSL author    http://www.apache-ssl.org/
London, England.      |"Apache: TDG" http://www.ora.com/catalog/apache

Re: Apology

Posted by Dean Gaudet <dg...@arctic.org>.
On Tue, 19 May 1998, Ben Laurie wrote:

> Yeah, that is a possible argument, but if you poke around, you'll
> discover that there are sometimes two different request_recs involved,
> for example, from mod_cgi:
> 
>     /*
>      * we spawn out of r->main if it's there so that we can avoid
>      * waiting for free_proc_chain to cleanup in the middle of an
>      * SSI request -djg
>      */
>     if (!ap_spawn_child_err_buff(r->main ? r->main->pool : r->pool,
> cgi_child,
> 			         (void *) &cld,
> 			         kill_after_timeout,
> 			         &script_out, &script_in, &script_err)) {
> 	ap_log_error(APLOG_MARK, APLOG_ERR, r->server,
> 		    "couldn't spawn child process: %s", r->filename);
> 	return SERVER_ERROR;
>     }

Yeah, this is because of something that Roy keeps saying is wrong -- that
we send kill -TERM, wait 3 seconds and then send kill -9.  I think in the
past I've said "I like the -9 for the reliability of my server when
there's clueless users developing CGIs".  I changed my mind though... and
in apache-nspr it just closes all the pipes and sends a single -TERM and
then forgets about it.  Completely.  There isn't even a cleanup registered
for it. 

Dean


Re: Apology

Posted by Ben Laurie <be...@algroup.co.uk>.
Brian Behlendorf wrote:
> 
> At 08:47 PM 5/18/98 +0100, you wrote:
> >It is regrettable that the interface to the child-spawner needs to
> >change, but the kludge of adding stuff to the request_rec was just not
> >acceptable (to me, at least - I'm willing to be overruled, though - but
> >not without some argument!).
> 
> How about: by adding things to the request_rec you don't have to change the
> API in a way which breaks 3rd-party modules which use ap_call_exec?

Yeah, that is a possible argument, but if you poke around, you'll
discover that there are sometimes two different request_recs involved,
for example, from mod_cgi:

    /*
     * we spawn out of r->main if it's there so that we can avoid
     * waiting for free_proc_chain to cleanup in the middle of an
     * SSI request -djg
     */
    if (!ap_spawn_child_err_buff(r->main ? r->main->pool : r->pool,
cgi_child,
			         (void *) &cld,
			         kill_after_timeout,
			         &script_out, &script_in, &script_err)) {
	ap_log_error(APLOG_MARK, APLOG_ERR, r->server,
		    "couldn't spawn child process: %s", r->filename);
	return SERVER_ERROR;
    }

so that kludge is liable to lead to confusion and bugs. Well, it
certainly confused me, and I really didn't like the idea of passing a
request_rec and a pool to ap_spawn_child_err_buff(), or two
request_recs. And it wasn't at all clear to me how to get away with only
passing one.

Also, unless 3rd party modules use ap_spawn_child_err_buff(), they are
already broken.

> Did the MMN get bumped?

Nope, I forgot that bit again. Coming RSN.

>  Did docs/new_features_1_3.html#api get touched?
> Or docs/upgrading_to_1_3.html?

I didn't know they should be.

Cheers,

Ben.

-- 
Ben Laurie            |Phone: +44 (181) 735 0686|  Apache Group member
Freelance Consultant  |Fax:   +44 (181) 735 0689|http://www.apache.org
and Technical Director|Email: ben@algroup.co.uk |
A.L. Digital Ltd,     |Apache-SSL author    http://www.apache-ssl.org/
London, England.      |"Apache: TDG" http://www.ora.com/catalog/apache

Re: Apology

Posted by Brian Behlendorf <br...@hyperreal.org>.
At 08:47 PM 5/18/98 +0100, you wrote:
>OK, I screwed up. I hadn't realised what an impact that change made. I
>am now fixing it for Unix, and I agree it isn't obvious what needs doing
>(well, it is to me, but I can see the problem).

Thank you, Ben.  I am willing to tolerate a little breakage from time to
time between platforms (it happens all the time elsewhere), but when it's
so easy to make it at least compile and run on other platforms, it really
is moral to do that.  From your commit message I was not sure exactly how
it should have been fixed either, or even that Unix folks should fix it.  I
think whenever a change is committed that someone thinks might break
something on Win32, at least it's noted.

>It is regrettable that the interface to the child-spawner needs to
>change, but the kludge of adding stuff to the request_rec was just not
>acceptable (to me, at least - I'm willing to be overruled, though - but
>not without some argument!). 

How about: by adding things to the request_rec you don't have to change the
API in a way which breaks 3rd-party modules which use ap_call_exec?  

Did the MMN get bumped?  Did docs/new_features_1_3.html#api get touched?
Or docs/upgrading_to_1_3.html?

>I can't see any other way around it that
>isn't a kludge. Note that to make this fix truly correct we must
>eliminated the non-BUFF methods of spawning children

Okay, so is anything else broken right now that you can think of?

	Brian


--=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--
pure chewing satisfaction                                  brian@apache.org
                                                        brian@hyperreal.org

Re: Apology

Posted by Dean Gaudet <dg...@arctic.org>.
Yup I've been hammering at this in apache-nspr too.  It's a lot more
clean, or has the potential to be more clean.  Right now I've ripped out
all the spawn_foo code from alloc.c and tore out some pieces of
scan_script_headers.  mod_cgi contains a bit more "low level" stuff than
it did before -- it sets up stdin/out explicitly.  I could add a wrapper
somewhat like ap_spawn_child... dunno how much I like it though, I really
prefer to use the NSPR interfaces directly. 

Dean

On Mon, 18 May 1998, Ben Laurie wrote:

> OK, I screwed up. I hadn't realised what an impact that change made. I
> am now fixing it for Unix, and I agree it isn't obvious what needs doing
> (well, it is to me, but I can see the problem).
> 
> It is regrettable that the interface to the child-spawner needs to
> change, but the kludge of adding stuff to the request_rec was just not
> acceptable (to me, at least - I'm willing to be overruled, though - but
> not without some argument!). I can't see any other way around it that
> isn't a kludge. Note that to make this fix truly correct we must
> eliminated the non-BUFF methods of spawning children (I imagine this is
> also needed for NSPR - Dean?).
> 
> Sorry to all I snapped at.
> 
> Cheers,
> 
> Ben.
> 
> -- 
> Ben Laurie            |Phone: +44 (181) 735 0686|  Apache Group member
> Freelance Consultant  |Fax:   +44 (181) 735 0689|http://www.apache.org
> and Technical Director|Email: ben@algroup.co.uk |
> A.L. Digital Ltd,     |Apache-SSL author    http://www.apache-ssl.org/
> London, England.      |"Apache: TDG" http://www.ora.com/catalog/apache
>