Do not un-throttle TCP connections on isc_nm_read()

Due to omission it was possible to un-throttle a TCP connection
previously throttled due to the peer not reading back data we are
sending.

In particular, that affected DoH code, but it could also affect other
transports (the current or future ones) that pause/resume reading
according to its internal state.

(cherry picked from commit d228aa8bbb944fbd04baf22d151fde5c33561e26)

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index 4ceb182e7bd0c47688083c89da52b6ef974145a1..75b221e33881a0fb06786dcb8992716bfac8491f 100644 (file)
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -1060,6 +1060,12 @@ struct isc_nmsocket {
         */
        uint64_t write_timeout;
 
+       /*
+        * Reading was throttled over TCP as the peer does not read the
+        * data we are sending back.
+        */
+       bool reading_throttled;
+
        /*% outer socket is for 'wrapped' sockets - e.g. tcpdns in tcp */
        isc_nmsocket_t *outer;
 

diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c
index 336cad4e231038647d461a13b442032103c07bb2..23af9cdf1c1e6ae10920e822d97a9b064bdf38f6 100644 (file)
--- a/lib/isc/netmgr/netmgr.c
+++ b/lib/isc/netmgr/netmgr.c
@@ -2096,7 +2096,6 @@ isc__nmsocket_readtimeout_cb(uv_timer_t *timer) {
 
        REQUIRE(VALID_NMSOCK(sock));
        REQUIRE(sock->tid == isc_nm_tid());
-       REQUIRE(atomic_load(&sock->reading));
 
        if (atomic_load(&sock->client)) {
                uv_timer_stop(timer);

diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c
index 016a9e9059a4a942bb86aeed7bd33cf61cb4b571..37d44bd9c846a3334eed1eba1b483f429f73e028 100644 (file)
--- a/lib/isc/netmgr/tcp.c
+++ b/lib/isc/netmgr/tcp.c
@@ -766,7 +766,7 @@ isc__nm_async_tcpstartread(isc__networker_t *worker, isc__netievent_t *ev0) {
        isc__netievent_tcpstartread_t *ievent =
                (isc__netievent_tcpstartread_t *)ev0;
        isc_nmsocket_t *sock = ievent->sock;
-       isc_result_t result;
+       isc_result_t result = ISC_R_SUCCESS;
 
        REQUIRE(VALID_NMSOCK(sock));
        REQUIRE(sock->tid == isc_nm_tid());
@@ -774,7 +774,7 @@ isc__nm_async_tcpstartread(isc__networker_t *worker, isc__netievent_t *ev0) {
 
        if (isc__nmsocket_closing(sock)) {
                result = ISC_R_CANCELED;
-       } else {
+       } else if (!sock->reading_throttled) {
                result = isc__nm_start_reading(sock);
        }
 
@@ -926,6 +926,7 @@ isc__nm_tcp_read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) {
                                              "the other side is "
                                              "not reading the data (%zu)",
                                              write_queue_size);
+                               sock->reading_throttled = true;
                                isc__nm_stop_reading(sock);
                        }
                }
@@ -1122,7 +1123,7 @@ isc__nm_tcp_send(isc_nmhandle_t *handle, const isc_region_t *region,
 
 static void
 tcp_maybe_restart_reading(isc_nmsocket_t *sock) {
-       if (!sock->client && sock->reading &&
+       if (!sock->client && sock->reading_throttled &&
            !uv_is_active(&sock->uv_handle.handle))
        {
                /*
@@ -1142,6 +1143,7 @@ tcp_maybe_restart_reading(isc_nmsocket_t *sock) {
                                "resuming TCP connection, the other side  "
                                "is reading the data again (%zu)",
                                write_queue_size);
+                       sock->reading_throttled = false;
                        isc__nm_start_reading(sock);
                }
        }
@@ -1165,7 +1167,14 @@ tcp_send_cb(uv_write_t *req, int status) {
                isc__nm_failed_send_cb(sock, uvreq,
                                       isc__nm_uverr2result(status));
 
-               if (!sock->client && sock->reading) {
+               if (!sock->client &&
+                   (atomic_load(&sock->reading) || sock->reading_throttled))
+               {
+                       /*
+                        * As we are resuming reading, it is not throttled
+                        * anymore (technically).
+                        */
+                       sock->reading_throttled = false;
                        isc__nm_start_reading(sock);
                        isc__nmsocket_reset(sock);
                }

diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c
index 00ecb0f3d2b883b4ed4e5dd0e5c9c91949144615..d7ec755bec6860d1b6cfb8f62d6cc62889d16c4b 100644 (file)
--- a/lib/isc/netmgr/tcpdns.c
+++ b/lib/isc/netmgr/tcpdns.c
@@ -734,7 +734,7 @@ isc__nm_async_tcpdnsread(isc__networker_t *worker, isc__netievent_t *ev0) {
        isc__netievent_tcpdnsread_t *ievent =
                (isc__netievent_tcpdnsread_t *)ev0;
        isc_nmsocket_t *sock = ievent->sock;
-       isc_result_t result;
+       isc_result_t result = ISC_R_SUCCESS;
 
        UNUSED(worker);
 
@@ -743,7 +743,7 @@ isc__nm_async_tcpdnsread(isc__networker_t *worker, isc__netievent_t *ev0) {
 
        if (isc__nmsocket_closing(sock)) {
                result = ISC_R_CANCELED;
-       } else {
+       } else if (!sock->reading_throttled) {
                result = isc__nm_process_sock_buffer(sock);
        }
 
@@ -932,6 +932,7 @@ isc__nm_tcpdns_read_cb(uv_stream_t *stream, ssize_t nread,
                                      "the other side is "
                                      "not reading the data (%zu)",
                                      write_queue_size);
+                       sock->reading_throttled = true;
                        isc__nm_stop_reading(sock);
                }
        }
@@ -1158,7 +1159,7 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
 
 static void
 tcpdns_maybe_restart_reading(isc_nmsocket_t *sock) {
-       if (!sock->client && sock->reading &&
+       if (!sock->client && sock->reading_throttled &&
            !uv_is_active(&sock->uv_handle.handle))
        {
                /*
@@ -1178,6 +1179,7 @@ tcpdns_maybe_restart_reading(isc_nmsocket_t *sock) {
                                "resuming TCP connection, the other side  "
                                "is reading the data again (%zu)",
                                write_queue_size);
+                       sock->reading_throttled = false;
                        isc__nm_start_reading(sock);
                }
        }
@@ -1201,7 +1203,14 @@ tcpdns_send_cb(uv_write_t *req, int status) {
                isc__nm_failed_send_cb(sock, uvreq,
                                       isc__nm_uverr2result(status));
 
-               if (!sock->client && sock->reading) {
+               if (!sock->client &&
+                   (atomic_load(&sock->reading) || sock->reading_throttled))
+               {
+                       /*
+                        * As we are resuming reading, it is not throttled
+                        * anymore (technically).
+                        */
+                       sock->reading_throttled = false;
                        isc__nm_start_reading(sock);
                        isc__nmsocket_reset(sock);
                }