From: =?utf-8?b?VmxhZGltw61yIMSMdW7DoXQ=?= <vladimir.cunat@nic.cz>
Date: Sat, 29 Jul 2023 17:53:34 +0200
Subject: daemon: more avoidance of excessive TCP reconnections

Previously this penalization was only triggered if the remote server
closed TCP.  Now it's extended to us closing it when the server
(only) sends back some nonsense.  At least for the cases which I could
see immediately.

That's just three trivial one-line additions; the rest is refactoring.
---
 daemon/io.c      | 38 ++++++++++----------------------------
 daemon/session.c | 26 +++++++++++++++++++++-----
 daemon/session.h |  5 +++--
 3 files changed, 34 insertions(+), 35 deletions(-)

diff --git a/daemon/io.c b/daemon/io.c
index 48bfed3..ac0d969 100644
--- a/daemon/io.c
+++ b/daemon/io.c
@@ -337,33 +337,6 @@ void tcp_timeout_trigger(uv_timer_t *timer)
 	}
 }
 
-static void tcp_disconnect(struct session *s, int errcode)
-{
-	if (kr_log_is_debug(IO, NULL)) {
-		struct sockaddr *peer = session_get_peer(s);
-		char *peer_str = kr_straddr(peer);
-		kr_log_debug(IO, "=> connection to '%s' closed by peer (%s)\n",
-			       peer_str ? peer_str : "",
-			       uv_strerror(errcode));
-	}
-
-	if (!session_was_useful(s) && session_flags(s)->outgoing) {
-		/* We want to penalize the IP address, if a task is asking a query.
-		 * It might not be the right task, but that doesn't matter so much
-		 * for attributing the useless session to the IP address. */
-		struct qr_task *t = session_tasklist_get_first(s);
-		struct kr_query *qry = NULL;
-		if (t) {
-			struct kr_request *req = worker_task_request(t);
-			qry = array_tail(req->rplan.pending);
-		}
-		if (qry) /* We reuse the error for connection, as it's quite similar. */
-			qry->server_selection.error(qry, worker_task_get_transport(t),
-							KR_SELECTION_TCP_CONNECT_FAILED);
-	}
-	worker_end_tcp(s);
-}
-
 static void tcp_recv(uv_stream_t *handle, ssize_t nread, const uv_buf_t *buf)
 {
 	struct session *s = handle->data;
@@ -381,7 +354,16 @@ static void tcp_recv(uv_stream_t *handle, ssize_t nread, const uv_buf_t *buf)
 	}
 
 	if (nread < 0 || !buf->base) {
-		tcp_disconnect(s, nread);
+		if (kr_log_is_debug(IO, NULL)) {
+			struct sockaddr *peer = session_get_peer(s);
+			char *peer_str = kr_straddr(peer);
+			kr_log_debug(IO, "=> connection to '%s' closed by peer (%s)\n",
+				       peer_str ? peer_str : "",
+				       uv_strerror(nread));
+		}
+
+		session_tcp_penalize(s);
+		worker_end_tcp(s);
 		return;
 	}
 
diff --git a/daemon/session.c b/daemon/session.c
index a1f2207..ed0ff68 100644
--- a/daemon/session.c
+++ b/daemon/session.c
@@ -123,11 +123,6 @@ void session_close(struct session *session)
 	}
 }
 
-bool session_was_useful(const struct session *session)
-{
-	return session->was_useful;
-}
-
 int session_start_read(struct session *session)
 {
 	return io_start_read(session->handle);
@@ -582,6 +577,24 @@ ssize_t session_wirebuf_trim(struct session *session, ssize_t len)
 	return len;
 }
 
+void session_tcp_penalize(struct session *s)
+{
+	if (s->was_useful || !s->sflags.outgoing)
+		return;
+	/* We want to penalize the IP address, if a task is asking a query.
+	 * It might not be the right task, but that doesn't matter so much
+	 * for attributing the useless session to the IP address. */
+	struct qr_task *t = session_tasklist_get_first(s);
+	struct kr_query *qry = NULL;
+	if (t) {
+		struct kr_request *req = worker_task_request(t);
+		qry = array_tail(req->rplan.pending);
+	}
+	if (qry) /* We reuse the error for connection, as it's quite similar. */
+		qry->server_selection.error(qry, worker_task_get_transport(t),
+						KR_SELECTION_TCP_CONNECT_FAILED);
+}
+
 knot_pkt_t *session_produce_packet(struct session *session, knot_mm_t *mm)
 {
 	session->sflags.wirebuf_error = false;
@@ -617,6 +630,7 @@ knot_pkt_t *session_produce_packet(struct session *session, knot_mm_t *mm)
 		msg_size = knot_wire_read_u16(msg_start);
 		if (msg_size >= session->wire_buf_size) {
 			session->sflags.wirebuf_error = true;
+			session_tcp_penalize(session);
 			return NULL;
 		}
 		if (msg_size + 2 > wirebuf_msg_data_size) {
@@ -624,6 +638,7 @@ knot_pkt_t *session_produce_packet(struct session *session, knot_mm_t *mm)
 		}
 		if (msg_size == 0) {
 			session->sflags.wirebuf_error = true;
+			session_tcp_penalize(session);
 			return NULL;
 		}
 		msg_start += 2;
@@ -631,6 +646,7 @@ knot_pkt_t *session_produce_packet(struct session *session, knot_mm_t *mm)
 		msg_size = wirebuf_msg_data_size;
 	} else {
 		session->sflags.wirebuf_error = true;
+		session_tcp_penalize(session);
 		return NULL;
 	}
 
diff --git a/daemon/session.h b/daemon/session.h
index 603d7cb..1f95ac5 100644
--- a/daemon/session.h
+++ b/daemon/session.h
@@ -91,8 +91,9 @@ int session_tasklist_finalize_expired(struct session *session);
 /** Both of task lists (associated & waiting). */
 /** Check if empty. */
 bool session_is_empty(const struct session *session);
-/** Return whether session seems to have done something useful. */
-bool session_was_useful(const struct session *session);
+/** Penalize this server if the session hasn't been useful (and is outgoing). */
+void session_tcp_penalize(struct session *session);
+
 /** Get pointer to session flags */
 struct session_flags *session_flags(struct session *session);
 /** Get pointer to peer address. */