From 23800ecd863be7166ad37ebd3c3d8e88ab140b97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 29 Aug 2022 14:30:54 +0200 Subject: [PATCH 1/3] Add developer note for the libuv quirks --- doc/dev/libuv.md | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 doc/dev/libuv.md diff --git a/doc/dev/libuv.md b/doc/dev/libuv.md new file mode 100644 index 0000000000..0650889322 --- /dev/null +++ b/doc/dev/libuv.md @@ -0,0 +1,46 @@ + + +## Libuv Notes + +This document describes various notes related to the using of the libuv library. + +### Queueing Events onto the ``uv_loop_t`` + +The upstream documentation on [the I/O +loop](http://docs.libuv.org/en/v1.x/design.html#the-i-o-loop) describes the +order in which are the various handles processed. However, it does not describe +the order in which the loop processes the events in the same buckets, and +because it is counterintuitive, it is described here. + +When scheduling the events of the same class (f.e. ``uv_*_start()`` or +``uv_close()``), the events are executed in the LIFO order (e.g. it's a stack, +not a queue). The reasoning for the upstream design choice is described in [the +upstream issue](https://github.com/libuv/libuv/issues/3582). + +What does this means in practice? F.e. when closing the handles: + + uv_close(&handle1, callback1); + uv_close(&handle2, callback2); + +The ``callback2()`` will be called before the ``callback1()``, so if they are +using the same resource, the resource can be freed in the ``callback1()`` and +not in the ``callback2()``. + +Same applies f.e. to the ``uv_idle_t``, if you want the ``action1()`` to execute +before ``action2()``, the valid code would be: + + uv_idle_start(&idle2, action2); + uv_idle_start(&idle1, action1); + +which is really counterintuitive. 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 2/3] 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 From b1026dd4c122b35706a65dba72f7739e2f3f1c9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 29 Aug 2022 13:42:14 +0200 Subject: [PATCH 3/3] Add missing isc_refcount_destroy() for isc__nmsocket_t The destructor for the isc__nmsocket_t was missing call to the isc_refcount_destroy() on the reference counter, which might lead to spurious ThreadSanitizer data race warnings if we ever change the acquire-release memory order in the isc_refcount_decrement(). --- lib/isc/netmgr/netmgr.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index 5f3a17f12c..14f5d2e832 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -665,6 +665,8 @@ nmsocket_cleanup(isc_nmsocket_t *sock, bool dofree FLARG) { "\n", sock, isc_refcount_current(&sock->references)); + isc_refcount_destroy(&sock->references); + isc__nm_decstats(sock, STATID_ACTIVE); atomic_store(&sock->destroying, true); @@ -675,7 +677,9 @@ nmsocket_cleanup(isc_nmsocket_t *sock, bool dofree FLARG) { * so we can clean up and free the children. */ for (size_t i = 0; i < sock->nchildren; i++) { - if (!atomic_load(&sock->children[i].destroying)) { + REQUIRE(!atomic_load(&sock->children[i].destroying)); + if (isc_refcount_decrement( + &sock->children[i].references)) { nmsocket_cleanup(&sock->children[i], false FLARG_PASS); }