httpd-notifications mailing list archives

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

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



##########
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:
       Why changing the order?

##########
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:
       Is it wise to use `now` here or should we use a new temporary variable here to use
it for the `ap_log_error` parameters? Otherwise trace7 logs have side effects.

##########
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:
       Why now run `ap_run_input_pending` when `listener_may_exit` is set?

##########
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:
       Why now shuting down the process if adding to the pollset failed?

##########
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:
       Why is this no longer needed?

##########
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:
       Why now shuting down the process if removing the pollset failed?

##########
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:
       Why now `0` in case `expiry <= now` instead of `1` before?




-- 
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