no async http2 read for now, fixed GOAWAY flush at end of session, scoreboard keepali...

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1723125 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 40f88076fe8355ee6545c1c1a3aff2eb9150488f..d22273f955c95839e1adacc2817eac7adcf02316 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,19 +1,15 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.5.0
 
+  *) mod_http2: async reading for http2 connections disabled for now. Fixed
+     flushing of last GOAWAY frame. Fixed calculation of last stream id
+     accepted as described in rfc7540. Reading in KEEPALIVE state now 
+     correctly show in scoreboard. Fixed possible race in connection
+     shutdown after review by Ylavic. [Stefan Eissing]
+
   *) mod_ssl: Avoid one TLS record (application data) fragmentation by
      including the last suitable bucket when coalescing.  [Yann Ylavic]
 
-  *) mod_http2: reworked synching of session shutdown with worker threads. h2 
-     workers now stick to a session until no more reuqquest are tbd, keepalive 
-     handling revisited, users report problems with connection close without
-     GOAWAY frames. [Stefan Eissing]
-  
-  *) mod_http2: Fixed several errors when connections are closed in the middle
-     of requests, changed H2KeepAliveTimeout defaults to be the same as H2Timeout
-     for synchronous MPMs and leave keepalive timeout to async MPMs default.
-     [Stefan Eissing]
-
   *) mod_cache_socache: Fix a possible cached entity body corruption when it
      is received from an origin server in multiple batches and forwarded by
      mod_proxy.  [Yann Ylavic]

diff --git a/modules/http2/h2_conn.c b/modules/http2/h2_conn.c
index abc8d8989c16b24a510c8745b1956db56194c079..49570c9798fb2a0342b6d18994e4a4a4b085f21b 100644 (file)
--- a/modules/http2/h2_conn.c
+++ b/modules/http2/h2_conn.c
@@ -167,9 +167,6 @@ apr_status_t h2_conn_setup(h2_ctx *ctx, conn_rec *c, request_rec *r)
     }
 
     h2_ctx_session_set(ctx, session);
-    
-    ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_READ, c);
-
     return APR_SUCCESS;
 }
 

diff --git a/modules/http2/h2_filter.c b/modules/http2/h2_filter.c
index 5ad3aed5d70a55d0bc42577ddcb86604f172a82e..fd8e25ce468f6789c5aac78467ef0874a3356579 100644 (file)
--- a/modules/http2/h2_filter.c
+++ b/modules/http2/h2_filter.c
@@ -133,7 +133,6 @@ apr_status_t h2_filter_core_input(ap_filter_t* f,
                 apr_socket_timeout_set(cin->socket, t);
             }
         }
-        ap_update_child_status_from_conn(f->c->sbh, SERVER_BUSY_READ, f->c);
         status = ap_get_brigade(f->next, cin->bb, AP_MODE_READBYTES,
                                 block, readbytes);
         if (saved_timeout != UNSET) {

diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c
index 4b483addea8a6430d32cda13c48c2575a78837d9..0cc0fcb6212d5566ee35f6ff42cfc6abb200fce2 100644 (file)
--- a/modules/http2/h2_session.c
+++ b/modules/http2/h2_session.c
@@ -695,6 +695,46 @@ static void h2_session_destroy(h2_session *session)
     }
 }
 
+static apr_status_t h2_session_shutdown(h2_session *session, int reason)
+{
+    apr_status_t status = APR_SUCCESS;
+    
+    AP_DEBUG_ASSERT(session);
+    session->aborted = 1;
+    if (session->state != H2_SESSION_ST_CLOSING
+        && session->state != H2_SESSION_ST_ABORTED) {
+        h2_mplx_abort(session->mplx);
+        if (session->server_goaway) {
+            /* already sent one */
+        }
+        else if (!reason) {
+            nghttp2_submit_goaway(session->ngh2, NGHTTP2_FLAG_NONE, 
+                                  h2_mplx_get_max_stream_started(session->mplx), 
+                                  reason, NULL, 0);
+            status = nghttp2_session_send(session->ngh2);
+            session->server_goaway = 1;
+            ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c,
+                          "session(%ld): shutdown, no err", session->id);
+        }
+        else {
+            const char *err = nghttp2_strerror(reason);
+            nghttp2_submit_goaway(session->ngh2, NGHTTP2_FLAG_NONE, 
+                                  h2_mplx_get_max_stream_started(session->mplx), 
+                                  reason, (const uint8_t *)err, 
+                                  strlen(err));
+            status = nghttp2_session_send(session->ngh2);
+            session->server_goaway = 1;
+            ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c,
+                          "session(%ld): shutdown, err=%d '%s'",
+                          session->id, reason, err);
+        }
+        
+        h2_conn_io_flush(&session->io);
+        session->state = H2_SESSION_ST_CLOSING;
+    }
+    return status;
+}
+
 static apr_status_t session_pool_cleanup(void *data)
 {
     h2_session *session = data;
@@ -707,6 +747,22 @@ static apr_status_t session_pool_cleanup(void *data)
      */
     ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, session->c,
                   "session(%ld): pool_cleanup", session->id);
