[PATCH] NT CGI fix

[W G Stoddard <wg...@us.ibm.com>]

Ref PRs 1607 and 1129

Here is a fix for the WIN32 CGI hang problem, implemented using native WIN32
calls.
This fix has been stressed for over 24 hours under heavy load and with no hangs
or noticeable
storage leaks. Was able to recreate the problem in the unpatched code using
sleepcgi.
(Thanks Bill O'Donnell!)

There is a bit of uglyness in the patch I'm not very happy with; added three
new fields to
request_rec.  Unix 'spawn' (fork and exec) allows the child process to inherit
the parent's
open file descriptors.  CreateProcess does not, so we must explicitly pass
handles for stdin,
stdout and stderr on the CreateProcess call.  Created pipes in mod_cgi.c, saved
the pipe handles
in struct request_rec (uugh!) and picked them up off the request structure in
ap_call_exec.
Not pretty.There are some architectural difficulties that I hope can be fixed
in the 2.0 release.

I'm just learning Apache and am interested in any suggestions or comments the
group may have.

Thanks,
Bill Stoddard (wgstodda@us.ibm.com)


Users of the Apache webserver are hereby granted a non-exclusive, irrevocable,
world-wide, royalty-free, non-transferable license to use, execute, prepare
derivative works of, and distribute (internally and externally, and including
derivative works) the code accompanying this license as part of, and integrated
into the Apache webserver.  This code is provided "AS IS" WITHOUT WARRANTY OF
ANY KIND EITHER EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE IMPLIED
WARRANTY OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE.  THE ENTIRE
RISK ARISING OUT OF THE USE OR PERFORMANCE OF THIS CODE REMAINS WITH USERS OF
THE APACHE WEBSERVER.  I represent and warrant that I am legally entitled to
grant the above license.

Re: [PATCH] NT CGI fix

[Dean Gaudet <dg...@arctic.org>]

On Sat, 16 May 1998, Ben Laurie wrote:

> 6. With all that out of the way, I decided to disable the other methods
> of spawning children (since they are unsafe, after all). This leads to
> other interesting problems, like piped logs and mod_rewrite no longer
> working, so I've enabled them again for now. However, they will need to
> be eliminated.

Gee, have you looked at apache-nspr?  I've already dealt with all of this.
i.e. you're doing 2.0 things in 1.3.

> 7. Now the good news: it seems to work. I'll be committing what I've
> done shortly (I hope).

I hope not.  If you've broken all that stuff then I certainly hope you
didn't committ it.

> Did I ever mention this would all be much neater in C++? :-)

And gee, it would be so much neater if we got to redesign the C interface
as well?  If you're going to compare apples to apples you can't compare an
unchanged, legacy, crappy C interface to a newly designed C++ interface.

Dean

Re: [PATCH] NT CGI fix

[Ben Laurie <be...@algroup.co.uk>]

Dirk-Willem van Gulik wrote:
> 
> On Sat, 16 May 1998, Ben Laurie wrote:
> 
> > > Ref PRs 1607 and 1129
> > >
> > > Here is a fix for the WIN32 CGI hang problem, implemented using native WIN32
> > > calls.
> > > This fix has been stressed for over 24 hours under heavy load and with no hangs
> > > or noticeable
> > > storage leaks. Was able to recreate the problem in the unpatched code using
> > > sleepcgi.
> 
> Asides from Bens coding comments; it does certainly seem to fix the
> problem. We've tried it on a difficult site; and it has survived the night
> very, very, very well.

Dirk - want to try my version (i.e. the current repository), or was it
that version you tried anyway?

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: [PATCH] NT CGI fix

[Dirk-Willem van Gulik <di...@jrc.it>]

On Sat, 16 May 1998, Ben Laurie wrote:

> > Ref PRs 1607 and 1129
> > 
> > Here is a fix for the WIN32 CGI hang problem, implemented using native WIN32
> > calls.
> > This fix has been stressed for over 24 hours under heavy load and with no hangs
> > or noticeable
> > storage leaks. Was able to recreate the problem in the unpatched code using
> > sleepcgi.

Asides from Bens coding comments; it does certainly seem to fix the
problem. We've tried it on a difficult site; and it has survived the night
very, very, very well. 

Dw.

Re: [PATCH] NT CGI fix

[Ben Laurie <be...@algroup.co.uk>]

W G Stoddard wrote:
> 
> Ref PRs 1607 and 1129
> 
> Here is a fix for the WIN32 CGI hang problem, implemented using native WIN32
> calls.
> This fix has been stressed for over 24 hours under heavy load and with no hangs
> or noticeable
> storage leaks. Was able to recreate the problem in the unpatched code using
> sleepcgi.
> (Thanks Bill O'Donnell!)
> 
> There is a bit of uglyness in the patch I'm not very happy with; added three
> new fields to
> request_rec.  Unix 'spawn' (fork and exec) allows the child process to inherit
> the parent's
> open file descriptors.  CreateProcess does not, so we must explicitly pass
> handles for stdin,
> stdout and stderr on the CreateProcess call.  Created pipes in mod_cgi.c, saved
> the pipe handles
> in struct request_rec (uugh!) and picked them up off the request structure in
> ap_call_exec.
> Not pretty.There are some architectural difficulties that I hope can be fixed
> in the 2.0 release.
> 
> I'm just learning Apache and am interested in any suggestions or comments the
> group may have.

I'm reviewing the patch now, and have both some general and specific
comments:

1. Rearranging/moving around comments without good reason is not usually
done.

2. I didn't like the name hDevice, coz it isn't a device, so I replaced
it with hFH (for file handle), which I don't like either, but at least
it isn't misleading.

3. The style "if(NULL == x)" is not the way we do it; "if(x == NULL)" or
even "if(x)" (in the case of a NULL test) is the way we like it.

4. The nesting of ifs in ap_nt_spawn_child_err_buff() (which should've
been in alloc.c not mod_cgi.c) was positively baroque - what was wrong
with &&? Anyway, I completely restructured this to be more careful about
only creating the needed pipes and cleaning up after errors.

5. Having done all that, I realised that ap_nt_spawn_child_err_buff()
may as well get merged with ap_spawn_child_err_buff(), which I then did.
In the process, I decided I didn't like having the extra handles in
request_rec, so I invented a new structure for them (child_info). This
needs a change to ap_call_exec(), so essentially this is the
ap_call_exec2() mentioned in a comment, but I feel it is better to keep
a single interface even if *nix doesn't need the child_info. This also
has the desirable side-effect of forcing everyone else to do things
properly...

6. With all that out of the way, I decided to disable the other methods
of spawning children (since they are unsafe, after all). This leads to
other interesting problems, like piped logs and mod_rewrite no longer
working, so I've enabled them again for now. However, they will need to
be eliminated.

7. Now the good news: it seems to work. I'll be committing what I've
done shortly (I hope).

Did I ever mention this would all be much neater in C++? :-)

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