You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by rb...@covalent.net on 2001/06/12 22:55:13 UTC
Re: cvs commit: httpd-2.0 CHANGES
-1. We have discussed this on list multiple times. The solution is to
use the setaside pool argument that Greg Stein just added. This patch is
as bad as the original hack (using c->pool for files), because it creates
a huge resource leak. Imagine a simple SSI file with twenty small
included files, you just leaked 20 files until the end of the request.
This gets worse if the number of included files is larger, and other
modules may make the problem even worse.
Ryan
On 12 Jun 2001 gregames@apache.org wrote:
> gregames 01/06/12 12:03:09
>
> Modified: server core.c
> . CHANGES
> Log:
> Fix seg faults and/or missing output from mod_include. The
> default_handler was using the subrequest pool for files and
> MMAPs, even though the associated APR structures typically
> live longer than the subrequest.
>
> Revision Changes Path
> 1.18 +3 -1 httpd-2.0/server/core.c
>
> Index: core.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/server/core.c,v
> retrieving revision 1.17
> retrieving revision 1.18
> diff -u -d -b -u -r1.17 -r1.18
> --- core.c 2001/06/08 16:39:48 1.17
> +++ core.c 2001/06/12 19:03:04 1.18
> @@ -2920,6 +2920,7 @@
> * when the charset is translated).
> */
> int bld_content_md5;
> + apr_pool_t *main_pool;
>
> /*
> * The old way of doing handlers meant that this handler would
> @@ -2964,8 +2965,9 @@
> if (r->method_number != M_GET && r->method_number != M_POST) {
> return HTTP_METHOD_NOT_ALLOWED;
> }
> + main_pool = (r->main) ? (r->main->pool) : (r->pool);
>
> - if ((status = apr_file_open(&fd, r->filename, APR_READ | APR_BINARY, 0, r->pool)) != APR_SUCCESS) {
> + if ((status = apr_file_open(&fd, r->filename, APR_READ | APR_BINARY, 0, main_pool)) != APR_SUCCESS) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r,
> "file permissions deny server access: %s", r->filename);
> return HTTP_FORBIDDEN;
>
>
>
> 1.225 +5 -0 httpd-2.0/CHANGES
>
> Index: CHANGES
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/CHANGES,v
> retrieving revision 1.224
> retrieving revision 1.225
> diff -u -d -b -u -r1.224 -r1.225
> --- CHANGES 2001/06/12 17:05:55 1.224
> +++ CHANGES 2001/06/12 19:03:08 1.225
> @@ -1,4 +1,9 @@
> Changes with Apache 2.0.19-dev
> + *) Fix seg faults and/or missing output from mod_include. The
> + default_handler was using the subrequest pool for files and
> + MMAPs, even though the associated APR structures typically
> + live longer than the subrequest. [Greg Ames]
> +
> *) Extend mod_setenvif to support specifying regular expressions
> on the SetEnvIf (and SetEnvIfNoCase) directive attribute field.
> Example: SetEnvIf ^TS* [a-z].* HAVE_TS
>
>
>
>
>
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: httpd-2.0 CHANGES
Posted by Jeff Trawick <tr...@bellsouth.net>.
"Bill Stoddard" <bi...@wstoddard.com> writes:
> I agree with both Greg Ames and Ryan. I would like to keep this fix in for now, even
> though it is not perfect. It is certainly not nearly as bad as using c->pool. When the
> setaside pool is ready, take it out. This keeps us working and able to debug other
> problems in mod_include untill then. We may even be able to roll
> 2.0.19 late this week.
I'd like to see it stay in until we get the setaside working. It is
as easy as can be to back out when the time comes.
(cool... my e-mail works again!)
--
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: cvs commit: httpd-2.0 CHANGES
Posted by Bill Stoddard <bi...@wstoddard.com>.
I agree with both Greg Ames and Ryan. I would like to keep this fix in for now, even
though it is not perfect. It is certainly not nearly as bad as using c->pool. When the
setaside pool is ready, take it out. This keeps us working and able to debug other
problems in mod_include untill then. We may even be able to roll 2.0.19 late this week.
Bill
> rbb@covalent.net wrote:
> >
> > -1. We have discussed this on list multiple times. The solution is to
> > use the setaside pool argument that Greg Stein just added.
>
> includes have been totally broken since April 29. My patch fixes
> includes, so I believe
> this patch qualifies as making progress toward a usable server.
>
> I agree that the setaside idea has some merits, but it is slower for the
> common case
> (a few small files which turn into MMAPs) because it involves
> re-allocating and
> recreating the apr_blah_t structures. Shouldn't we be optimizing for
> the common cases
> rather than the basket cases?
>
> > This patch is
> > as bad as the original hack (using c->pool for files), because it creates
> > a huge resource leak. Imagine a simple SSI file with twenty small
> > included files, you just leaked 20 files until the end of the request.
>
> How huge are 20 apr_mmap_t's? Big deal if they have to wait for the end
> of the
> request. and how common would it be to have 20 include files anyway?
>
> I believe the patch which broke includes was attempting to handle the
> case where the little apr structures could stay around indefinately, due
> to keepalives. That doesn't happen with my patch.
>
> > This gets worse if the number of included files is larger, and other
> > modules may make the problem even worse.
> >
>
> Let's see some real world examples where this is noticeably bad.
>
> Remember, apache.org has been running fine for ages using the connection
> pool/lifetime for subrequests. I've removed any possible exposure to
> the keepalive problem by always using the main request lifetime.
>
> I would welcome a more elegant solution to the problem that actually
> works, and doesn't slow down the typical use of server side includes.
>
> Greg
>
Re: cvs commit: httpd-2.0 CHANGES
Posted by rb...@covalent.net.
On Tue, 12 Jun 2001, Greg Ames wrote:
> rbb@covalent.net wrote:
> >
> > -1. We have discussed this on list multiple times. The solution is to
> > use the setaside pool argument that Greg Stein just added.
>
> includes have been totally broken since April 29. My patch fixes
> includes, so I believe
> this patch qualifies as making progress toward a usable server.
>
> I agree that the setaside idea has some merits, but it is slower for the
> common case
> (a few small files which turn into MMAPs) because it involves
> re-allocating and
> recreating the apr_blah_t structures. Shouldn't we be optimizing for
> the common cases
> rather than the basket cases?
Forget completely about MMAPs. What about the case where the platform
doesn't support MMAPs? This has nothing to do with optimizing for one
case or another. This has to do with not leaking resources like a sieve.
> > This patch is
> > as bad as the original hack (using c->pool for files), because it creates
> > a huge resource leak. Imagine a simple SSI file with twenty small
> > included files, you just leaked 20 files until the end of the request.
>
> How huge are 20 apr_mmap_t's? Big deal if they have to wait for the end
> of the
> request. and how common would it be to have 20 include files anyway?
>
> I believe the patch which broke includes was attempting to handle the
> case where the little apr structures could stay around indefinately, due
> to keepalives. That doesn't happen with my patch.
Again, this hsa nothing to do with MMAP's. This has to do with file
descriptors. By allocating out of the main pool, you are keeping file
descriptors far longer than you should.
> > This gets worse if the number of included files is larger, and other
> > modules may make the problem even worse.
> >
>
> Let's see some real world examples where this is noticeably bad.
Well, I removed the original hack because some people had posted real
world examples where this was noticably bad.
> Remember, apache.org has been running fine for ages using the connection
> pool/lifetime for subrequests. I've removed any possible exposure to
> the keepalive problem by always using the main request lifetime.
>
> I would welcome a more elegant solution to the problem that actually
> works, and doesn't slow down the typical use of server side includes.
My veto stands on this code. We have documented on this list two other
mechanisms for removing this problem that do not leak file descriptors
like this. This patch just regresses to a leak that we don't need. I
will try to write the setaside logic later tonight for either files or
mmaps to help alleviate this problem.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: httpd-2.0 CHANGES
Posted by "William A. Rowe, Jr." <ad...@rowe-clan.net>.
From: "Greg Ames" <gr...@remulak.net>
Sent: Wednesday, June 13, 2001 9:56 AM
> Cliff Woolley wrote:
>
> > > How huge are 20 apr_mmap_t's? Big deal if they have to wait for the end
> > > of the request. and how common would it be to have 20 include files
> > > anyway?
> >
> > Yeah, but there're other problems, like chewing up too many file handles
> > in pathological cases...
Either way, you are chewing up system handles like they are going out of style.
Most OS's optimize for the 'appropriate' working set, appropriate being defined
by demand. This sort of spikey behavior will circumvent any kernel optimization.
> > > Let's see some real world examples where this is noticeably bad.
> >
> > What about mod_autoindex on a directory with 1000 files?
>
> <gulp> If the little icons on each line turn into subrequests, you've
> got me, for sure.
No. Each file in the directory becomes it's own little temporary subrequest,
to determine if would be served. We do this to assure that no file is included
which wouldn't then be passable. So, we have 1000 subrequests testing for the
validity of the file, and this drops out the .htpasswd, .htaccess, and the
restricted subdirectories.
Each one isn't bad, until you stop freeing them.
The li'l icons themselves? They are just seperate requests (pipelined, kept alive
and/or browser cached, we hope :-)
> I don't think many admins would intentionally turn on autoindexing on
> such a directory - imagine navigating thru this puppy with your favorite
> browser. But if they did, I'm sure that they would rather see output
> than seg faults.
And this is a ticket to segfaults.
> As I think I said, I don't mind a superior solution. But the last time
> I checked, it didn't exist. Seg faults in the core whenever you use
> something as popular as mod_include just don't cut it.
No, they don't. Glad others are already hacking out the correct setaside logic.
I tend to agree with (Dean? Roy?) here ... drag the last 500 bytes from the bucket,
or just send the dang thing down the wire. I'd be happier with a patch that says,
"hmmm... sendfile buckets are basically broken, let's dump it on the wire rather
than setting it aside", and close this bug in favor of poorer performance at this
moment. Fix the sendfile bucket and revert that logic, and we get the performance
back later on.
Bill
Re: cvs commit: httpd-2.0 CHANGES
Posted by rb...@covalent.net.
> > > How huge are 20 apr_mmap_t's? Big deal if they have to wait for the end
> > > of the request. and how common would it be to have 20 include files
> > > anyway?
> >
> > Yeah, but there're other problems, like chewing up too many file handles
> > in pathological cases...
> >
> > > Let's see some real world examples where this is noticeably bad.
> >
> > What about mod_autoindex on a directory with 1000 files?
>
> <gulp> If the little icons on each line turn into subrequests, you've
> got me, for sure.
>
> I don't think many admins would intentionally turn on autoindexing on
> such a directory - imagine navigating thru this puppy with your favorite
> browser. But if they did, I'm sure that they would rather see output
> than seg faults.
>
> As I think I said, I don't mind a superior solution. But the last time
> I checked, it didn't exist. Seg faults in the core whenever you use
> something as popular as mod_include just don't cut it.
The solution wasn't coded, but it was detailed on this list. My veto
still stands, please back out the code. I consider this as hiding a
problem, and a step backwards.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: httpd-2.0 CHANGES
Posted by Greg Ames <gr...@remulak.net>.
Cliff Woolley wrote:
>
>
> > How huge are 20 apr_mmap_t's? Big deal if they have to wait for the end
> > of the request. and how common would it be to have 20 include files
> > anyway?
>
> Yeah, but there're other problems, like chewing up too many file handles
> in pathological cases...
>
> > Let's see some real world examples where this is noticeably bad.
>
> What about mod_autoindex on a directory with 1000 files?
<gulp> If the little icons on each line turn into subrequests, you've
got me, for sure.
I don't think many admins would intentionally turn on autoindexing on
such a directory - imagine navigating thru this puppy with your favorite
browser. But if they did, I'm sure that they would rather see output
than seg faults.
As I think I said, I don't mind a superior solution. But the last time
I checked, it didn't exist. Seg faults in the core whenever you use
something as popular as mod_include just don't cut it.
Greg
Re: cvs commit: httpd-2.0 CHANGES
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 12 Jun 2001, Greg Ames wrote:
> includes have been totally broken since April 29. My patch fixes
> includes, so I believe this patch qualifies as making progress toward
> a usable server.
I don't have a problem with this as a stopgap measure, though I think it
should get replaced with the setaside stuff ASAP.
> recreating the apr_blah_t structures. Shouldn't we be optimizing for
> the common cases rather than the basket cases?
;-)
> How huge are 20 apr_mmap_t's? Big deal if they have to wait for the end
> of the request. and how common would it be to have 20 include files
> anyway?
Yeah, but there're other problems, like chewing up too many file handles
in pathological cases...
> Let's see some real world examples where this is noticeably bad.
What about mod_autoindex on a directory with 1000 files?
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: cvs commit: httpd-2.0 CHANGES
Posted by Greg Ames <gr...@remulak.net>.
rbb@covalent.net wrote:
>
> -1. We have discussed this on list multiple times. The solution is to
> use the setaside pool argument that Greg Stein just added.
includes have been totally broken since April 29. My patch fixes
includes, so I believe
this patch qualifies as making progress toward a usable server.
I agree that the setaside idea has some merits, but it is slower for the
common case
(a few small files which turn into MMAPs) because it involves
re-allocating and
recreating the apr_blah_t structures. Shouldn't we be optimizing for
the common cases
rather than the basket cases?
> This patch is
> as bad as the original hack (using c->pool for files), because it creates
> a huge resource leak. Imagine a simple SSI file with twenty small
> included files, you just leaked 20 files until the end of the request.
How huge are 20 apr_mmap_t's? Big deal if they have to wait for the end
of the
request. and how common would it be to have 20 include files anyway?
I believe the patch which broke includes was attempting to handle the
case where the little apr structures could stay around indefinately, due
to keepalives. That doesn't happen with my patch.
> This gets worse if the number of included files is larger, and other
> modules may make the problem even worse.
>
Let's see some real world examples where this is noticeably bad.
Remember, apache.org has been running fine for ages using the connection
pool/lifetime for subrequests. I've removed any possible exposure to
the keepalive problem by always using the main request lifetime.
I would welcome a more elegant solution to the problem that actually
works, and doesn't slow down the typical use of server side includes.
Greg