fix for stream output stalling, v1.10.8

ChangeLog

@@ -1,3 +1,7 @@
+v1.10.8
+--------------------------------------------------------------------------------
+  * a stream could stall and not send any more output to the client which in the end aborts the connection due to a timeout.
+
 v1.10.7
 --------------------------------------------------------------------------------
   * disable and give warning when mpm_prefork is encountered. The server will

configure.ac

@@ -14,7 +14,7 @@
 #
 
 AC_PREREQ([2.69])
-AC_INIT([mod_http2], [1.10.7], [[email protected]])
+AC_INIT([mod_http2], [1.10.8], [[email protected]])
 
 LT_PREREQ([2.2.6])
 LT_INIT()

mod_http2/h2_stream.c

@@ -774,20 +774,20 @@ static apr_bucket *get_first_headers_bucket(apr_bucket_brigade *bb)
     return NULL;
 }
 
-static apr_status_t add_data(h2_stream *stream, apr_off_t requested,
-                             apr_off_t *plen, int *peos, int *complete, 
-                             h2_headers **pheaders)
+static apr_status_t add_buffered_data(h2_stream *stream, apr_off_t requested,
+                                      apr_off_t *plen, int *peos, int *is_all, 
+                                      h2_headers **pheaders)
 {
     apr_bucket *b, *e;
 
     *peos = 0;
     *plen = 0;
-    *complete = 0;
+    *is_all = 0;
     if (pheaders) {
         *pheaders = NULL;
     }
 
-    H2_STREAM_OUT_LOG(APLOG_TRACE2, stream, "add_data");
+    H2_STREAM_OUT_LOG(APLOG_TRACE2, stream, "add_buffered_data");
     b = APR_BRIGADE_FIRST(stream->out_buffer);
     while (b != APR_BRIGADE_SENTINEL(stream->out_buffer)) {
         e = APR_BUCKET_NEXT(b);
@@ -833,7 +833,7 @@ static apr_status_t add_data(h2_stream *stream, apr_off_t requested,
         }
         b = e;
     }
-    *complete = 1;
+    *is_all = 1;
     return APR_SUCCESS;
 }
 
