From f2e7dbf218365e3f47b942ea999796b2724ccc24 Mon Sep 17 00:00:00 2001
From: "Steinar H. Gunderson" <sgunderson@bigfoot.com>
Date: Sun, 22 Apr 2018 16:34:00 +0200
Subject: [PATCH] Fix timeout behavior with persistent connections.

We'd either never time out clients (if the expiry check happened
while they were receiving a request) or time them out too soon
(based on the connection time instead of the time of last request end).
---
 client.cpp |  5 -----
 client.h   |  2 +-
 server.cpp | 30 ++++++++++++++++++++----------
 server.h   |  4 ++++
 4 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/client.cpp b/client.cpp
index 05b8e7d..23892ac 100644
--- a/client.cpp
+++ b/client.cpp
@@ -20,11 +20,6 @@ Client::Client(int sock)
 {
 	request.reserve(1024);
 
-	if (clock_gettime(CLOCK_MONOTONIC_COARSE, &connect_time) == -1) {
-		log_perror("clock_gettime(CLOCK_MONOTONIC_COARSE)");
-		return;
-	}
-
 	// Find the remote address, and convert it to ASCII.
 	sockaddr_in6 addr;
 	socklen_t addr_len = sizeof(addr);
diff --git a/client.h b/client.h
index c18df4a..858197f 100644
--- a/client.h
+++ b/client.h
@@ -42,7 +42,7 @@ struct Client {
 	// The file descriptor associated with this socket.
 	int sock;
 
-	// When the client connected, in terms of CLOCK_MONOTONIC_COARSE.
+	// When the client connected (or went into keepalive), in terms of CLOCK_MONOTONIC_COARSE.
 	timespec connect_time;
 
 	// Some information only used for logging.
diff --git a/server.cpp b/server.cpp
index ccac946..78273b0 100644
--- a/server.cpp
+++ b/server.cpp
@@ -210,16 +210,7 @@ void Server::add_client(int sock, Acceptor *acceptor)
 	assert(inserted.second == true);  // Should not already exist.
 	Client *client_ptr = &inserted.first->second;
 
-	// Connection timestamps must be nondecreasing. I can't find any guarantee
-	// that even the monotonic clock can't go backwards by a small amount
-	// (think switching between CPUs with non-synchronized TSCs), so if
-	// this actually should happen, we hack around it by fudging
-	// connect_time.
-	if (!clients_ordered_by_connect_time.empty() &&
-	    is_earlier(client_ptr->connect_time, clients_ordered_by_connect_time.back().first)) {
-		client_ptr->connect_time = clients_ordered_by_connect_time.back().first;
-	}
-	clients_ordered_by_connect_time.push(make_pair(client_ptr->connect_time, sock));
+	start_client_timeout_timer(client_ptr);
 
 	// Start listening on data from this socket.
 	epoll_event ev;
@@ -302,6 +293,24 @@ void Server::add_client_from_serialized(const ClientProto &client, const vector<
 	}
 }
 
+void Server::start_client_timeout_timer(Client *client)
+{
+	// Connection timestamps must be nondecreasing. I can't find any guarantee
+	// that even the monotonic clock can't go backwards by a small amount
+	// (think switching between CPUs with non-synchronized TSCs), so if
+	// this actually should happen, we hack around it by fudging
+	// connect_time.
+	if (clock_gettime(CLOCK_MONOTONIC_COARSE, &client->connect_time) == -1) {
+		log_perror("clock_gettime(CLOCK_MONOTONIC_COARSE)");
+	} else {
+		if (!clients_ordered_by_connect_time.empty() &&
+		    is_earlier(client->connect_time, clients_ordered_by_connect_time.back().first)) {
+			client->connect_time = clients_ordered_by_connect_time.back().first;
+		}
+		clients_ordered_by_connect_time.push(make_pair(client->connect_time, client->sock));
+	}
+}
+
 int Server::lookup_stream_by_url(const string &url) const
 {
 	const auto stream_url_it = stream_url_map.find(url);
@@ -1323,6 +1332,7 @@ bool Server::more_requests(Client *client)
 	client->header_or_short_response_holder.clear();
 	client->header_or_short_response_ref.reset();
 	client->header_or_short_response_bytes_sent = 0;
+	start_client_timeout_timer(client);
 
 	change_epoll_events(client, EPOLLIN | EPOLLET | EPOLLRDHUP);  // No TLS handshake, so no EPOLLOUT needed.
 
diff --git a/server.h b/server.h
index 4ec908b..cfb6b2e 100644
--- a/server.h
+++ b/server.h
@@ -201,6 +201,10 @@ private:
 	void skip_lost_data(Client *client);
 
 	void add_client(int sock, Acceptor *acceptor);
+
+	// Mark that a client just went into READING_REQUEST state, so we should
+	// note the current time of day and then put it into <clients_ordered_by_connect_time>.
+	void start_client_timeout_timer(Client *client);
 };
 
 #endif  // !defined(_SERVER_H)
-- 
2.39.2