From 9b8d4324037ca95913d7411e19c1ed78a02fc2b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 29 Aug 2022 12:11:37 +0200 Subject: [PATCH] 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. --- lib/isc/netmgr/tcp.c | 61 ++++++++++++++++++++++++++------------- lib/isc/netmgr/tcpdns.c | 62 +++++++++++++++++++++++++-------------- lib/isc/netmgr/tlsdns.c | 64 +++++++++++++++++++++++++++-------------- lib/isc/netmgr/udp.c | 26 +++++++++-------- 4 files changed, 137 insertions(+), 76 deletions(-) diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 82953cb0d9..1fd4bc3e6e 100644 --- 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 172d5b0740..47695e1528 100644 --- 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 3ad86a3979..73826ca9c5 100644 --- 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 ff7cc4163d..58f31edc55 100644 --- 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