From 4a99004327a0611c68f07cd2be2d5248d7c82157 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tam=C3=A1s=20B=C3=A1lint=20Misius?= <lbphacker@gmail.com>
Date: Mon, 1 Jan 2024 17:08:11 +0100
Subject: [PATCH] Fix requests not being marked done when they fail early

They were marked done as far as Libcurl.cpp semantics went, but they weren't noticed by the Worker loop and thus weren't actually marked done with MarkDone.

Broken by 0cc179ae4ea2 where I quietly (i.e. no mention of this in the commit message >_>) replaced curl_multi_wait with curl_multi_poll, the former of which would return after some time even if nothing happened and the latter of which doesn't. Thus, the request manager loop kept iterating, if slowly, and masked this issue: these half-baked requests ended up being noticed in the iteration of the Worker loop that came after the one during which they were marked done.

Also fix the MotD being finalized improperly in Client.
---
 src/client/Client.cpp                      |  8 ++++----
 src/client/http/requestmanager/Libcurl.cpp | 20 +++++++++++++-------
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/src/client/Client.cpp b/src/client/Client.cpp
index 9679ca68c..b3589a1f0 100644
--- a/src/client/Client.cpp
+++ b/src/client/Client.cpp
@@ -131,7 +131,7 @@ void Client::Tick()
 			{
 				updateInfo = info.updateInfo;
 				applyUpdateInfo = true;
-				messageOfTheDay = info.messageOfTheDay;
+				SetMessageOfTheDay(info.messageOfTheDay);
 			}
 			for (auto &notification : info.notifications)
 			{
@@ -142,7 +142,7 @@ void Client::Tick()
 		{
 			if (!usingAltUpdateServer)
 			{
-				messageOfTheDay = ByteString::Build("Error while fetching MotD: ", ex.what()).FromUtf8();
+				SetMessageOfTheDay(ByteString::Build("Error while fetching MotD: ", ex.what()).FromUtf8());
 			}
 		}
 		versionCheckRequest.reset();
@@ -154,7 +154,7 @@ void Client::Tick()
 			auto info = alternateVersionCheckRequest->Finish();
 			updateInfo = info.updateInfo;
 			applyUpdateInfo = true;
-			messageOfTheDay = info.messageOfTheDay;
+			SetMessageOfTheDay(info.messageOfTheDay);
 			for (auto &notification : info.notifications)
 			{
 				AddServerNotification(notification);
@@ -162,7 +162,7 @@ void Client::Tick()
 		}
 		catch (const http::RequestError &ex)
 		{
-			messageOfTheDay = ByteString::Build("Error while checking for updates: ", ex.what()).FromUtf8();
+			SetMessageOfTheDay(ByteString::Build("Error while checking for updates: ", ex.what()).FromUtf8());
 		}
 		alternateVersionCheckRequest.reset();
 	}
diff --git a/src/client/http/requestmanager/Libcurl.cpp b/src/client/http/requestmanager/Libcurl.cpp
index 657b2085e..af8420fcb 100644
--- a/src/client/http/requestmanager/Libcurl.cpp
+++ b/src/client/http/requestmanager/Libcurl.cpp
@@ -296,13 +296,8 @@ namespace http
 		{
 			{
 				std::lock_guard lk(sharedStateMx);
-				for (auto &requestHandle : requestHandles)
-				{
-					if (requestHandle->statusCode)
-					{
-						requestHandlesToUnregister.push_back(requestHandle);
-					}
-				}
+				// Register new handles first. This always succeeds even if the handle is "failed early" so that
+				// a single MarkDone call could be issued on all handles further down in this block.
 				for (auto &requestHandle : requestHandlesToRegister)
 				{
 					// Must not be present
@@ -311,6 +306,17 @@ namespace http
 					RegisterRequestHandle(requestHandle);
 				}
 				requestHandlesToRegister.clear();
+				// Then unregister done handles. As explained above, registering a new handle may also immediately mark
+				// it done and we won't be coming back here until Wait() returns, so this has to come second.
+				for (auto &requestHandle : requestHandles)
+				{
+					if (requestHandle->statusCode)
+					{
+						requestHandlesToUnregister.push_back(requestHandle);
+					}
+				}
+				// Actually unregister handles queued to be unregistered. They can be queued just above, or from another thread.
+				// Thus, it's ok for them to be in the queue multiple times, but it's not ok to try to unregister them multiple times.
 				for (auto &requestHandle : requestHandlesToUnregister)
 				{
 					auto eraseFrom = std::remove(requestHandles.begin(), requestHandles.end(), requestHandle);