httpd-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [httpd] ylavic commented on a change in pull request #208: Event child graceful stop
Date Tue, 20 Jul 2021 12:21:15 GMT

ylavic commented on a change in pull request #208:
URL: https://github.com/apache/httpd/pull/208#discussion_r672226382



##########
File path: server/mpm/event/event.c
##########
@@ -1020,14 +1011,14 @@ static void process_socket(apr_thread_t *thd, apr_pool_t * p, apr_socket_t
* soc
                                   apr_pool_cleanup_null);
         ap_set_module_config(c->conn_config, &mpm_event_module, cs);
         c->current_thread = thd;
+        c->cs = &cs->pub;
         cs->c = c;
-        c->cs = &(cs->pub);
         cs->p = p;

Review comment:
       Not necessary, just the initialization of all the `c` fields and then the `cs` ones
which I grouped together by reading/understanding the code. I can revert this.

##########
File path: server/mpm/event/event.c
##########
@@ -1189,17 +1180,19 @@ static void process_socket(apr_thread_t *thd, apr_pool_t * p, apr_socket_t
* soc
         }
         if (pending != DECLINED
                 || c->aborted
-                || c->keepalive != AP_CONN_KEEPALIVE
-                || listener_may_exit) {
+                || c->keepalive != AP_CONN_KEEPALIVE) {
             cs->pub.state = CONN_STATE_LINGER;
         }
         else if (ap_run_input_pending(c) == OK) {
             cs->pub.state = CONN_STATE_READ_REQUEST_LINE;
             goto read_request;
         }
-        else {

Review comment:
       If ap_run_input_pending() then the next request is already here (or at least part of
it), lingering close will shutdown the connection and read it fully before closing (without
a response).
   I thought it was too interruptive so the new behaviour would handle such pipelined requests
up to `MaxKeepAliveRequests` (or untilpipelining stops), which sounds better to me. I could
be wrong..
   

##########
File path: server/mpm/event/event.c
##########
@@ -1228,8 +1221,8 @@ static void process_socket(apr_thread_t *thd, apr_pool_t * p, apr_socket_t
* soc
             ap_log_error(APLOG_MARK, APLOG_ERR, rv, ap_server_conf, APLOGNO(03093)
                          "process_socket: apr_pollset_add failure for "
                          "keep alive");
-            apr_socket_close(cs->pfd.desc.s);
-            ap_queue_info_push_pool(worker_queue_info, cs->p);
+            close_connection(cs);
+            signal_threads(ST_GRACEFUL);

Review comment:
       If adding to or removing from the pollset fails (besides EEXIST or NOTFOUND resp.)
then evreything becomes inconsistent and further add/remove are likely to fail too, that's
an unrecoverable system error to me. Let's the process be restarted?

##########
File path: server/mpm/event/event.c
##########
@@ -1921,37 +1961,40 @@ static void * APR_THREAD_FUNC listener_thread(apr_thread_t * thd,
void *dummy)
         }
 
         /* Same for queues, use their next expiry, if any. */
-        timeout_time = queues_next_expiry;
-        if (timeout_time
-                && (timeout_interval < 0
-                    || timeout_time <= now
-                    || timeout_interval > timeout_time - now)) {
-            timeout_interval = timeout_time > now ? timeout_time - now : 1;
+        expiry = queues_next_expiry;
+        if (expiry
+                && (timeout < 0
+                    || expiry <= now
+                    || timeout > expiry - now)) {
+            timeout = expiry > now ? expiry - now : 0;

Review comment:
       Below rounding up to milliseconds checks for timeout > 0, which shouldn't apply
here.
   Here we can afford a nonblocking poll() if the timeout already expired, this is `0` (`1`
was almost that, I put `1` before here because I wasn't sure that `0` would work as expected,
but it seems to by reading apr_pollset implementations).

##########
File path: server/mpm/event/event.c
##########
@@ -1961,13 +2004,21 @@ static void * APR_THREAD_FUNC listener_thread(apr_thread_t * thd,
void *dummy)
             num = 0;
         }
 
-        if (listener_may_exit) {
-            close_listeners(&closed);
-            if (terminate_mode == ST_UNGRACEFUL
-                || apr_atomic_read32(&connection_count) == 0)
-                break;
+        if (APLOGtrace7(ap_server_conf)) {
+            now = apr_time_now();

Review comment:
       `now` is (re)set to `apr_time_now()` just before it's used (everywhere) in listener
loop, `now` before and after the poll() are different anyway. So it's indeed a side affect
but just like ap_log_error() itself when the LogLevel is really trace7.
   But I agree that the handling of `now` is not very nice in the listener, too many `now
= apr_time_now();` (including for the `goto do_maintenance;`, I'll see if I can write this
better.

##########
File path: server/mpm/event/event.c
##########
@@ -2020,25 +2071,21 @@ static void * APR_THREAD_FUNC listener_thread(apr_thread_t * thd,
void *dummy)
                         AP_DEBUG_ASSERT(0);
                         ap_log_error(APLOG_MARK, APLOG_ERR, rc, ap_server_conf,
                                      APLOGNO(03094) "pollset remove failed");
-                        start_lingering_close_nonblocking(cs);
+                        close_connection(cs);
+                        signal_threads(ST_GRACEFUL);

Review comment:
       Same reason as above, let's discuss there if you don't mind.

##########
File path: server/mpm/event/event.c
##########
@@ -2249,7 +2303,6 @@ static void * APR_THREAD_FUNC listener_thread(apr_thread_t * thd, void
*dummy)
         }
     } /* listener main loop */
 
-    close_listeners(&closed);

Review comment:
       Single point of exit in the listener loop now, and close_listeners() is already called
there.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org


Mime
View raw message