+    
+    AP_DEBUG_ASSERT(session->aborted || session->server_goaway);
+    if (!session->aborted && !session->server_goaway) {
+        /* Not good. The connection is being torn down and we have
+         * not sent a goaway. This is considered a protocol error and
+         * the client has to assume that any streams "in flight" may have
+         * been processed and are not safe to retry.
+         * As clients with idle connection may only learn about a closed
+         * connection when sending the next request, this has the effect
+         * that at least this one request will fail.
+         */
+        ap_log_cerror(APLOG_MARK, APLOG_WARNING, 0, session->c,
+                      "session(%ld): connection disappeared without proper "
+                      "goodbye, clients will be confused, should not happen", 
+                      session->id);
+    }
     /* keep us from destroying the pool, since that is already ongoing. */
     session->pool = NULL;
     h2_session_destroy(session);
@@ -889,44 +945,6 @@ void h2_session_eoc_callback(h2_session *session)
     h2_session_destroy(session);
 }
 
-static apr_status_t h2_session_shutdown(h2_session *session, int reason)
-{
-    apr_status_t status = APR_SUCCESS;
-    
-    AP_DEBUG_ASSERT(session);
-    session->aborted = 1;
-    if (session->state != H2_SESSION_ST_CLOSING
-        && session->state != H2_SESSION_ST_ABORTED) {
-        h2_mplx_abort(session->mplx);
-        if (session->server_goaway) {
-            /* already sent one */
-        }
-        else if (!reason) {
-            nghttp2_submit_goaway(session->ngh2, NGHTTP2_FLAG_NONE, 
-                                  h2_mplx_get_max_stream_started(session->mplx), 
-                                  reason, NULL, 0);
-            status = nghttp2_session_send(session->ngh2);
-            session->server_goaway = 1;
-            ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c,
-                          "session(%ld): shutdown, no err", session->id);
-        }
-        else {
-            const char *err = nghttp2_strerror(reason);
-            nghttp2_submit_goaway(session->ngh2, NGHTTP2_FLAG_NONE, 
-                                  h2_mplx_get_max_stream_started(session->mplx), 
-                                  reason, (const uint8_t *)err, 
-                                  strlen(err));
-            status = nghttp2_session_send(session->ngh2);
-            session->server_goaway = 1;
-            ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c,
-                          "session(%ld): shutdown, err=%d '%s'",
-                          session->id, reason, err);
-        }
-        session->state = H2_SESSION_ST_CLOSING;
-    }
-    return status;
-}
-
 void h2_session_abort(h2_session *session, apr_status_t status)
 {
     AP_DEBUG_ASSERT(session);
@@ -1716,7 +1734,7 @@ apr_status_t h2_session_process(h2_session *session, int async)
 {
     apr_status_t status = APR_SUCCESS;
     conn_rec *c = session->c;
-    int rv, have_written, have_read, remain_secs;
+    int rv, have_written, have_read;
     const char *reason = "";
 
     ap_log_cerror( APLOG_MARK, APLOG_TRACE1, status, c,
@@ -1740,6 +1758,7 @@ apr_status_t h2_session_process(h2_session *session, int async)
                     session->server_goaway = 1;
                 } 
                 
+                ap_update_child_status(c->sbh, SERVER_BUSY_READ, NULL);
                 status = h2_session_start(session, &rv);
                 ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, c,
                               "h2_session(%ld): started on %s:%d", session->id,
@@ -1755,13 +1774,14 @@ apr_status_t h2_session_process(h2_session *session, int async)
                 break;
                 
             case H2_SESSION_ST_IDLE_READ:
-                h2_filter_cin_timeout_set(session->cin, session->timeout_secs);
-                ap_update_child_status(c->sbh, SERVER_BUSY_READ, NULL);
+                h2_filter_cin_timeout_set(session->cin, session->keepalive_secs);
+                ap_update_child_status(c->sbh, SERVER_BUSY_KEEPALIVE, NULL);
                 status = h2_session_read(session, 1, 10);
                 if (APR_STATUS_IS_TIMEUP(status)) {
                     ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, c,
                                   "h2_session(%ld): IDLE -> KEEPALIVE", session->id);
-                    session->state = H2_SESSION_ST_KEEPALIVE;
+                    h2_session_shutdown(session, 0);
+                    goto out;
                 }
                 else if (status == APR_SUCCESS) {
                     /* got something, go busy again */
@@ -1777,6 +1797,7 @@ apr_status_t h2_session_process(h2_session *session, int async)
                 
             case H2_SESSION_ST_BUSY:
                 if (nghttp2_session_want_read(session->ngh2)) {
+                    ap_update_child_status(c->sbh, SERVER_BUSY_READ, NULL);
                     h2_filter_cin_timeout_set(session->cin, session->timeout_secs);
                     status = h2_session_read(session, 0, 10);
                     if (status == APR_SUCCESS) {
@@ -1879,6 +1900,7 @@ apr_status_t h2_session_process(h2_session *session, int async)
                 }
                 else if (status == APR_TIMEUP) {
                     if (nghttp2_session_want_read(session->ngh2)) {
+                        ap_update_child_status(c->sbh, SERVER_BUSY_READ, NULL);
                         status = h2_session_read(session, 0, 1);
                         if (status == APR_SUCCESS) {
                             /* got something, go busy again */
@@ -1904,57 +1926,9 @@ apr_status_t h2_session_process(h2_session *session, int async)
                 }
                 break;
                 
-            case H2_SESSION_ST_KEEPALIVE:
-                /* Our normal H2Timeout has passed and we are considering to
-                 * extend that with the H2KeepAliveTimeout. */
-                remain_secs = session->keepalive_secs - session->timeout_secs;
-                if (remain_secs <= 0) {
-                    /* keepalive is <= normal timeout, close the session */
-                    reason = "keepalive expired";
-                    h2_session_shutdown(session, 0);
-                    goto out;
-                }
-                session->c->keepalive = AP_CONN_KEEPALIVE;
-                ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_KEEPALIVE, c);
-                
-                if ((apr_time_sec(session->s->keep_alive_timeout) >= remain_secs)
-                    && async && session->c->cs
-                    && !session->r) {
-                    /* Async MPMs are able to handle keep-alive connections without
-                     * blocking a thread. For this to happen, we need to return from
-                     * processing, indicating the IO event we are waiting for, and
-                     * may be called again if the event happens.
-                     * TODO: this does not properly GOAWAY connections...
-                     * TODO: This currently does not work on upgraded requests...
-                     */
-                    ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, c,
-                                  "h2_session(%ld): async KEEPALIVE -> IDLE_READ", session->id);
-                    session->state = H2_SESSION_ST_IDLE_READ;
-                    session->c->cs->state = CONN_STATE_WRITE_COMPLETION;
-                    reason = "async keepalive";
-                    status = APR_SUCCESS;
-                    goto out;
-                }
-                ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, c,
-                              "h2_session(%ld): KEEPALIVE read", session->id);
-                h2_filter_cin_timeout_set(session->cin, remain_secs);
-                status = h2_session_read(session, 1, 1);
-                if (APR_STATUS_IS_TIMEUP(status)) {
-                    reason = "keepalive expired";
-                    h2_session_shutdown(session, 0);
-                    goto out;
-                }
-                else if (status != APR_SUCCESS) {
-                    reason = "keepalive error";
-                    goto out;
-                }
-                ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, c,
-                              "h2_session(%ld): KEEPALIVE -> BUSY", session->id);
-                session->state = H2_SESSION_ST_BUSY;
-                break;
-                
             case H2_SESSION_ST_CLOSING:
                 if (nghttp2_session_want_write(session->ngh2)) {
+                    ap_update_child_status(c->sbh, SERVER_BUSY_READ, NULL);
                     status = h2_session_send(session);
                     if (status != APR_SUCCESS) {
                         reason = "send error";

diff --git a/modules/http2/h2_session.h b/modules/http2/h2_session.h
index 2dba10f7200b0d3689be137d8e57daf1fa10a50c..b3b26b7b005294d701f2169a381ae90cefb025ae 100644 (file)
--- a/modules/http2/h2_session.h
+++ b/modules/http2/h2_session.h
@@ -58,7 +58,6 @@ typedef enum {
     H2_SESSION_ST_IDLE_READ,        /* nothing to write, expecting data inc */
     H2_SESSION_ST_BUSY,             /* read/write without stop */
     H2_SESSION_ST_BUSY_WAIT,        /* waiting for tasks reporting back */
-    H2_SESSION_ST_KEEPALIVE,        /* nothing to write, normal timeout passed */
     H2_SESSION_ST_CLOSING,          /* shuting down */
     H2_SESSION_ST_ABORTED,          /* client closed connection or sky fall */
 } h2_session_state;