Make it possible to recover from read timeouts

Previously, when the client timed out on read, the client socket would
be automatically closed and destroyed when the nmhandle was detached.
This commit changes the logic so that it's possible for the callback to
recover from the ISC_R_TIMEDOUT event by restarting the timer. This is
done by calling isc_nmhandle_settimeout(), which prevents the timeout
handling code from destroying the socket; instead, it continues to wait
for data.

One specific use case for multiple timeouts is serve-stale - the client
socket could be created with shorter timeout (as specified with
stale-answer-client-timeout), so we can serve the requestor with stale
answer, but keep the original query running for a longer time.

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index f625cf8aaf1fd4741458f5632ed151d228d15e71..4525add9abc221d64db0040b5a5534e9e0494ad5 100644 (file)
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -854,8 +854,6 @@ struct isc_nmsocket {
         * TCP read/connect timeout timers.
         */
        uv_timer_t timer;
-       bool timer_initialized;
-       bool timer_running;
        uint64_t read_timeout;
        uint64_t connect_timeout;
 
@@ -1167,8 +1165,10 @@ void
 isc__nmsocket_timer_start(isc_nmsocket_t *sock);
 void
 isc__nmsocket_timer_restart(isc_nmsocket_t *sock);
+bool
+isc__nmsocket_timer_running(isc_nmsocket_t *sock);
 /*%<
- * Start/stop/restart the read timeout on the socket
+ * Start/stop/restart/check the timeout on the socket
  */
 
 void

diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c
index a0b2892555fe77d021c3e838937adb3a37427fcb..28da9159b66aff61e97190b0ea8ee7848c22d69f 100644 (file)
--- a/lib/isc/netmgr/netmgr.c
+++ b/lib/isc/netmgr/netmgr.c
@@ -987,15 +987,6 @@ nmsocket_cleanup(isc_nmsocket_t *sock, bool dofree FLARG) {
 
        sock->pquota = NULL;
 
-       if (sock->timer_initialized) {
-               sock->timer_initialized = false;
-               /* We might be in timer callback */
-               if (!uv_is_closing((uv_handle_t *)&sock->timer)) {
-                       uv_timer_stop(&sock->timer);
-                       uv_close((uv_handle_t *)&sock->timer, NULL);
-               }
-       }
-
        isc_astack_destroy(sock->inactivehandles);
 
        while ((uvreq = isc_astack_pop(sock->inactivereqs)) != NULL) {
@@ -1397,11 +1388,16 @@ isc___nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer,
 #endif
        UNLOCK(&sock->lock);
 
-       if (sock->type == isc_nm_tcpsocket || sock->type == isc_nm_tlssocket ||
-           (sock->type == isc_nm_udpsocket && atomic_load(&sock->client)) ||
-           (sock->type == isc_nm_tcpdnssocket && atomic_load(&sock->client)) ||
-           (sock->type == isc_nm_tlsdnssocket && atomic_load(&sock->client)))
-       {
+       switch (sock->type) {
+       case isc_nm_udpsocket:
+       case isc_nm_tcpdnssocket:
+       case isc_nm_tlsdnssocket:
+               if (!atomic_load(&sock->client)) {
+                       break;
+               }
+               /* fallthrough */
+       case isc_nm_tcpsocket:
+       case isc_nm_tlssocket:
                INSIST(sock->statichandle == NULL);
 
                /*
@@ -1411,6 +1407,9 @@ isc___nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer,
                 * handle and socket would never be freed.
                 */
                sock->statichandle = handle;
+               break;
+       default:
+               break;
        }
 
        if (sock->type == isc_nm_httpsocket && sock->h2.session) {
@@ -1704,7 +1703,19 @@ isc__nmsocket_readtimeout_cb(uv_timer_t *timer) {
        REQUIRE(sock->tid == isc_nm_tid());
        REQUIRE(sock->reading);
 
-       isc__nm_failed_read_cb(sock, ISC_R_TIMEDOUT);
+       if (atomic_load(&sock->client)) {
+               if (sock->recv_cb != NULL) {
+                       isc__nm_uvreq_t *req = isc__nm_get_read_req(sock, NULL);
+                       isc__nm_readcb(sock, req, ISC_R_TIMEDOUT);
+               }
+
+               if (!isc__nmsocket_timer_running(sock)) {
+                       isc__nmsocket_clearcb(sock);
+                       isc__nm_failed_read_cb(sock, ISC_R_CANCELED);
+               }
+       } else {
+               isc__nm_failed_read_cb(sock, ISC_R_TIMEDOUT);
+       }
 }
 
 void
@@ -1720,11 +1731,18 @@ isc__nmsocket_timer_restart(isc_nmsocket_t *sock) {
        RUNTIME_CHECK(r == 0);
 }
 
+bool
+isc__nmsocket_timer_running(isc_nmsocket_t *sock) {
+       REQUIRE(VALID_NMSOCK(sock));
+
+       return (uv_is_active((uv_handle_t *)&sock->timer));
+}
+
 void
 isc__nmsocket_timer_start(isc_nmsocket_t *sock) {
        REQUIRE(VALID_NMSOCK(sock));
 
-       if (uv_is_active((uv_handle_t *)&sock->timer)) {
+       if (isc__nmsocket_timer_running(sock)) {
                return;
        }
 
@@ -1735,9 +1753,7 @@ void
 isc__nmsocket_timer_stop(isc_nmsocket_t *sock) {
        REQUIRE(VALID_NMSOCK(sock));
 
-       if (!uv_is_active((uv_handle_t *)&sock->timer)) {
-               return;
-       }
+       /* uv_timer_stop() is idempotent, no need to check if running */
 
        int r = uv_timer_stop(&sock->timer);
        RUNTIME_CHECK(r == 0);
@@ -1751,13 +1767,21 @@ isc__nm_get_read_req(isc_nmsocket_t *sock, isc_sockaddr_t *sockaddr) {
        req->cb.recv = sock->recv_cb;
        req->cbarg = sock->recv_cbarg;
 
-       if (atomic_load(&sock->client)) {
+       switch (sock->type) {
+       case isc_nm_tcpsocket:
+       case isc_nm_tlssocket:
                isc_nmhandle_attach(sock->statichandle, &req->handle);
-       } else {
-               req->handle = isc__nmhandle_get(sock, sockaddr, NULL);
+               break;
+       default:
+               if (atomic_load(&sock->client)) {
+                       isc_nmhandle_attach(sock->statichandle, &req->handle);
+               } else {
+                       req->handle = isc__nmhandle_get(sock, sockaddr, NULL);
+               }
+               break;
        }
 
-       return req;
+       return (req);
 }
 
 /*%<
@@ -1987,9 +2011,7 @@ isc_nmhandle_settimeout(isc_nmhandle_t *handle, uint32_t timeout) {
                return;
        default:
                handle->sock->read_timeout = timeout;
-               if (uv_is_active((uv_handle_t *)&handle->sock->timer)) {
-                       isc__nmsocket_timer_restart(handle->sock);
-               }
+               isc__nmsocket_timer_restart(handle->sock);
        }
 }
 

diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c
index ad80a4b76f56615e5a45c1ddda35eaa157162fcd..b5d97d2ab80efb762c6739b673b5659d1d561b14 100644 (file)
--- a/lib/isc/netmgr/tcp.c
+++ b/lib/isc/netmgr/tcp.c
@@ -92,9 +92,6 @@ start_reading(isc_nmsocket_t *sock);
 static void
 stop_reading(isc_nmsocket_t *sock);
 
-static isc__nm_uvreq_t *
-get_read_req(isc_nmsocket_t *sock);
-
 static void
 tcp_alloc_cb(uv_handle_t *handle, size_t size, uv_buf_t *buf);
 
@@ -701,7 +698,7 @@ failed_read_cb(isc_nmsocket_t *sock, isc_result_t result) {
        sock->recv_read = false;
 
        if (sock->recv_cb != NULL) {
-               isc__nm_uvreq_t *req = get_read_req(sock);
+               isc__nm_uvreq_t *req = isc__nm_get_read_req(sock, NULL);
                isc__nmsocket_clearcb(sock);
                isc__nm_readcb(sock, req, result);
        }
@@ -734,18 +731,6 @@ failed_send_cb(isc_nmsocket_t *sock, isc__nm_uvreq_t *req,
        }
 }
 
-static isc__nm_uvreq_t *
-get_read_req(isc_nmsocket_t *sock) {
-       isc__nm_uvreq_t *req = NULL;
-
-       req = isc__nm_uvreq_get(sock->mgr, sock);
-       req->cb.recv = sock->recv_cb;
-       req->cbarg = sock->recv_cbarg;
-       isc_nmhandle_attach(sock->statichandle, &req->handle);
-
-       return req;
-}
-
 static void
 start_reading(isc_nmsocket_t *sock) {
        if (sock->reading) {
@@ -947,7 +932,7 @@ read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) {
                goto free;
        }
 
-       req = get_read_req(sock);
+       req = isc__nm_get_read_req(sock, NULL);
 
        /*
         * The callback will be called synchronously because the
@@ -1459,5 +1444,5 @@ isc__nm_tcp_listener_nactive(isc_nmsocket_t *listener) {
 
        nactive = atomic_load(&listener->active_child_connections);
        INSIST(nactive >= 0);
-       return nactive;
+       return (nactive);
 }