Merge r1818804, r1818951, r1818958, r1818960, r1819027, r1819214, r1820035 from trunk:

mpm_event: close connections not reported as handled by any module.

This avoids losing track of them and leaking scoreboard entries.
PR 61551.

mpm_event: follow up to r1818804.

Address corner case where connection is aborted due to ap_run_pre_connection()
failure, and update comment about ap_run_process_connection() expected return
status and state.

mpm_event: follow up to r1818804 and r1818951.

Align comment and fix typos.

mpm_event: follow up to r1818804.

Allow DONE as a successful ap_run_process_connection() return value, for
instance h2_conn_run() and h2_task_process_conn() uses it, third-party
modules may too...

mpm_event: follow up to r1818804 and r1818951.

Be more correct in comment about CONN_STATE_WRITE_COMPLETION.
We currently have/need no state to simply wait for readability on a socket,
so the previous comment was misleading. Write completion can't be used for
a simple "wait for read event and come back to process_connection hooks".

mpm_event: follow up to r1818804 and r1818960.

Align mod_http2 with expected returned state from process_connection hooks in
async MPMs.
When the master connection is handled, enter CONN_STATE_LINGER in any case.

Add missing APLOGNO

Submitted by: ylavic, jailletc36
Reviewed by: ylavic, icing, covener

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1820796 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 1d528ea7eb382cc3edccc6efdf84f3b3d34352fa..f1fe3cc8a005589bf069421657f9b227c40521b4 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,10 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.4.30
  
+  *) mpm_event: close connections not reported as handled by any module to
+     avoid losing track of them and leaking scoreboard entries.  PR 61551.
+     [Yann Ylavic]
+
   *) core: A signal received while stopping could have crashed the main
      process.  PR 61558.  [Yann Ylavic]
 

diff --git a/modules/http2/h2_conn.c b/modules/http2/h2_conn.c
index 53497d03efe24f8b5c3019db7c3a45d12b6b772e..f37d95172740924019a0b78da44b83ca018e3861 100644 (file)
--- a/modules/http2/h2_conn.c
+++ b/modules/http2/h2_conn.c
@@ -253,25 +253,12 @@ apr_status_t h2_conn_run(struct h2_ctx *ctx, conn_rec *c)
     } while (!async_mpm
              && c->keepalive == AP_CONN_KEEPALIVE 
              && mpm_state != AP_MPMQ_STOPPING);
-    
+
     if (c->cs) {
-        switch (session->state) {
-            case H2_SESSION_ST_INIT:
-            case H2_SESSION_ST_CLEANUP:
-            case H2_SESSION_ST_DONE:
-            case H2_SESSION_ST_IDLE:
-                c->cs->state = CONN_STATE_WRITE_COMPLETION;
-                break;
-            case H2_SESSION_ST_BUSY:
-            case H2_SESSION_ST_WAIT:
-            default:
-                c->cs->state = CONN_STATE_HANDLER;
-                break;
-                
-        }
+        c->cs->state = CONN_STATE_LINGER;
     }
-    
-    return DONE;
+
+    return APR_SUCCESS;
 }
 
 apr_status_t h2_conn_pre_close(struct h2_ctx *ctx, conn_rec *c)

diff --git a/modules/http2/h2_h2.c b/modules/http2/h2_h2.c
index 8487e413f4b44db1daa6b4ef3d64983baaff6cd4..f206beb7886ca0a4e91c48a91927159fa3feacdc 100644 (file)
--- a/modules/http2/h2_h2.c
+++ b/modules/http2/h2_h2.c
@@ -669,10 +669,11 @@ int h2_h2_process_conn(conn_rec* c)
             ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, c, "conn_setup");
             if (status != APR_SUCCESS) {
                 h2_ctx_clear(c);
-                return status;
+                return !OK;
             }
         }
-        return h2_conn_run(ctx, c);
+        h2_conn_run(ctx, c);
+        return OK;
     }
     
     ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c, "h2_h2, declined");

diff --git a/modules/http2/h2_switch.c b/modules/http2/h2_switch.c
index 3a8567eb1b0bd9d7434c02e9366fba4cab970b3d..ef6ae29e0802c12fb25a8065c89cf112e11e36c6 100644 (file)
--- a/modules/http2/h2_switch.c
+++ b/modules/http2/h2_switch.c
@@ -185,13 +185,12 @@ static int h2_protocol_switch(conn_rec *c, request_rec *r, server_rec *s,
                 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, status, r, APLOGNO(03088)
                               "session setup");
                 h2_ctx_clear(c);
-                return status;
+                return !OK;
             }
             
             h2_conn_run(ctx, c);
-            return DONE;
         }
-        return DONE;
+        return OK;
     }
     
     return DECLINED;

diff --git a/server/mpm/event/event.c b/server/mpm/event/event.c
index 2efde3982dee377a589a66edf10163802e1d50ff..54c9256074106424d3845d1816600991b1ec42bc 100644 (file)
--- a/server/mpm/event/event.c
+++ b/server/mpm/event/event.c
@@ -993,14 +993,12 @@ static void process_socket(apr_thread_t *thd, apr_pool_t * p, apr_socket_t * soc
         c->current_thread = thd;
         /* Subsequent request on a conn, and thread number is part of ID */
         c->id = conn_id;
-
-        if (c->aborted) {
-            cs->pub.state = CONN_STATE_LINGER;
-        }
     }
 
