You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Bill Stoddard <bi...@wstoddard.com> on 2001/05/01 20:00:47 UTC
Re: cvs commit: httpd-2.0/server core.c
This patch is seriously broken. Request a very large file (100MB or greater) and watch what happens
to memory usage.
The problem is this loop. We basically read the entire content of the file into memory before
sending it out on the network. Haven't given much though on the best way to fix this.
> + APR_BRIGADE_FOREACH(bucket, b) {
> + const char *str;
> + apr_size_t n;
> +
> + rv = apr_bucket_read(bucket, &str, &n, APR_BLOCK_READ);
> + apr_brigade_write(ctx->b, NULL, NULL, str, n);
> + }
Bill
----- Original Message -----
From: <rb...@apache.org>
To: <ht...@apache.org>
Sent: Sunday, April 29, 2001 1:05 PM
Subject: cvs commit: httpd-2.0/server core.c
> rbb 01/04/29 10:05:50
>
> Modified: . CHANGES
> server core.c
> Log:
> Create Files, and thus MMAPs, out of the request pool, not the
> connection pool. This solves a small resource leak that had us
> not closing files until a connection was closed. In order to do
> this, at the end of the core_output_filter, we loop through the
> brigade and convert any data we have into a single HEAP bucket
> that we know will survive clearing the request_rec.
>
> Submitted by: Ryan Bloom, Justin Erenkrantz <je...@ebuilt.com>,
> Cliff Woolley
>
> Revision Changes Path
> 1.190 +9 -0 httpd-2.0/CHANGES
>
> Index: CHANGES
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/CHANGES,v
> retrieving revision 1.189
> retrieving revision 1.190
> diff -u -d -b -w -u -r1.189 -r1.190
> --- CHANGES 2001/04/29 06:45:34 1.189
> +++ CHANGES 2001/04/29 17:05:49 1.190
> @@ -1,5 +1,14 @@
> Changes with Apache 2.0.18-dev
>
> + *) Create Files, and thus MMAPs, out of the request pool, not the
> + connection pool. This solves a small resource leak that had us
> + not closing files until a connection was closed. In order to do
> + this, at the end of the core_output_filter, we loop through the
> + brigade and convert any data we have into a single HEAP bucket
> + that we know will survive clearing the request_rec.
> + [Ryan Bloom, Justin Erenkrantz <je...@ebuilt.com>,
> + Cliff Woolley]
> +
> *) Completely revamp configure so that it preserves the standard make
> variables CPPFLAGS, CFLAGS, CXXFLAGS, LDFLAGS and LIBS by moving
> the configure additions to EXTRA_* variables. Also, allow the user
>
>
>
> 1.10 +21 -4 httpd-2.0/server/core.c
>
> Index: core.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/server/core.c,v
> retrieving revision 1.9
> retrieving revision 1.10
> diff -u -d -b -w -u -r1.9 -r1.10
> --- core.c 2001/04/22 22:19:32 1.9
> +++ core.c 2001/04/29 17:05:49 1.10
> @@ -2965,7 +2965,7 @@
> return HTTP_METHOD_NOT_ALLOWED;
> }
>
> - if ((status = apr_file_open(&fd, r->filename, APR_READ | APR_BINARY, 0,
r->connection->pool)) != APR_SUCCESS) {
> + if ((status = apr_file_open(&fd, r->filename, APR_READ | APR_BINARY, 0, r->pool)) !=
APR_SUCCESS) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r,
> "file permissions deny server access: %s", r->filename);
> return HTTP_FORBIDDEN;
> @@ -3134,15 +3134,32 @@
> if ((!fd && !more &&
> (nbytes < AP_MIN_BYTES_TO_WRITE) && !APR_BUCKET_IS_FLUSH(e))
> || (APR_BUCKET_IS_EOS(e) && c->keepalive)) {
> -
> /* NEVER save an EOS in here. If we are saving a brigade with
> * an EOS bucket, then we are doing keepalive connections, and
> * we want to process to second request fully.
> */
> if (APR_BUCKET_IS_EOS(e)) {
> - apr_bucket_delete(e);
> + apr_bucket *bucket = NULL;
> + /* If we are in here, then this request is a keepalive. We
> + * need to be certain that any data in a bucket is valid
> + * after the request_pool is cleared.
> + */
> + if (ctx->b == NULL) {
> + ctx->b = apr_brigade_create(f->c->pool);
> }
> +
> + APR_BRIGADE_FOREACH(bucket, b) {
> + const char *str;
> + apr_size_t n;
> +
> + rv = apr_bucket_read(bucket, &str, &n, APR_BLOCK_READ);
> + apr_brigade_write(ctx->b, NULL, NULL, str, n);
> + }
> + apr_brigade_destroy(b);
> + }
> + else {
> ap_save_brigade(f, &ctx->b, &b);
> + }
> return APR_SUCCESS;
> }
>
>
>
>
Re: cvs commit: httpd-2.0/server core.c
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 1 May 2001, Bill Stoddard wrote:
> This patch is seriously broken. Request a very large file (100MB or
> greater) and watch what happens to memory usage.
>
> The problem is this loop. We basically read the entire content of the
> file into memory before sending it out on the network. Haven't given
> much though on the best way to fix this.
>
> > + APR_BRIGADE_FOREACH(bucket, b) {
> > + const char *str;
> > + apr_size_t n;
> > +
> > + rv = apr_bucket_read(bucket, &str, &n, APR_BLOCK_READ);
> > + apr_brigade_write(ctx->b, NULL, NULL, str, n);
> > + }
Hmmm... that's not what the patch was supposed to do at all. It was
supposed to read the file into memory, yes, but ONLY the "leftover" part
of the file, which is essentially (filesize % AP_MIN_BYTES_TO_SEND) bytes.
If it really is reading in the whole file, then you're right, it's broken.
I'll try to look into this later today, but no guarantees.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: cvs commit: httpd-2.0/server core.c
Posted by Bill Stoddard <bi...@wstoddard.com>.
> On Tue, May 01, 2001 at 12:16:21PM -0700, rbb@covalent.net wrote:
> >
> > > > > if ((!fd && !more &&
> > > > > (nbytes < AP_MIN_BYTES_TO_WRITE) && !APR_BUCKET_IS_FLUSH(e))
> > > > > || (APR_BUCKET_IS_EOS(e) && c->keepalive)) {
> > > > >
> > > > > I think the logic in the conditional is just wrong.
> > > >
> > > > I agree completely. I think I can fix this in a few minutes. Watch for a
> > > > patch.
> > >
> > > Hmm. It seems that we'd just want to completely skip the whole thing if fd
> > > has something in it. So the conditional might be:
> > >
> > > if (!fd && ((!more && nbytes < AP_MIN_BYTES_TO_WRITE
> > > && !APR_BUCKET_IS_FLUSH(e))
> > > || (APR_BUCKET_IS_EOS(e) && c->keepalive)))
> > >
> > > Does that seem right?
> >
> > I don't think that is enough. We need to also make sure that we don't
> > have nbytes >= AP_MIN_BYTES_TO_WRITE. The problem is that last
> > conditional is really just kind of bogus. I am working on a patch, and
> > should have it soon-ish.
>
> We don't count bytes for the files, so that doesn't quite work (which is why
> I did the !fd thang).
>
> However... now that I think about it. We *should* count bytes for the files.
> We have the lower threshold for use of sendfile(), but we shouldn't write to
> the network for, say, a 500 byte file and 10 bytes of other content.
>
> To clarify what I'm trying to say:
>
> Let's say that we have 200 bytes of headers and a 500 byte file. If we just
> key off of the file, then we'd end up doing a sendfile with 700 bytes total
> content. Bummer.
>
Only a bummer if the next request if pipelined. If I catch what you are bummed about :-)
Bill
Re: cvs commit: httpd-2.0/server core.c
Posted by rb...@covalent.net.
> To clarify what I'm trying to say:
>
> Let's say that we have 200 bytes of headers and a 500 byte file. If we just
> key off of the file, then we'd end up doing a sendfile with 700 bytes total
> content. Bummer.
>
> So... that means we need to add a line in the FILE bucket stuff:
>
> nbytes += flen;
>
> That should allow us to key on nbytes like you suggest.
This was my first approach. It ends up triggering a few asserts later on,
because of how we call sendfile_it_all. It ends up being easier to just
do the addition in the if statement.
> Note that the last conditional is fine *IFF* you also check nbytes.
Yep. Which is what I did.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: httpd-2.0/server core.c
Posted by Greg Stein <gs...@lyra.org>.
On Tue, May 01, 2001 at 12:16:21PM -0700, rbb@covalent.net wrote:
>
> > > > if ((!fd && !more &&
> > > > (nbytes < AP_MIN_BYTES_TO_WRITE) && !APR_BUCKET_IS_FLUSH(e))
> > > > || (APR_BUCKET_IS_EOS(e) && c->keepalive)) {
> > > >
> > > > I think the logic in the conditional is just wrong.
> > >
> > > I agree completely. I think I can fix this in a few minutes. Watch for a
> > > patch.
> >
> > Hmm. It seems that we'd just want to completely skip the whole thing if fd
> > has something in it. So the conditional might be:
> >
> > if (!fd && ((!more && nbytes < AP_MIN_BYTES_TO_WRITE
> > && !APR_BUCKET_IS_FLUSH(e))
> > || (APR_BUCKET_IS_EOS(e) && c->keepalive)))
> >
> > Does that seem right?
>
> I don't think that is enough. We need to also make sure that we don't
> have nbytes >= AP_MIN_BYTES_TO_WRITE. The problem is that last
> conditional is really just kind of bogus. I am working on a patch, and
> should have it soon-ish.
We don't count bytes for the files, so that doesn't quite work (which is why
I did the !fd thang).
However... now that I think about it. We *should* count bytes for the files.
We have the lower threshold for use of sendfile(), but we shouldn't write to
the network for, say, a 500 byte file and 10 bytes of other content.
To clarify what I'm trying to say:
Let's say that we have 200 bytes of headers and a 500 byte file. If we just
key off of the file, then we'd end up doing a sendfile with 700 bytes total
content. Bummer.
So... that means we need to add a line in the FILE bucket stuff:
nbytes += flen;
That should allow us to key on nbytes like you suggest.
Note that the last conditional is fine *IFF* you also check nbytes.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server core.c
Posted by rb...@covalent.net.
> > > if ((!fd && !more &&
> > > (nbytes < AP_MIN_BYTES_TO_WRITE) && !APR_BUCKET_IS_FLUSH(e))
> > > || (APR_BUCKET_IS_EOS(e) && c->keepalive)) {
> > >
> > > I think the logic in the conditional is just wrong.
> >
> > I agree completely. I think I can fix this in a few minutes. Watch for a
> > patch.
>
> Hmm. It seems that we'd just want to completely skip the whole thing if fd
> has something in it. So the conditional might be:
>
> if (!fd && ((!more && nbytes < AP_MIN_BYTES_TO_WRITE
> && !APR_BUCKET_IS_FLUSH(e))
> || (APR_BUCKET_IS_EOS(e) && c->keepalive)))
>
> Does that seem right?
I don't think that is enough. We need to also make sure that we don't
have nbytes >= AP_MIN_BYTES_TO_WRITE. The problem is that last
conditional is really just kind of bogus. I am working on a patch, and
should have it soon-ish.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: httpd-2.0/server core.c
Posted by Greg Stein <gs...@lyra.org>.
On Tue, May 01, 2001 at 11:13:10AM -0700, rbb@covalent.net wrote:
>...
> > 1 heap bucket containing the headers
> > 1 file bucket with the file descriptor
> > 1 eos bucket
> >
> > The following code is hit and we enter the conditional because the last bucket was an eos and the
> > connection is keep-alive.
> >
> > if ((!fd && !more &&
> > (nbytes < AP_MIN_BYTES_TO_WRITE) && !APR_BUCKET_IS_FLUSH(e))
> > || (APR_BUCKET_IS_EOS(e) && c->keepalive)) {
> >
> > I think the logic in the conditional is just wrong.
>
> I agree completely. I think I can fix this in a few minutes. Watch for a
> patch.
Hmm. It seems that we'd just want to completely skip the whole thing if fd
has something in it. So the conditional might be:
if (!fd && ((!more && nbytes < AP_MIN_BYTES_TO_WRITE
&& !APR_BUCKET_IS_FLUSH(e))
|| (APR_BUCKET_IS_EOS(e) && c->keepalive)))
Does that seem right?
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server core.c
Posted by rb...@covalent.net.
> > > This patch is seriously broken. Request a very large file (100MB or greater) and watch what
> happens
> > > to memory usage.
> > >
> > > The problem is this loop. We basically read the entire content of the file into memory before
> > > sending it out on the network. Haven't given much though on the best way to fix this.
> > >
> > > > + APR_BRIGADE_FOREACH(bucket, b) {
> > > > + const char *str;
> > > > + apr_size_t n;
> > > > +
> > > > + rv = apr_bucket_read(bucket, &str, &n, APR_BLOCK_READ);
> > > > + apr_brigade_write(ctx->b, NULL, NULL, str, n);
> > > > + }
> >
> > I don't see how that could happen. We only enter that section of the
> > core_output_filter if we are saving some data off to the side for
> > keepalive requests. In fact, we specifically do not enter this loop if we
> > are serving a file from disk.
>
> Attach a debugger and watch what happens. I am seeing the following buckets...
>
> 1 heap bucket containing the headers
> 1 file bucket with the file descriptor
> 1 eos bucket
>
> The following code is hit and we enter the conditional because the last bucket was an eos and the
> connection is keep-alive.
>
> if ((!fd && !more &&
> (nbytes < AP_MIN_BYTES_TO_WRITE) && !APR_BUCKET_IS_FLUSH(e))
> || (APR_BUCKET_IS_EOS(e) && c->keepalive)) {
>
> I think the logic in the conditional is just wrong.
I agree completely. I think I can fix this in a few minutes. Watch for a
patch.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: httpd-2.0/server core.c
Posted by Bill Stoddard <bi...@wstoddard.com>.
> On Tue, 1 May 2001, Bill Stoddard wrote:
>
> > This patch is seriously broken. Request a very large file (100MB or greater) and watch what
happens
> > to memory usage.
> >
> > The problem is this loop. We basically read the entire content of the file into memory before
> > sending it out on the network. Haven't given much though on the best way to fix this.
> >
> > > + APR_BRIGADE_FOREACH(bucket, b) {
> > > + const char *str;
> > > + apr_size_t n;
> > > +
> > > + rv = apr_bucket_read(bucket, &str, &n, APR_BLOCK_READ);
> > > + apr_brigade_write(ctx->b, NULL, NULL, str, n);
> > > + }
>
> I don't see how that could happen. We only enter that section of the
> core_output_filter if we are saving some data off to the side for
> keepalive requests. In fact, we specifically do not enter this loop if we
> are serving a file from disk.
Attach a debugger and watch what happens. I am seeing the following buckets...
1 heap bucket containing the headers
1 file bucket with the file descriptor
1 eos bucket
The following code is hit and we enter the conditional because the last bucket was an eos and the
connection is keep-alive.
if ((!fd && !more &&
(nbytes < AP_MIN_BYTES_TO_WRITE) && !APR_BUCKET_IS_FLUSH(e))
|| (APR_BUCKET_IS_EOS(e) && c->keepalive)) {
I think the logic in the conditional is just wrong.
Bill
Re: cvs commit: httpd-2.0/server core.c
Posted by rb...@covalent.net.
On Tue, 1 May 2001, Bill Stoddard wrote:
> This patch is seriously broken. Request a very large file (100MB or greater) and watch what happens
> to memory usage.
>
> The problem is this loop. We basically read the entire content of the file into memory before
> sending it out on the network. Haven't given much though on the best way to fix this.
>
> > + APR_BRIGADE_FOREACH(bucket, b) {
> > + const char *str;
> > + apr_size_t n;
> > +
> > + rv = apr_bucket_read(bucket, &str, &n, APR_BLOCK_READ);
> > + apr_brigade_write(ctx->b, NULL, NULL, str, n);
> > + }
I don't see how that could happen. We only enter that section of the
core_output_filter if we are saving some data off to the side for
keepalive requests. In fact, we specifically do not enter this loop if we
are serving a file from disk.
The only way this could be the culprit, is if we have MMAP'ed the file,
and we didn't send it out for some reason. I will try to look at this
today to see the problem.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------