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

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

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

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

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