Re: graceful-stop closes established connections without response

[Yann Ylavic <ylavic.dev@xxxxxxxxx>]

On Sun, Jan 28, 2024 at 5:26 AM Sherrard Burton <sburton@xxxxxxxxxxxxx> wrote:
>
> On 1/27/24 09:46 PM, Eric Covener wrote:
> >
> > Both worker and event MPMs have a dedicated listener thread per child
> > process, so it will close those copies of the listening sockets much
> > more quickly.
>
> so that i am clear, are you saying that this behavior is still possible,
> although less likely under the worker and event MPMs?

I think it's possible regardless of the MPM, and there is quite little
a server can do about it without the help of the system or some
tcp/bpf filter (something that prepares the graceful shutdown at the
system level to prevent the 3-way handshake from completing).
This is because when the connections are ready to be accept()ed (i.e.
in the listening socket's backlog), they are already fully established
and likely contain the request data (at least partly), the system has
done this underneath httpd already.
So if/when httpd closes its listening socket(s) all the connections in
the backlog(s) are lost/reset anyway, and there is always going to be
a race condition with the draining of the backlog if nothing stops new
connections from being established at the system level.

To minimize the race condition maybe httpd can do better at trying to
drain the backlog before closing the listeners. Does the attached
patch help for instance (it's against mpm_event 2.4.x)?
But I don't think it can be fully solved at httpd level anyway, with
this change the effective stop could be longer (so long as there are
incoming/pending connections routed to each child by the system), it
could even last forever theoretically if connections keep coming
indefinitely..

Regards;
Yann.

Index: server/mpm/event/event.c
===================================================================
--- server/mpm/event/event.c	(revision 1915442)
+++ server/mpm/event/event.c	(working copy)
@@ -174,7 +174,7 @@ static int had_healthy_child = 0;
 static volatile int dying = 0;
 static volatile int workers_may_exit = 0;
 static volatile int start_thread_may_exit = 0;
-static volatile int listener_may_exit = 0;
+static volatile apr_uint32_t listener_may_exit = 0;
 static int listener_is_wakeable = 0;        /* Pollset supports APR_POLLSET_WAKEABLE */
 static int num_listensocks = 0;
 static apr_int32_t conns_this_child;        /* MaxConnectionsPerChild, only access
@@ -481,8 +481,7 @@ static void disable_listensocks(void)
 static void enable_listensocks(void)
 {
     int i;
-    if (listener_may_exit
-            || apr_atomic_cas32(&listensocks_disabled, 0, 1) != 1) {
+    if (dying || apr_atomic_cas32(&listensocks_disabled, 0, 1) != 1) {
         return;
     }
     ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf, APLOGNO(00457)
@@ -575,8 +574,7 @@ static void wakeup_listener(void)
     ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf,
                  "wake up listener%s", listener_may_exit ? " again" : "");
 
-    listener_may_exit = 1;
-    disable_listensocks();
+    apr_atomic_cas32(&listener_may_exit, 1, 0);
 
     /* Unblock the listener if it's poll()ing */
     if (event_pollset && listener_is_wakeable) {
@@ -1184,12 +1182,9 @@ read_request:
             cs->pub.state = CONN_STATE_READ_REQUEST_LINE;
             goto read_request;
         }
-        else if (!listener_may_exit) {
+        else {
             cs->pub.state = CONN_STATE_CHECK_REQUEST_LINE_READABLE;
         }
-        else {
-            cs->pub.state = CONN_STATE_LINGER;
-        }
     }
 
     if (cs->pub.state == CONN_STATE_CHECK_REQUEST_LINE_READABLE) {
@@ -1654,7 +1649,7 @@ static void * APR_THREAD_FUNC listener_thread(apr_
     proc_info *ti = dummy;
     int process_slot = ti->pslot;
     struct process_score *ps = ap_get_scoreboard_process(process_slot);
-    int closed = 0;
+    int may_exit = 0, closed = 0;
     int have_idle_worker = 0;
     apr_time_t last_log;
 
@@ -1678,7 +1673,7 @@ static void * APR_THREAD_FUNC listener_thread(apr_
         if (conns_this_child <= 0)
             check_infinite_requests();
 
-        if (listener_may_exit) {
+        if (may_exit) {
             int first_close = close_listeners(&closed);
 
             if (terminate_mode == ST_UNGRACEFUL
@@ -1899,7 +1894,7 @@ static void * APR_THREAD_FUNC listener_thread(apr_
                                  "Idle workers: %u",
                                  ap_queue_info_num_idlers(worker_queue_info));
                 }
-                else if (!listener_may_exit) {
+                else {
                     void *csd = NULL;
                     ap_listen_rec *lr = (ap_listen_rec *) pt->baton;
                     apr_pool_t *ptrans;         /* Pool for per-transaction stuff */
@@ -1960,6 +1955,14 @@ static void * APR_THREAD_FUNC listener_thread(apr_
             }               /* if:else on pt->type */
         } /* for processing poll */
 
+        /* On graceful shutdown/stop we can close the listening sockets
+         * since the backlog should be drained now.
+         */
+        if (apr_atomic_cas32(&listener_may_exit, 2, 1) == 1) {
+            may_exit = 1;
+            continue;
+        }
+
         /* We process the timeout queues here only when the global
          * queues_next_expiry is passed. This happens accurately since
          * adding to the queues (in workers) can only decrease this expiry,

---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@xxxxxxxxxxxxxxxx
For additional commands, e-mail: users-help@xxxxxxxxxxxxxxxx