[PATCH] pipelining bug in Event MPM

[Greg Ames]

I was able to easily create a browser hang by configuring Mozilla to enable HTTP 
  pipelining, then pointing my DocumentRoot at an old copy of the 
xml.apache.org web site which had tons of imbedded graphics.  Thanks, Justin, 
for pointing out the bug.

The attached patch fixes it when there are no connection filters such as mod_ssl 
involved.  When I switch to the version of the patch that does speculative 
reads, it behaves oddly.  With pipelining, the last couple of .gifs aren't 
displayed until something times out.  Without pipelining nearly all the .gifs 
are delayed.  In gdb, it looks like it's not properly detecting when the input 
filters become empty.  I'm going to investigate further but need to take care of 
some day job stuff first.

Paul, my apologies, this is against my version of the patch.  I plan to switch 
over to yours after figuring out what's happening with the speculative reads.

Greg
--- include/httpd.h.old 2004-10-28 13:06:07.000000000 -0400
+++ include/httpd.h     2004-10-27 21:56:44.000000000 -0400
@@ -1016,6 +1016,7 @@
     /** The bucket allocator to use for all bucket/brigade creations */
     struct apr_bucket_alloc_t *bucket_alloc;
     http_state_t *hs;
+    int data_in_input_filters;
 };
 
 typedef enum  {
Index: modules/http/http_request.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_request.c,v
retrieving revision 1.165
diff -u -d -b -r1.165 http_request.c
--- modules/http/http_request.c 2 Aug 2004 17:12:32 -0000       1.165
+++ modules/http/http_request.c 28 Oct 2004 17:56:32 -0000
@@ -205,9 +205,19 @@
      */
     /* ### shouldn't this read from the connection input filters? */
     /* ### is zero correct? that means "read one line" */
-    if (r->connection->keepalive == AP_CONN_CLOSE || 
-        ap_get_brigade(r->input_filters, bb, AP_MODE_EATCRLF, 
+    if (r->connection->keepalive != AP_CONN_CLOSE) {
+        /* if (ap_get_brigade(r->input_filters, bb, AP_MODE_SPECULATIVE, 
+         *                       APR_NONBLOCK_READ, 1) != APR_SUCCESS) {
+         */                       
+        if (ap_get_brigade(r->input_filters, bb, AP_MODE_EATCRLF, 
                        APR_NONBLOCK_READ, 0) != APR_SUCCESS) {
+            c->data_in_input_filters = 0;  /* we got APR_EOF or an error */
+        }
+        else {
+            c->data_in_input_filters = 1;
+            return;    /* don't flush */
+        }
+    }    
         apr_bucket *e = apr_bucket_flush_create(c->bucket_alloc);
 
         /* We just send directly to the connection based filters.  At
@@ -218,7 +228,6 @@
          */
         APR_BRIGADE_INSERT_HEAD(bb, e);
         ap_pass_brigade(r->connection->output_filters, bb);
-    }
 }
 
 void ap_process_request(request_rec *r)
--- modules/http/http_core.c.old        2004-10-28 13:23:21.000000000 -0400
+++ modules/http/http_core.c    2004-10-27 23:22:48.000000000 -0400
@@ -279,9 +279,9 @@
     request_rec *r;
     http_state_t *hs = c->hs;
     
-        switch (hs->state) {
-        case HTTP_STATE_READ_REQUEST_LINE:
-        {
+    AP_DEBUG_ASSERT(hs->state == HTTP_STATE_READ_REQUEST_LINE);
+    
+    while (hs->state == HTTP_STATE_READ_REQUEST_LINE) {
             ap_update_child_status(c->sbh, SERVER_BUSY_READ, NULL);
             
             if ((r = ap_read_request(c))) {
@@ -299,7 +299,7 @@
                 if (c->keepalive != AP_CONN_KEEPALIVE || c->aborted) {
                     hs->state = HTTP_STATE_LINGER;
                 }
-                else {
+            else if (!c->data_in_input_filters) {
                     hs->state = HTTP_STATE_CHECK_REQUEST_LINE_READABLE;
                 }
                 apr_pool_destroy(r->pool);
@@ -307,11 +307,6 @@
             else {   /* ap_read_request failed - client may have closed */
                 hs->state = HTTP_STATE_LINGER;
             }                                                                         
                                                
-            break;
-        }
-        default:
-            AP_DEBUG_ASSERT(0);
-            hs->state = HTTP_STATE_LINGER;
     }
     return OK;
 }