OSDir


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: ap_request_core_filter issues


On Thu, Oct 18, 2018 at 1:52 PM Joe Orton <jorton@xxxxxxxxxx> wrote:
>
> On Thu, Oct 18, 2018 at 12:51:06PM +0200, Ruediger Pluem wrote:
> > >>>
> > >>> Curiously inefficient writev use when stracing the process, though,
> > >>> dunno what's going on there (trunk/prefork):
> > >>>
> > >>> writev(46, [{iov_base="\r\n", iov_len=2}], 1) = 2
> > >>> writev(46, [{iov_base="1f84\r\n", iov_len=6}], 1) = 6
> > >>> writev(46, [{iov_base="<D:lockdiscovery/>\n<D:getcontent"..., iov_len=7820}], 1) = 7820
> > >>> writev(46, [{iov_base="<D:supportedlock>\n<D:lockentry>\n"..., iov_len=248}], 1) = 248
> > >>
> > >> The reason is ap_request_core_filter. It iterates over the brigade and hands over each bucket alone to
> > >> ap_core_output_filter. IMHO a bug.
> > >
> > > How about the attached patch for fixing?
> >
> > Scratch that. I guess it is much more complex.
>
> Hmm, well it works for me and looks correct, it batches up those the
> writev(,,1) calls as expected, but it seems YMMV :)

After a closer look into this, Rüdiger's patch seems indeed incomplete.
What we possibly want is keep buckets in tmp_bb until it's time to
pass it down the chain, per ap_filter_reinstate_brigade() logic (i.e.
FLUSH bucket, size limit, ...).

I tried to implement that in the attached patch, WDYT?

>
> I notice the function is doing unconditional blocking reads without
> flushing first - rule 10 of the golden rules of output filters! --

The patch also addresses this.


Regards,
Yann.
Index: server/request.c
===================================================================
--- server/request.c	(revision 1843207)
+++ server/request.c	(working copy)
@@ -2066,8 +2066,10 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_sub_req_ou
 AP_CORE_DECLARE_NONSTD(apr_status_t) ap_request_core_filter(ap_filter_t *f,
                                                             apr_bucket_brigade *bb)
 {
+    apr_status_t status = APR_SUCCESS;
+    apr_read_type_e block = APR_NONBLOCK_READ;
+    conn_rec *c = f->r->connection;
     apr_bucket *flush_upto = NULL;
-    apr_status_t status = APR_SUCCESS;
     apr_bucket_brigade *tmp_bb;
     int seen_eor = 0;
 
@@ -2092,6 +2094,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_request_co
     /* Don't touch *bb after seen_eor */
     while (status == APR_SUCCESS && !seen_eor && !APR_BRIGADE_EMPTY(bb)) {
         apr_bucket *bucket = APR_BRIGADE_FIRST(bb);
+        apr_bucket *pass_upto = NULL;
 
         if (AP_BUCKET_IS_EOR(bucket)) {
             /* pass out everything and never come back again,
@@ -2098,6 +2101,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_request_co
              * r is destroyed with this bucket!
              */
             APR_BRIGADE_CONCAT(tmp_bb, bb);
+            pass_upto = APR_BRIGADE_LAST(tmp_bb);
             ap_remove_output_filter(f);
             seen_eor = 1;
         }
@@ -2116,31 +2120,61 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_request_co
              * something safe to pass down to the connection filters without
              * needing to be set aside.
              */
-            if (!APR_BUCKET_IS_METADATA(bucket)
-                    && bucket->length == (apr_size_t)-1) {
+            if (bucket->length == (apr_size_t)-1) {
                 const char *data;
                 apr_size_t size;
-                status = apr_bucket_read(bucket, &data, &size, APR_BLOCK_READ);
+
+                status = apr_bucket_read(bucket, &data, &size, block);
                 if (status != APR_SUCCESS) {
-                    break;
+                    if (!APR_STATUS_IS_EAGAIN(status)
+                            || block != APR_NONBLOCK_READ) {
+                        break;
+                    }
+                    /* Flush everything so far and retry in blocking mode */
+                    pass_upto = apr_bucket_flush_create(c->bucket_alloc);
+                    APR_BRIGADE_INSERT_TAIL(tmp_bb, pass_upto);
+                    block = APR_BLOCK_READ;
                 }
+                else {
+                    block = APR_NONBLOCK_READ;
+                }
             }
 
-            /* pass each bucket down the chain */
-            APR_BUCKET_REMOVE(bucket);
-            APR_BRIGADE_INSERT_TAIL(tmp_bb, bucket);
+            /* Move the bucket (if any) to tmp_bb and check whether it exhausts
+             * bb or brings tmp_bb above the limit; in both cases it's time to
+             * pass things down the chain.
+             */
+            if (!pass_upto) {
+                APR_BUCKET_REMOVE(bucket);
+                APR_BRIGADE_INSERT_TAIL(tmp_bb, bucket);
+                if (!APR_BRIGADE_EMPTY(bb)) {
+                    ap_filter_reinstate_brigade(f, tmp_bb, &pass_upto);
+                }
+                else {
+                    pass_upto = bucket;
+                }
+            }
         }
 
-        status = ap_pass_brigade(f->next, tmp_bb);
-        apr_brigade_cleanup(tmp_bb);
+        /* Pass tmp_bb when asked above only */
+        if (pass_upto) {
+            status = ap_pass_brigade(f->next, tmp_bb);
+            apr_brigade_cleanup(tmp_bb);
+        }
     }
 
+    /* Don't touch *bb after seen_eor */
+    if (!seen_eor) {
+        apr_status_t rv;
+        APR_BRIGADE_PREPEND(bb, tmp_bb);
+        rv = ap_filter_setaside_brigade(f, bb);
+        if (status == APR_SUCCESS) {
+            status = rv;
+        }
+    }
+
     ap_release_brigade(f->c, tmp_bb);
 
-    /* Don't touch *bb after seen_eor */
-    if (status == APR_SUCCESS && !seen_eor) {
-        status = ap_filter_setaside_brigade(f, bb);
-    }
     return status;
 }