@@ -865,7 +865,7 @@ apr_status_t h2_stream_out_prepare(h2_stream *stream, apr_off_t *plen,
     requested = (*plen > 0)? H2MIN(*plen, max_chunk) : max_chunk;
 
     /* count the buffered data until eos or a headers bucket */
-    status = add_data(stream, requested, plen, peos, &complete, pheaders);
+    status = add_buffered_data(stream, requested, plen, peos, &complete, pheaders);
 
     if (status == APR_EAGAIN) {
         /* TODO: ugly, someone needs to retrieve the response first */
@@ -882,28 +882,38 @@ apr_status_t h2_stream_out_prepare(h2_stream *stream, apr_off_t *plen,
         return APR_SUCCESS;
     }
 
+    /* If there we do not have enough buffered data to satisfy the requested
+     * length *and* we counted the _complete_ buffer (and did not stop in the middle
+     * because of meta data there), lets see if we can read more from the
+     * output beam */
     missing = H2MIN(requested, stream->max_mem) - *plen;
     if (complete && !*peos && missing > 0) {
+        apr_status_t rv = APR_EOF;
+
         if (stream->output) {
             H2_STREAM_OUT_LOG(APLOG_TRACE2, stream, "pre");
-            status = h2_beam_receive(stream->output, stream->out_buffer, 
-                                     APR_NONBLOCK_READ, 
-                                     stream->max_mem - *plen);
+            rv = h2_beam_receive(stream->output, stream->out_buffer, 
+                                 APR_NONBLOCK_READ, stream->max_mem - *plen);
             H2_STREAM_OUT_LOG(APLOG_TRACE2, stream, "post");
         }
-        else {
-            status = APR_EOF;
-        }
 
-        if (APR_STATUS_IS_EOF(status)) {
+        if (rv == APR_SUCCESS) {
+            /* count the buffer again, now that we have read output */
+            status = add_buffered_data(stream, requested, plen, peos, &complete, pheaders);
+        }
+        else if (APR_STATUS_IS_EOF(rv)) {
             apr_bucket *eos = apr_bucket_eos_create(c->bucket_alloc);
             APR_BRIGADE_INSERT_TAIL(stream->out_buffer, eos);
             *peos = 1;
-            status = APR_SUCCESS;
         }
-        else if (status == APR_SUCCESS) {
-            /* do it again, now that we have gotten more */
-            status = add_data(stream, requested, plen, peos, &complete, pheaders);
+        else if (APR_STATUS_IS_EAGAIN(rv)) {
+            /* we set this is the status of this call only if there
+             * is no buffered data, see check below */
+        }
+        else {
+            /* real error reading. Give this back directly, even though
+             * we may have something buffered. */
+            status = rv;
         }
     }
 

mod_http2/h2_task.c

@@ -129,7 +129,7 @@ static apr_status_t slave_out(h2_task *task, ap_filter_t* f,
                               apr_bucket_brigade* bb)
 {
     apr_bucket *b;
-    apr_status_t status = APR_SUCCESS;
+    apr_status_t rv = APR_SUCCESS;
     int flush = 0, blocking;
 
     if (task->frozen) {
@@ -148,50 +148,65 @@ static apr_status_t slave_out(h2_task *task, ap_filter_t* f,
         return APR_SUCCESS;
     }
 
+send:
     /* we send block once we opened the output, so someone is there
      * reading it *and* the task is not assigned to a h2_req_engine */
     blocking = (!task->assigned && task->output.opened);
-    if (!task->output.opened) {
-        for (b = APR_BRIGADE_FIRST(bb);
-             b != APR_BRIGADE_SENTINEL(bb);
-             b = APR_BUCKET_NEXT(b)) {
-            if (APR_BUCKET_IS_FLUSH(b)) {
-                flush = 1;
-                break;
-            }
+    for (b = APR_BRIGADE_FIRST(bb);
+         b != APR_BRIGADE_SENTINEL(bb);
+         b = APR_BUCKET_NEXT(b)) {
+        if (APR_BUCKET_IS_FLUSH(b) || APR_BUCKET_IS_EOS(b) || AP_BUCKET_IS_EOR(b)) {
+            flush = 1;
+            break;
         }
     }
 
     if (task->output.bb && !APR_BRIGADE_EMPTY(task->output.bb)) {
         /* still have data buffered from previous attempt.
          * setaside and append new data and try to pass the complete data */
         if (!APR_BRIGADE_EMPTY(bb)) {
-            status = ap_save_brigade(f, &task->output.bb, &bb, task->pool);
+            if (APR_SUCCESS != (rv = ap_save_brigade(f, &task->output.bb, &bb, task->pool))) {
+                goto out;
+            }
         }
-        if (status == APR_SUCCESS) {
-            status = send_out(task, task->output.bb, blocking);
-        } 
+        rv = send_out(task, task->output.bb, blocking);
     }
     else {
-        /* no data buffered here, try to pass the brigade directly */
-        status = send_out(task, bb, blocking); 
-        if (status == APR_SUCCESS && !APR_BRIGADE_EMPTY(bb)) {
-            /* could not write all, buffer the rest */
-            ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, task->c, APLOGNO(03405)
-                          "h2_slave_out(%s): saving brigade", 
-                          task->id);
-            status = ap_save_brigade(f, &task->output.bb, &bb, task->pool);
-            flush = 1;
+        /* no data buffered previously, pass brigade directly */
+        rv = send_out(task, bb, blocking);
+
+        if (APR_SUCCESS == rv && !APR_BRIGADE_EMPTY(bb)) {
+            /* output refused to buffer it all, time to open? */
+            if (!task->output.opened && APR_SUCCESS == (rv = open_output(task))) {
+                /* Make another attempt to send the data. With the output open,
+                 * the call might be blocking and send all data, so we do not need
+                 * to save the brigade */
+                goto send;
+            }
+            else if (blocking && flush) {
+                /* Need to keep on doing this. */
+                goto send;
+            }
+
+            if (APR_SUCCESS == rv) {
+                /* could not write all, buffer the rest */
+                ap_log_cerror(APLOG_MARK, APLOG_DEBUG, rv, task->c, APLOGNO(03405)
+                              "h2_slave_out(%s): saving brigade", task->id);
+                ap_assert(NULL);
+                rv = ap_save_brigade(f, &task->output.bb, &bb, task->pool);
+                flush = 1;
+            }
         }
     }
 
-    if (status == APR_SUCCESS && !task->output.opened && flush) {
+    if (APR_SUCCESS == rv && !task->output.opened && flush) {
         /* got a flush or could not write all, time to tell someone to read */
-        status = open_output(task);
+        rv = open_output(task);
     }
-    ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, task->c, 
+out:
+    ap_log_cerror(APLOG_MARK, APLOG_TRACE2, rv, task->c, 
                   "h2_slave_out(%s): slave_out leave", task->id);    
-    return status;
+    return rv;
 }
 
 static apr_status_t output_finish(h2_task *task)

mod_http2/h2_version.h

@@ -26,15 +26,15 @@
  * @macro
  * Version number of the http2 module as c string
  */
-#define MOD_HTTP2_VERSION "1.10.7-git"
+#define MOD_HTTP2_VERSION "1.10.8-git"
 
 /**
  * @macro
  * Numerical representation of the version number of the http2 module
  * release. This is a 24 bit number with 8 bits for major number, 8 bits
  * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203.
  */
-#define MOD_HTTP2_VERSION_NUM 0x010a07
+#define MOD_HTTP2_VERSION_NUM 0x010a08
 
 
 #endif /* mod_h2_h2_version_h */