From 4b7c61381f186e20a476c35032a871295ebbd385 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 2 Jul 2024 20:17:49 +0200 Subject: [PATCH 1/3] Throttle the reading when writes are asynchronous Be more aggressive when throttling the reading - when we can't send the outgoing TCP synchronously with uv_try_write(), we start throttling the reading immediately instead of waiting for the send buffers to fill up. This should not affect behaved clients that read the data from the TCP on the other end. (cherry picked from commit bc3e713317df838b4bbe6582b357cd8d78e585cd) --- lib/isc/netmgr/tcpdns.c | 7 +++++++ lib/isc/netmgr/tlsdns.c | 8 ++++++++ 2 files changed, 15 insertions(+) diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index d7ec755bec..639ff7af02 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -1284,6 +1284,13 @@ isc__nm_async_tcpdnssend(isc__networker_t *worker, isc__netievent_t *ev0) { goto fail; } + isc_log_write(isc_lctx, ISC_LOGCATEGORY_GENERAL, ISC_LOGMODULE_NETMGR, + ISC_LOG_DEBUG(3), + "throttling TCP connection, the other side is not " + "reading the data, switching to uv_write()"); + sock->reading_throttled = true; + isc__nm_stop_reading(sock); + r = uv_write(&uvreq->uv_req.write, &sock->uv_handle.stream, bufs, nbufs, tcpdns_send_cb); if (r < 0) { diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c index b41c353842..dbcd34b8ed 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -1377,6 +1377,14 @@ tls_cycle_output(isc_nmsocket_t *sock) { break; } + isc_log_write( + isc_lctx, ISC_LOGCATEGORY_GENERAL, ISC_LOGMODULE_NETMGR, + ISC_LOG_DEBUG(3), + "throttling TCP connection, the other side is not " + "reading the data, switching to uv_write()"); + sock->reading_throttled = true; + isc__nm_stop_reading(sock); + r = uv_write(&req->uv_req.write, &sock->uv_handle.stream, &req->uvbuf, 1, tls_write_cb); if (r < 0) { From e31190e704e7c9d571c3d16e8580040062f28e06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 2 Jul 2024 20:17:49 +0200 Subject: [PATCH 2/3] Reset the TCP connection on a failed send When sending fails, the ns__client_request() would not reset the connection and continue as nothing is happening. This comes from the model that we don't care about failed UDP sends because datagrams are unreliable anyway, but it greatly affects TCP connections with keep-alive. The worst case scenario is as follows: 1. the 3-way TCP handshake gets completed 2. the libuv calls the "uv_connection_cb" callback 3. the TCP connection gets queue because of the tcp-clients quota 4. the TCP client sends as many DNS messages as the buffers allow 5. the TCP connection gets dropped by the client due to the timeout 6. the TCP connection gets accepted by the server 7. the data already sent by the client gets read 8. all sending fails immediately because the TCP connection is dead 9. we consume all the data in the buffer in a very tight loop As it doesn't make sense to trying to process more data on the TCP connection when the sending is failing, drop the connection immediately on the first sending error. (cherry picked from commit bf9fd2a6ff1a81ee2974ad984fda71e096ffb6fb) --- lib/ns/client.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/ns/client.c b/lib/ns/client.c index 002185d642..5d2ad0bdbf 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -333,6 +333,7 @@ client_senddone(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(3), "send failed: %s", isc_result_totext(result)); + isc_nm_bad_request(handle); } } From c33b3d26f695d342af3fa81ab404a366bb8ce873 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 3 Jul 2024 13:58:32 +0300 Subject: [PATCH 3/3] TCP/TLS DNS: unthrottle only when all input data processing This commit ensures that we restart reading only when all DNS data in the input buffer is processed so the we will not get into the situation when the buffer is overrun. --- lib/isc/netmgr/netmgr.c | 36 ++++++++++++++++++++++++++++++++---- lib/isc/netmgr/tcpdns.c | 23 ++++------------------- lib/isc/netmgr/tlsdns.c | 23 ++++------------------- 3 files changed, 40 insertions(+), 42 deletions(-) diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index 01c770d56a..a42ca90e8d 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -2369,14 +2369,41 @@ isc__nm_process_sock_buffer(isc_nmsocket_t *sock) { int_fast32_t ah = atomic_load(&sock->ah); isc_result_t result = processbuffer(sock); switch (result) { - case ISC_R_NOMORE: + case ISC_R_NOMORE: { /* * Don't reset the timer until we have a * full DNS message. */ - result = isc__nm_start_reading(sock); - if (result != ISC_R_SUCCESS) { - return (result); + + /* + * Restart reading if we have less data in the send + * queue than the send buffer size, this means that the + * TCP client has started reading some data again. + * Starting reading when we go under the limit instead + * of waiting for all data has been flushed allows + * faster recovery (in case there was a congestion and + * now there isn't). + */ + size_t write_queue_size = + uv_stream_get_write_queue_size( + &sock->uv_handle.stream); + if (write_queue_size < ISC_NETMGR_TCP_SENDBUF_SIZE) { + if (sock->reading_throttled) { + isc_log_write(isc_lctx, + ISC_LOGCATEGORY_GENERAL, + ISC_LOGMODULE_NETMGR, + ISC_LOG_DEBUG(3), + "resuming TCP " + "connection, the other " + "side is reading the " + "data again (%zu)", + write_queue_size); + sock->reading_throttled = false; + } + result = isc__nm_start_reading(sock); + if (result != ISC_R_SUCCESS) { + return (result); + } } /* * Start the timer only if there are no externally used @@ -2388,6 +2415,7 @@ isc__nm_process_sock_buffer(isc_nmsocket_t *sock) { isc__nmsocket_timer_start(sock); } goto done; + } case ISC_R_CANCELED: isc__nmsocket_timer_stop(sock); isc__nm_stop_reading(sock); diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index 639ff7af02..4b5ee58b6a 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -1162,25 +1162,10 @@ tcpdns_maybe_restart_reading(isc_nmsocket_t *sock) { if (!sock->client && sock->reading_throttled && !uv_is_active(&sock->uv_handle.handle)) { - /* - * Restart reading if we have less data in the send queue than - * the send buffer size, this means that the TCP client has - * started reading some data again. Starting reading when we go - * under the limit instead of waiting for all data has been - * flushed allows faster recovery (in case there was a - * congestion and now there isn't). - */ - size_t write_queue_size = - uv_stream_get_write_queue_size(&sock->uv_handle.stream); - if (write_queue_size < ISC_NETMGR_TCP_SENDBUF_SIZE) { - isc_log_write( - isc_lctx, ISC_LOGCATEGORY_GENERAL, - ISC_LOGMODULE_NETMGR, ISC_LOG_DEBUG(3), - "resuming TCP connection, the other side " - "is reading the data again (%zu)", - write_queue_size); - sock->reading_throttled = false; - isc__nm_start_reading(sock); + isc_result_t result = isc__nm_process_sock_buffer(sock); + if (result != ISC_R_SUCCESS) { + atomic_store(&sock->reading, true); + isc__nm_failed_read_cb(sock, result, false); } } } diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c index dbcd34b8ed..dcec05347f 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -1828,25 +1828,10 @@ tlsdns_maybe_restart_reading(isc_nmsocket_t *sock) { if (!sock->client && sock->reading_throttled && !uv_is_active(&sock->uv_handle.handle)) { - /* - * Restart reading if we have less data in the send queue than - * the send buffer size, this means that the TCP client has - * started reading some data again. Starting reading when we go - * under the limit instead of waiting for all data has been - * flushed allows faster recovery (in case there was a - * congestion and now there isn't). - */ - size_t write_queue_size = - uv_stream_get_write_queue_size(&sock->uv_handle.stream); - if (write_queue_size < ISC_NETMGR_TCP_SENDBUF_SIZE) { - isc_log_write( - isc_lctx, ISC_LOGCATEGORY_GENERAL, - ISC_LOGMODULE_NETMGR, ISC_LOG_DEBUG(3), - "resuming TCP connection, the other side " - "is reading the data again (%zu)", - write_queue_size); - sock->reading_throttled = false; - isc__nm_start_reading(sock); + isc_result_t result = isc__nm_process_sock_buffer(sock); + if (result != ISC_R_SUCCESS) { + atomic_store(&sock->reading, true); + isc__nm_failed_read_cb(sock, result, false); } } }