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/07/04 05:15:58 UTC
[PATCH] Fix CGI flush to network problem
cvs diff -u protocol.c
Index: protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/protocol.c,v
Here is an interesting patch. I don't think this is quite what we should commit, but it
was entertaining to see how well this exercised the chunk filter. This patch does a
non-blocking read each and every time and only falls back to a blocking read when we get
APR_EAGAIN on a non-blocking read. We get wildly varying number of bytes read on each
non-blocking read :-). The 'correct' fix is to do a non-blocking read once then make
subsequent reads blocking. Will commit something tomorrow.
Bill
retrieving revision 1.28
diff -u -r1.28 protocol.c
--- protocol.c 2001/06/27 20:18:09 1.28
+++ protocol.c 2001/07/04 02:58:13
@@ -868,8 +868,23 @@
send_it = 1;
}
if (e->length == -1) { /* if length unknown */
- rv = apr_bucket_read(e, &ignored, &length, APR_BLOCK_READ);
+ rv = apr_bucket_read(e, &ignored, &length, APR_NONBLOCK_READ);
+ if (rv == APR_EAGAIN) {
+ /* If we can chunk the output, flush the filter chain to
+ * the network then do a blocking read.
+ */
+ if (r->proto_num >= HTTP_VERSION(1,1)) {
+ apr_bucket_brigade *split;
+ split = apr_brigade_split(b, e);
+ rv = ap_fflush(f, b);
+ if (rv != APR_SUCCESS)
+ return rv;
+ b = split;
+ }
+ rv = apr_bucket_read(e, &ignored, &length, APR_BLOCK_READ);
+ }
if (rv != APR_SUCCESS) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, "ap_content_length_filter:
apr_bucket_read() failed");
return rv;
}
}
Re: [PATCH] Fix CGI flush to network problem
Posted by Cliff Woolley <cl...@yahoo.com>.
On Wed, 4 Jul 2001, Bill Stoddard wrote:
> Yea, I saw your comments. I'll modify the patch to check for
> APR_SUCCESS with no bytes read and on a nonblocking call, interpret
> that as APR_EAGAIN.
Hmmm, no, you shouldn't need to do that. If APR_SUCCESS is returned from
apr_bucket_read(), regardless of whether any bytes were read or not or
whether it was a nonblocking read or not, that means that the bucket is
empty and that you should move on to the next bucket. Only APR_EAGAIN on
nonblocking reads means you need to retry.
What I was saying is that right now, the pipe/socket buckets in
nonblocking mode will return APR_EOF if they received APR_EOF from APR and
zero bytes were read. That's wrong. They should just tell the caller of
apr_bucket_read() "okay, that's it... move on. APR_SUCCESS." You
shouldn't need to change your patch at all.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: [PATCH] Fix CGI flush to network problem
Posted by Bill Stoddard <bi...@wstoddard.com>.
Yea, I saw your comments. I'll modify the patch to check for APR_SUCCESS with no bytes
read and on a nonblocking call, interpret that as APR_EAGAIN.
Bill
----- Original Message -----
From: "Cliff Woolley" <cl...@yahoo.com>
To: <ne...@apache.org>
Sent: Wednesday, July 04, 2001 1:07 AM
Subject: Re: [PATCH] Fix CGI flush to network problem
> On Tue, 3 Jul 2001, Bill Stoddard wrote:
>
> > if (e->length == -1) { /* if length unknown */
> > - rv = apr_bucket_read(e, &ignored, &length, APR_BLOCK_READ);
> > + rv = apr_bucket_read(e, &ignored, &length, APR_NONBLOCK_READ);
> > + if (rv == APR_EAGAIN) {
> > + /* If we can chunk the output, flush the filter chain to
> > + * the network then do a blocking read.
> > + */
> > + if (r->proto_num >= HTTP_VERSION(1,1)) {
> > + apr_bucket_brigade *split;
> > + split = apr_brigade_split(b, e);
> > + rv = ap_fflush(f, b);
> > + if (rv != APR_SUCCESS)
> > + return rv;
> > + b = split;
> > + }
> > + rv = apr_bucket_read(e, &ignored, &length, APR_BLOCK_READ);
> > + }
> > if (rv != APR_SUCCESS) {
> > + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
"ap_content_length_filter:
> > apr_bucket_read() failed");
> > return rv;
>
> <aside>
>
> This patch brings back up the problem of bucket types (pipe, socket) that
> can return APR_EOF when nonblocking reads are used, when they really ought
> to return APR_SUCCESS. The fact that they don't would break this patch
> (the buckets' fault, not this patch's). Granted, all that will happen in
> this particular case is that you'll get a meaningless error message, but
> that's beside the point.
>
> We agreed on the list months and months ago that they should return
> APR_SUCCESS and just quietly remove themselves from the brigade once they
> were empty. I put a comment that this change needed to be made in the
> buckets code ages ago, but chickened out on actually making the change
> because I was afraid it would break Apache and I wouldn't have known
> exactly where to look to fix it (possibly in the http input filter?
> dunno).
>
> Anyway, We need to go ahead and take care of that so that seemingly
> harmless patches like this don't come back and bite us.
>
> </aside>
>
> --Cliff
>
>
> --------------------------------------------------------------
> Cliff Woolley
> cliffwoolley@yahoo.com
> Charlottesville, VA
>
>
Re: [PATCH] Fix CGI flush to network problem
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 3 Jul 2001, Bill Stoddard wrote:
> if (e->length == -1) { /* if length unknown */
> - rv = apr_bucket_read(e, &ignored, &length, APR_BLOCK_READ);
> + rv = apr_bucket_read(e, &ignored, &length, APR_NONBLOCK_READ);
> + if (rv == APR_EAGAIN) {
> + /* If we can chunk the output, flush the filter chain to
> + * the network then do a blocking read.
> + */
> + if (r->proto_num >= HTTP_VERSION(1,1)) {
> + apr_bucket_brigade *split;
> + split = apr_brigade_split(b, e);
> + rv = ap_fflush(f, b);
> + if (rv != APR_SUCCESS)
> + return rv;
> + b = split;
> + }
> + rv = apr_bucket_read(e, &ignored, &length, APR_BLOCK_READ);
> + }
> if (rv != APR_SUCCESS) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, "ap_content_length_filter:
> apr_bucket_read() failed");
> return rv;
<aside>
This patch brings back up the problem of bucket types (pipe, socket) that
can return APR_EOF when nonblocking reads are used, when they really ought
to return APR_SUCCESS. The fact that they don't would break this patch
(the buckets' fault, not this patch's). Granted, all that will happen in
this particular case is that you'll get a meaningless error message, but
that's beside the point.
We agreed on the list months and months ago that they should return
APR_SUCCESS and just quietly remove themselves from the brigade once they
were empty. I put a comment that this change needed to be made in the
buckets code ages ago, but chickened out on actually making the change
because I was afraid it would break Apache and I wouldn't have known
exactly where to look to fix it (possibly in the http input filter?
dunno).
Anyway, We need to go ahead and take care of that so that seemingly
harmless patches like this don't come back and bite us.
</aside>
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA