Reorder the uv_close() calls to close the socket immediately

Simplify the closing code - during the loopmgr implementation, it was
discovered that the various lists used by the uv_loop_t aren't FIFO, but
LIFO.  See doc/dev/libuv.md for more details.

With this knowledge, we can close the protocol handles (uv_udp_t and
uv_tcp_t) and uv_timer_t at the same time by reordering the uv_close()
calls, and thus making sure that after calling the
isc__nm_stoplistening(), the code will not issue any additional callback
calls (accept, read) on the socket that stopped listening.

This might help with the TLS and DoH shutting down sequence as described
in the [GL #3509] as we now stop the reading, stop the timer and call
the uv_close() as earliest as possible.

diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c
index 82953cb0d925334624c91aa2443de4bb3be179db..1fd4bc3e6eeedfc4a20e10208f5ee4c6b4de0016 100644 (file)
--- a/lib/isc/netmgr/tcp.c
+++ b/lib/isc/netmgr/tcp.c
@@ -62,6 +62,8 @@ static isc_result_t
 tcp_send_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req);
 static void
 tcp_connect_cb(uv_connect_t *uvreq, int status);
+static void
+tcp_stop_cb(uv_handle_t *handle);
 
 static void
 tcp_connection_cb(uv_stream_t *server, int status);
