[From users@]

On Mon, Oct 19, 2015 at 11:44 PM, Andy Wang <aw...@ptc.com> wrote:
>
> The issue is currently reproduced using Apache httpd 2.4.16, mod_jk 1.2.41
> and tomcat 8.0.28.
>
> I've created a very very simple JSP page that does nothing but print a small
> string, but I've tried changing the jsp page to print a very very large
> string (10000+ characters) and no difference.
>
> If I POST to this JSP page, and something like mod_deflate is in place to
> force a chunked transfer the TCP packet capture looks like this:
>
> No.     Time           Source  Destination   Protocol Length Info
>    1850 4827.762721000 client  server        TCP      66     54131→2280
> [SYN] Seq=0 Win=8192 Len=0 MSS=1460 WS=256 SACK_PERM=1
>    1851 4827.764976000 server  client        TCP      66     2280→54131
> [SYN, ACK] Seq=0 Ack=1 Win=8192 Len=0 MSS=1460 WS=256 SACK_PERM=1
>    1852 4827.765053000 client  server        TCP      54     54131→2280
> [ACK] Seq=1 Ack=1 Win=131328 Len=0
>    1853 4827.765315000 client  server        HTTP     791    POST
> /JSPtoPostTo HTTP/1.1
>    1854 4827.777981000 server  client        TCP      466    [TCP segment of
> a reassembled PDU]
>    1855 4827.982961000 client  server        TCP      54     54131→2280
> [ACK] Seq=738 Ack=413 Win=130816 Len=0
>    1856 4832.770458000 server  client        HTTP     74     HTTP/1.1 200 OK
> (text/html)
>    1857 4832.770459000 server  client        TCP      60     2280→54131
> [FIN, ACK] Seq=433 Ack=738 Win=65536 Len=0
>    1858 4832.770555000 client  server        TCP      54     54131→2280
> [ACK] Seq=738 Ack=434 Win=130816 Len=0
>    1859 4832.770904000 client  server        TCP      54     54131→2280
> [FIN, ACK] Seq=738 Ack=434 Win=130816 Len=0
>    1860 4832.774200000 server  client        TCP      60     2280→54131
> [ACK] Seq=434 Ack=739 Win=65536 Len=0
>
> Spdficially, note the 5 second delay between the first segment (No. 1854)
> and the second data segment (1856).

This is the deferred write triggering *after* the keepalive timeout,
whereas no subsequent request was pipelined.
I wonder if we shouldn't issue a flush at the end of each request when
the following is not already there, ie:

Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c    (revision 1708095)
+++ modules/http/http_request.c    (working copy)
@@ -228,8 +228,9 @@ AP_DECLARE(void) ap_die(int type, request_rec *r)
     ap_die_r(type, r, r->status);
 }

-static void check_pipeline(conn_rec *c, apr_bucket_brigade *bb)
+static int check_pipeline(conn_rec *c, apr_bucket_brigade *bb)
 {
+    c->data_in_input_filters = 0;
     if (c->keepalive != AP_CONN_CLOSE && !c->aborted) {
         apr_status_t rv;

@@ -236,17 +237,12 @@ AP_DECLARE(void) ap_die(int type, request_rec *r)
         AP_DEBUG_ASSERT(APR_BRIGADE_EMPTY(bb));
         rv = ap_get_brigade(c->input_filters, bb, AP_MODE_SPECULATIVE,
                             APR_NONBLOCK_READ, 1);
-        if (rv != APR_SUCCESS || APR_BRIGADE_EMPTY(bb)) {
-            /*
-             * Error or empty brigade: There is no data present in the input
-             * filter
-             */
-            c->data_in_input_filters = 0;
-        }
-        else {
+        if (rv == APR_SUCCESS && !APR_BRIGADE_EMPTY(bb)) {
             c->data_in_input_filters = 1;
+            return 1;
         }
     }
+    return 0;
 }


@@ -287,11 +283,30 @@ AP_DECLARE(void) ap_process_request_after_handler(
      * already by the EOR bucket's cleanup function.
      */

-    check_pipeline(c, bb);
+    if (!check_pipeline(c, bb)) {
+        apr_status_t rv;
+
+        b = apr_bucket_flush_create(c->bucket_alloc);
+        APR_BRIGADE_INSERT_HEAD(bb, b);
+        rv = ap_pass_brigade(c->output_filters, bb);
+        if (APR_STATUS_IS_TIMEUP(rv)) {
+            /*
+             * Notice a timeout as an error message. This might be
+             * valuable for detecting clients with broken network
+             * connections or possible DoS attacks.
+             *
+             * It is still safe to use r / r->pool here as the eor bucket
+             * could not have been destroyed in the event of a timeout.
+             */
+            ap_log_cerror(APLOG_MARK, APLOG_INFO, rv, c, APLOGNO(01581)
+                          "Timeout while flushing data to the client");
+        }
+    }
     apr_brigade_destroy(bb);
-    if (c->cs)
+    if (c->cs) {
         c->cs->state = (c->aborted) ? CONN_STATE_LINGER
                                     : CONN_STATE_WRITE_COMPLETION;
+    }
     AP_PROCESS_REQUEST_RETURN((uintptr_t)r, r->uri, r->status);
     if (ap_extended_status) {
         ap_time_process_request(c->sbh, STOP_PREQUEST);
@@ -373,33 +388,10 @@ void ap_process_async_request(request_rec *r)

 AP_DECLARE(void) ap_process_request(request_rec *r)
 {
-    apr_bucket_brigade *bb;
-    apr_bucket *b;
-    conn_rec *c = r->connection;
-    apr_status_t rv;
-
     ap_process_async_request(r);

-    if (!c->data_in_input_filters) {
-        bb = apr_brigade_create(c->pool, c->bucket_alloc);
-        b = apr_bucket_flush_create(c->bucket_alloc);
-        APR_BRIGADE_INSERT_HEAD(bb, b);
-        rv = ap_pass_brigade(c->output_filters, bb);
-        if (APR_STATUS_IS_TIMEUP(rv)) {
-            /*
-             * Notice a timeout as an error message. This might be
-             * valuable for detecting clients with broken network
-             * connections or possible DoS attacks.
-             *
-             * It is still safe to use r / r->pool here as the eor bucket
-             * could not have been destroyed in the event of a timeout.
-             */
-            ap_log_rerror(APLOG_MARK, APLOG_INFO, rv, r, APLOGNO(01581)
-                          "Timeout while writing data for URI %s to the"
-                          " client", r->unparsed_uri);
-        }
-    }
     if (ap_extended_status) {
+        conn_rec *c = r->connection;
         ap_time_process_request(c->sbh, STOP_PREQUEST);
     }
 }
--

Thoughts?

Reply via email to