From: Ondřej Surý Date: Mon, 29 Aug 2022 10:11:37 +0000 (+0200) Subject: Reorder the uv_close() calls to close the socket immediately X-Git-Tag: v9.19.6~54^2~1 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=9b8d4324037ca95913d7411e19c1ed78a02fc2b4;p=thirdparty%2Fbind9.git 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 82953cb0d92..1fd4bc3e6ee 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 172d5b07402..47695e15289 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 3ad86a39793..73826ca9c59 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 ff7cc4163dd..58f31edc551 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