@@ -683,7 +685,20 @@ isc__nm_async_tcpstop(isc__networker_t *worker, isc__netievent_t *ev0) {
        RUNTIME_CHECK(atomic_compare_exchange_strong(&sock->closing,
                                                     &(bool){ false }, true));
 
-       tcp_close_direct(sock);
+       /*
+        * The order of the close operation is important here, the uv_close()
+        * gets scheduled in the reverse order, so we need to close the timer
+        * last, so its gone by the time we destroy the socket
+        */
+
+       /* 2. close the listening socket */
+       isc__nmsocket_clearcb(sock);
+       isc__nm_stop_reading(sock);
+       uv_close(&sock->uv_handle.handle, tcp_stop_cb);
+
+       /* 1. close the read timer */
+       isc__nmsocket_timer_stop(sock);
+       uv_close(&sock->read_timer, NULL);
 
        (void)atomic_fetch_sub(&sock->parent->rchildren, 1);
 
@@ -1217,25 +1232,12 @@ tcp_close_cb(uv_handle_t *handle) {
        tcp_close_sock(sock);
 }
 
-static void
-read_timer_close_cb(uv_handle_t *handle) {
-       isc_nmsocket_t *sock = uv_handle_get_data(handle);
-       uv_handle_set_data(handle, NULL);
-
-       if (sock->parent) {
-               uv_close(&sock->uv_handle.handle, tcp_stop_cb);
-       } else if (uv_is_closing(&sock->uv_handle.handle)) {
-               tcp_close_sock(sock);
-       } else {
-               uv_close(&sock->uv_handle.handle, tcp_close_cb);
-       }
-}
-
 static void
 tcp_close_direct(isc_nmsocket_t *sock) {
        REQUIRE(VALID_NMSOCK(sock));
        REQUIRE(sock->tid == isc_tid());
        REQUIRE(atomic_load(&sock->closing));
+       REQUIRE(sock->parent == NULL);
 
        if (sock->server != NULL) {
                REQUIRE(VALID_NMSOCK(sock->server));
@@ -1251,12 +1253,31 @@ tcp_close_direct(isc_nmsocket_t *sock) {
                isc_quota_detach(&sock->quota);
        }
 
-       isc__nmsocket_clearcb(sock);
-       isc__nmsocket_timer_stop(sock);
-       isc__nm_stop_reading(sock);
+       /*
+        * The order of the close operation is important here, the uv_close()
+        * gets scheduled in the reverse order, so we need to close the timer
+        * last, so its gone by the time we destroy the socket
+        */
 
-       uv_handle_set_data((uv_handle_t *)&sock->read_timer, sock);
-       uv_close((uv_handle_t *)&sock->read_timer, read_timer_close_cb);
+       if (!uv_is_closing(&sock->uv_handle.handle)) {
+               /* Normal order of operation */
+
+               /* 2. close the socket + destroy the socket in callback */
+               isc__nmsocket_clearcb(sock);
+               isc__nm_stop_reading(sock);
+               uv_close(&sock->uv_handle.handle, tcp_close_cb);
+
+               /* 1. close the timer */
+               isc__nmsocket_timer_stop(sock);
+               uv_close((uv_handle_t *)&sock->read_timer, NULL);
+       } else {
+               /* The socket was already closed elsewhere */
+
+               /* 1. close the timer + destroy the socket in callback */
+               isc__nmsocket_timer_stop(sock);
+               uv_handle_set_data((uv_handle_t *)&sock->read_timer, sock);
+               uv_close((uv_handle_t *)&sock->read_timer, tcp_close_cb);
+       }
 }
 
 void

diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c
index 172d5b07402a515e8717c3982058b599844fe61e..47695e15289da7fab70be495e3dfc117a7f22a24 100644 (file)
--- a/lib/isc/netmgr/tcpdns.c
+++ b/lib/isc/netmgr/tcpdns.c
@@ -63,6 +63,9 @@ tcpdns_connect_cb(uv_connect_t *uvreq, int status);
 static void
 tcpdns_connection_cb(uv_stream_t *server, int status);
 
+static void
+tcpdns_stop_cb(uv_handle_t *handle);
+
 static void
 tcpdns_close_cb(uv_handle_t *uvhandle);
 
@@ -646,7 +649,20 @@ isc__nm_async_tcpdnsstop(isc__networker_t *worker, isc__netievent_t *ev0) {
        RUNTIME_CHECK(atomic_compare_exchange_strong(&sock->closing,
                                                     &(bool){ false }, true));
 
-       tcpdns_close_direct(sock);
+       /*
+        * The order of the close operation is important here, the uv_close()
+        * gets scheduled in the reverse order, so we need to close the timer
+        * last, so its gone by the time we destroy the socket
+        */
+
+       /* 2. close the listening socket */
+       isc__nmsocket_clearcb(sock);
+       isc__nm_stop_reading(sock);
+       uv_close(&sock->uv_handle.handle, tcpdns_stop_cb);
+
+       /* 1. close the read timer */
+       isc__nmsocket_timer_stop(sock);
+       uv_close(&sock->read_timer, NULL);
 
        (void)atomic_fetch_sub(&sock->parent->rchildren, 1);
 
@@ -1270,22 +1286,6 @@ tcpdns_close_cb(uv_handle_t *handle) {
        tcpdns_close_sock(sock);
 }
 
-static void
-read_timer_close_cb(uv_handle_t *timer) {
-       isc_nmsocket_t *sock = uv_handle_get_data(timer);
-       uv_handle_set_data(timer, NULL);
-
-       REQUIRE(VALID_NMSOCK(sock));
-
-       if (sock->parent) {
-               uv_close(&sock->uv_handle.handle, tcpdns_stop_cb);
-       } else if (uv_is_closing(&sock->uv_handle.handle)) {
-               tcpdns_close_sock(sock);
-       } else {
-               uv_close(&sock->uv_handle.handle, tcpdns_close_cb);
-       }
-}
-
 static void
 tcpdns_close_direct(isc_nmsocket_t *sock) {
        REQUIRE(VALID_NMSOCK(sock));
@@ -1300,12 +1300,30 @@ tcpdns_close_direct(isc_nmsocket_t *sock) {
                isc_nmhandle_detach(&sock->recv_handle);
        }
 
-       isc__nmsocket_clearcb(sock);
-       isc__nmsocket_timer_stop(sock);
-       isc__nm_stop_reading(sock);
+       /*
+        * The order of the close operation is important here, the uv_close()
+        * gets scheduled in the reverse order, so we need to close the timer
+        * last, so its gone by the time we destroy the socket
+        */
 
-       uv_handle_set_data((uv_handle_t *)&sock->read_timer, sock);
-       uv_close((uv_handle_t *)&sock->read_timer, read_timer_close_cb);
+       if (!uv_is_closing(&sock->uv_handle.handle)) {
+               /* Normal order of operation */
+
+               /* 2. close the socket + destroy the socket in callback */
+               isc__nmsocket_clearcb(sock);
+               isc__nm_stop_reading(sock);
+               uv_close(&sock->uv_handle.handle, tcpdns_close_cb);
+
+               /* 1. close the timer */
+               uv_close((uv_handle_t *)&sock->read_timer, NULL);
+       } else {
+               /* The socket was already closed elsewhere */
+
+               /* 1. close the timer + destroy the socket in callback */
+               isc__nmsocket_timer_stop(sock);
+               uv_handle_set_data((uv_handle_t *)&sock->read_timer, sock);
+               uv_close((uv_handle_t *)&sock->read_timer, tcpdns_close_cb);
+       }
 }
 
 void

diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c
index 3ad86a39793ca8e140190177e54089a3432805b7..73826ca9c59ff35747674bc88f5ce4ebc98923b4 100644 (file)
--- a/lib/isc/netmgr/tlsdns.c
+++ b/lib/isc/netmgr/tlsdns.c
@@ -56,6 +56,9 @@ tlsdns_connect_cb(uv_connect_t *uvreq, int status);
 static void
 tlsdns_connection_cb(uv_stream_t *server, int status);
 
+static void
+tlsdns_stop_cb(uv_handle_t *handle);
+
 static void
 tlsdns_close_cb(uv_handle_t *uvhandle);
 
@@ -769,7 +772,20 @@ isc__nm_async_tlsdnsstop(isc__networker_t *worker, isc__netievent_t *ev0) {
        RUNTIME_CHECK(atomic_compare_exchange_strong(&sock->closing,
                                                     &(bool){ false }, true));
 
-       tlsdns_close_direct(sock);
+       /*
+        * The order of the close operation is important here, the uv_close()
+        * gets scheduled in the reverse order, so we need to close the timer
+        * last, so its gone by the time we destroy the socket
+        */
+
+       /* 2. close the listening socket */
+       isc__nmsocket_clearcb(sock);
+       isc__nm_stop_reading(sock);
+       uv_close(&sock->uv_handle.handle, tlsdns_stop_cb);
+
+       /* 1. close the read timer */
+       isc__nmsocket_timer_stop(sock);
+       uv_close(&sock->read_timer, NULL);
 
        (void)atomic_fetch_sub(&sock->parent->rchildren, 1);
 
@@ -1908,27 +1924,12 @@ tlsdns_close_cb(uv_handle_t *handle) {
        tlsdns_close_sock(sock);
 }
 
-static void
-read_timer_close_cb(uv_handle_t *handle) {
-       isc_nmsocket_t *sock = uv_handle_get_data(handle);
-       uv_handle_set_data(handle, NULL);
-
-       REQUIRE(VALID_NMSOCK(sock));
-
-       if (sock->parent) {
-               uv_close(&sock->uv_handle.handle, tlsdns_stop_cb);
-       } else if (uv_is_closing(&sock->uv_handle.handle)) {
-               tlsdns_close_sock(sock);
-       } else {
-               uv_close(&sock->uv_handle.handle, tlsdns_close_cb);
-       }
-}
-
 static void
 tlsdns_close_direct(isc_nmsocket_t *sock) {
        REQUIRE(VALID_NMSOCK(sock));
        REQUIRE(sock->tid == isc_tid());
        REQUIRE(atomic_load(&sock->closing));
+       REQUIRE(sock->parent == NULL);
 
        REQUIRE(sock->tls.pending_req == NULL);
 
@@ -1940,12 +1941,31 @@ tlsdns_close_direct(isc_nmsocket_t *sock) {
                isc_nmhandle_detach(&sock->recv_handle);
        }
 
-       isc__nmsocket_clearcb(sock);
-       isc__nmsocket_timer_stop(sock);
-       isc__nm_stop_reading(sock);
+       /*
+        * The order of the close operation is important here, the uv_close()
+        * gets scheduled in the reverse order, so we need to close the timer
+        * last, so its gone by the time we destroy the socket
+        */
 
-       uv_handle_set_data((uv_handle_t *)&sock->read_timer, sock);
-       uv_close((uv_handle_t *)&sock->read_timer, read_timer_close_cb);
+       if (!uv_is_closing(&sock->uv_handle.handle)) {
+               /* Normal order of operation */
+
+               /* 2. close the socket + destroy the socket in callback */
+               isc__nmsocket_clearcb(sock);
+               isc__nm_stop_reading(sock);
+               uv_close(&sock->uv_handle.handle, tlsdns_close_cb);
+
+               /* 1. close the timer */
+               isc__nmsocket_timer_stop(sock);
+               uv_close((uv_handle_t *)&sock->read_timer, NULL);
+       } else {
+               /* The socket was already closed elsewhere */
+
+               /* 1. close the timer + destroy the socket in callback */
+               isc__nmsocket_timer_stop(sock);
+               uv_handle_set_data((uv_handle_t *)&sock->read_timer, sock);
+               uv_close((uv_handle_t *)&sock->read_timer, tlsdns_close_cb);
+       }
 }
 
 void

diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c
index ff7cc4163dd137da98ec952b2de6aae2a692c741..58f31edc551bb4be4fa601ff659810251cc3e37b 100644 (file)
--- a/lib/isc/netmgr/udp.c
+++ b/lib/isc/netmgr/udp.c
@@ -65,9 +65,6 @@ udp_send_cb(uv_udp_send_t *req, int status);
 static void
 udp_close_cb(uv_handle_t *handle);
 
-static void
-read_timer_close_cb(uv_handle_t *handle);
-
 static uv_os_sock_t
 isc__nm_udp_lb_socket(isc_nm_t *mgr, sa_family_t sa_family) {
        isc_result_t result;
@@ -1008,14 +1005,6 @@ udp_close_cb(uv_handle_t *handle) {
        }
 }
 
-static void
-read_timer_close_cb(uv_handle_t *handle) {
-       isc_nmsocket_t *sock = uv_handle_get_data(handle);
-       uv_handle_set_data(handle, NULL);
-
-       uv_close(&sock->uv_handle.handle, udp_close_cb);
-}
-
 void
 isc__nm_udp_close(isc_nmsocket_t *sock) {
        REQUIRE(VALID_NMSOCK(sock));
@@ -1031,7 +1020,20 @@ isc__nm_udp_close(isc_nmsocket_t *sock) {
        isc__nmsocket_timer_stop(sock);
        isc__nm_stop_reading(sock);
 
-       uv_close((uv_handle_t *)&sock->read_timer, read_timer_close_cb);
+       /*
+        * The order of the close operation is important here, the uv_close()
+        * gets scheduled in the reverse order, so we need to close the timer
+        * last, so its gone by the time we destroy the socket
+        */
+
+       /* 2. close the listening socket */
+       isc__nmsocket_clearcb(sock);
+       isc__nm_stop_reading(sock);
+       uv_close(&sock->uv_handle.handle, udp_close_cb);
+
+       /* 1. close the read timer */
+       isc__nmsocket_timer_stop(sock);
+       uv_close((uv_handle_t *)&sock->read_timer, NULL);
 }
 
 void