-    if (cs->pub.state == CONN_STATE_LINGER) {
+    rc = OK;
+    if (c->aborted || cs->pub.state == CONN_STATE_LINGER) {
         /* do lingering close below */
+        cs->pub.state = CONN_STATE_LINGER;
     }
     else if (c->clogging_input_filters) {
         /* Since we have an input filter which 'clogs' the input stream,
@@ -1010,20 +1008,54 @@ static void process_socket(apr_thread_t *thd, apr_pool_t * p, apr_socket_t * soc
          * otherwise write, should set the sense appropriately.
          */
         apr_atomic_inc32(&clogged_count);
-        ap_run_process_connection(c);
-        if (cs->pub.state != CONN_STATE_SUSPENDED) {
-            cs->pub.state = CONN_STATE_LINGER;
-        }
+        rc = ap_run_process_connection(c);
         apr_atomic_dec32(&clogged_count);
+        if (rc == DONE) {
+            rc = OK;
+        }
     }
     else if (cs->pub.state == CONN_STATE_READ_REQUEST_LINE) {
 read_request:
-        ap_run_process_connection(c);
-
-        /* state will be updated upon return
-         * fall thru to either wait for readability/timeout or
-         * do lingering close
-         */
+        rc = ap_run_process_connection(c);
+        if (rc == DONE) {
+            rc = OK;
+        }
+    }
+    /*
+     * The process_connection hooks above should set the connection state
+     * appropriately upon return, for event MPM to either:
+     * - do lingering close (CONN_STATE_LINGER),
+     * - wait for readability of the next request with respect to the keepalive
+     *   timeout (CONN_STATE_CHECK_REQUEST_LINE_READABLE),
+     * - keep flushing the output filters stack in nonblocking mode, and then
+     *   if required wait for read/write-ability of the underlying socket with
+     *   respect to its own timeout (CONN_STATE_WRITE_COMPLETION); since write
+     *   completion at some point may require reads (e.g. SSL_ERROR_WANT_READ),
+     *   an output filter can set the sense to CONN_SENSE_WANT_READ at any time
+     *   for event MPM to do the right thing,
+     * - suspend the connection (SUSPENDED) such that it now interracts with
+     *   the MPM through suspend/resume_connection() hooks, and/or registered
+     *   poll callbacks (PT_USER), and/or registered timed callbacks triggered
+     *   by timer events.
+     * If a process_connection hook returns an error or no hook sets the state
+     * to one of the above expected value, we forcibly close the connection w/
+     * CONN_STATE_LINGER.  This covers the cases where no process_connection
+     * hook executes (DECLINED), or one returns OK w/o touching the state (i.e.
+     * CONN_STATE_READ_REQUEST_LINE remains after the call) which can happen
+     * with third-party modules not updated to work specifically with event MPM
+     * while this was expected to do lingering close unconditionally with
+     * worker or prefork MPMs for instance.
+     */
+    if (rc != OK || (cs->pub.state != CONN_STATE_LINGER
+                     && cs->pub.state != CONN_STATE_WRITE_COMPLETION
+                     && cs->pub.state != CONN_STATE_CHECK_REQUEST_LINE_READABLE
+                     && cs->pub.state != CONN_STATE_SUSPENDED)) {
+        ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, APLOGNO(10111)
+                      "process_socket: connection processing %s: closing",
+                      rc ? apr_psprintf(c->pool, "returned error %i", rc)
+                         : apr_psprintf(c->pool, "unexpected state %i",
+                                                 (int)cs->pub.state));
+        cs->pub.state = CONN_STATE_LINGER;
     }
 
     if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
@@ -1046,10 +1078,20 @@ read_request:
              */
             cs->queue_timestamp = apr_time_now();
             notify_suspend(cs);
-            cs->pfd.reqevents = (
-                    cs->pub.sense == CONN_SENSE_WANT_READ ? APR_POLLIN :
-                            APR_POLLOUT) | APR_POLLHUP | APR_POLLERR;
+
+            if (cs->pub.sense == CONN_SENSE_WANT_READ) {
+                cs->pfd.reqevents = APR_POLLIN;
+            }
+            else {
+                cs->pfd.reqevents = APR_POLLOUT;
+            }
+            /* POLLHUP/ERR are usually returned event only (ignored here), but
+             * some pollset backends may require them in reqevents to do the
+             * right thing, so it shouldn't hurt.
+             */
+            cs->pfd.reqevents |= APR_POLLHUP | APR_POLLERR;
             cs->pub.sense = CONN_SENSE_DEFAULT;
+
             apr_thread_mutex_lock(timeout_mutex);
             TO_QUEUE_APPEND(cs->sc->wc_q, cs);
             rc = apr_pollset_add(event_pollset, &cs->pfd);