Re: [Bug 61551] Event MPM workers stuck in Gracefully Finishing with no connections left

[Yann Ylavic <yl...@gmail.com>]

On Sun, Oct 1, 2017 at 2:50 PM,  <bu...@apache.org> wrote:
> https://bz.apache.org/bugzilla/show_bug.cgi?id=61551
>
> --- Comment #18 from Eric Covener <co...@gmail.com> ---
>
> Maybe a bit premature to ask mod_Security to make a change API wise.
>
> Looks like a process_connection() hook could complete without changing
> the state and there is no catch-all for that in process_socket(). Yann,
> probably up your alley how we could ditch that connection.  Hard for
> mod_security to know we expect it to do anything (set it to linger? How about
> allowing errors returned by process_connection?

I came up to the same solution, how about:

Index: server/mpm/event/event.c
===================================================================
--- server/mpm/event/event.c    (revision 1814399)
+++ server/mpm/event/event.c    (working copy)
@@ -1010,8 +1010,8 @@ static void process_socket(apr_thread_t *thd, apr_
          * otherwise write, should set the sense appropriately.
          */
         apr_atomic_inc32(&clogged_count);
-        ap_run_process_connection(c);
-        if (cs->pub.state != CONN_STATE_SUSPENDED) {
+        rc = ap_run_process_connection(c);
+        if (rc || cs->pub.state != CONN_STATE_SUSPENDED) {
             cs->pub.state = CONN_STATE_LINGER;
         }
         apr_atomic_dec32(&clogged_count);
@@ -1018,12 +1018,23 @@ static void process_socket(apr_thread_t *thd, apr_
     }
     else if (cs->pub.state == CONN_STATE_READ_REQUEST_LINE) {
 read_request:
-        ap_run_process_connection(c);
+        rc = ap_run_process_connection(c);

-        /* state will be updated upon return
-         * fall thru to either wait for readability/timeout or
-         * do lingering close
+        /* State will be updated upon successful return: fall thru to either
+         * wait for readability/timeout, do write completion or lingering
+         * close. But forcibly close the connection if the run failed (handler
+         * raised an error for it) or the state is something unexpected at the
+         * MPM level (meaning that no module handled it and we can't do much
+         * here; note that if a handler wants mpm_event to keep POLLIN for the
+         * rest of the request line it should use CHECK_REQUEST_LINE_READABLE
+         * and not simply return OK with the initial READ_REQUEST_LINE state).
          */
+        if (rc || (cs->pub.state != CONN_STATE_CHECK_REQUEST_LINE_READABLE
+                   && cs->pub.state != CONN_STATE_WRITE_COMPLETION
+                   && cs->pub.state != CONN_STATE_SUSPENDED
+                   && cs->pub.state != CONN_STATE_LINGER)) {
+            cs->pub.state = CONN_STATE_LINGER;
+        }
     }

     if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
?

It shouldn't break anything since no module/handler can currently
return any other state without mpm_event to simply forget about the
connection (leak it).
I also don't see why we should continue if some process_connection hook failed.

That should address the mod_security case I think.

Thoughts?