httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: trunk/2.4 core output filter is broken
Date Fri, 20 Jan 2012 22:08:08 GMT
On Fri, Jan 20, 2012 at 09:04:30PM +0100, Stefan Fritsch wrote:
> This is a bigger problem. With the attached patch, the core output 
> filter will flush data to the client when it has read more than 64K 
> from the cgi bucket. Then it will setaside the remaining part of the 
> passed brigade for later write completion. Here it hits the second 
> part of the problem: ap_save_brigade() will call apr_bucket_read() on 
> bucket types that don't implement the setaside method. And the cgi 
> bucket doesn't.
> 
> To me, there seem to be two immediate solutions: Either require 
> setaside being implemented for all bucket types or disable async write 
> completion for requests involving such buckets. Or am I missing 
> something?

I think you are correct, and I don't see how it is feasible to require 
that setaside "works" for all bucket types, it would be a fundamental 
API change.  Maybe 6 years ago...

I am struggling to understand this code, I'm sure I am missing something 
too.

The big loop aims to segregrate the brigade into two parts, "buckets 
which must be written before returning" and "buckets which can be 
buffered".  (Right?)

If we assume that morphing buckets cannot be buffered, the code could be 
adjusted to always place them in the "to flush" segment, and then there 
is no need to read the buckets until they need to be sent, as in the 
patch below.  This seems to fix the memory consumption behaviour without 
obviously breaking anything else (cough cough).

send_brigade_nonblocking() also needs to be fixed to use a non-blocking 
bucket read, but that is a separate issue.

Good catch on ctx->bytes_in. I'd add: why is core_output_filter_ctx_t in 
a public header?

Index: server/core_filters.c
===================================================================
--- server/core_filters.c	(revision 1233882)
+++ server/core_filters.c	(working copy)
@@ -365,7 +365,7 @@
     apr_bucket_brigade *bb = NULL;
     apr_bucket *bucket, *next, *flush_upto = NULL;
     apr_size_t bytes_in_brigade, non_file_bytes_in_brigade;
-    int eor_buckets_in_brigade;
+    int eor_buckets_in_brigade, morphing_bucket_in_brigade;
     apr_status_t rv;
 
     /* Fail quickly if the connection has already been aborted. */
@@ -466,40 +466,40 @@
     bytes_in_brigade = 0;
     non_file_bytes_in_brigade = 0;
     eor_buckets_in_brigade = 0;
+    morphing_bucket_in_brigade = 0;
+
     for (bucket = APR_BRIGADE_FIRST(bb); bucket != APR_BRIGADE_SENTINEL(bb);
          bucket = next) {
         next = APR_BUCKET_NEXT(bucket);
 
         if (!APR_BUCKET_IS_METADATA(bucket)) {
             if (bucket->length == (apr_size_t)-1) {
-                const char *data;
-                apr_size_t length;
-                /* XXX support nonblocking read here? */
-                rv = apr_bucket_read(bucket, &data, &length, APR_BLOCK_READ);
-                if (rv != APR_SUCCESS) {
-                    return rv;
+                morphing_bucket_in_brigade = 1;
+            }
+            else {
+                bytes_in_brigade += bucket->length;
+
+                if (!APR_BUCKET_IS_FILE(bucket)) {
+                    non_file_bytes_in_brigade += bucket->length;
                 }
-                /* reading may have split the bucket, so recompute next: */
-                next = APR_BUCKET_NEXT(bucket);
             }
-            bytes_in_brigade += bucket->length;
-            if (!APR_BUCKET_IS_FILE(bucket)) {
-                non_file_bytes_in_brigade += bucket->length;
-            }
         }
         else if (AP_BUCKET_IS_EOR(bucket)) {
             eor_buckets_in_brigade++;
         }
 
-        if (APR_BUCKET_IS_FLUSH(bucket)                         ||
-            (non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER) ||
-            (eor_buckets_in_brigade > MAX_REQUESTS_IN_PIPELINE) )
-        {
+        if (APR_BUCKET_IS_FLUSH(bucket) 
+            || non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER
+            || morphing_bucket_in_brigade
+            || eor_buckets_in_brigade > MAX_REQUESTS_IN_PIPELINE) {
+            /* this segment of the brigade MUST be sent before returning. */
+
             if (APLOGctrace6(c)) {
                 char *reason = APR_BUCKET_IS_FLUSH(bucket) ?
                                "FLUSH bucket" :
                                (non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER) ?
                                "THRESHOLD_MAX_BUFFER" :
+                               morphing_bucket_in_brigade ? "morphing bucket" :
                                "MAX_REQUESTS_IN_PIPELINE";
                 ap_log_cerror(APLOG_MARK, APLOG_TRACE6, 0, c,
                               "core_output_filter: flushing because of %s",
@@ -513,6 +513,7 @@
             bytes_in_brigade = 0;
             non_file_bytes_in_brigade = 0;
             eor_buckets_in_brigade = 0;
+            morphing_bucket_in_brigade = 0;
         }
     }
 

Mime
View raw message