From: Ondřej Surý <ondrej@isc.org>
Date: Mon, 29 Mar 2021 08:52:05 +0000 (+0200)
Subject: Make it possible to recover from read timeouts
X-Git-Tag: v9.17.12~10^2~12
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=33c00c281f6279bc436b3fbff4a5069c483eb315;p=thirdparty%2Fbind9.git

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 f625cf8aaf1..4525add9abc 100644
--- 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 a0b2892555f..28da9159b66 100644
--- 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 ad80a4b76f5..b5d97d2ab80 100644
--- 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);
 }