httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ben Laurie <...@algroup.co.uk>
Subject Re: [PATCH] NT CGI fix
Date Sat, 16 May 1998 16:19:42 GMT
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

Mime
View raw message