From 9323e186164800d50b604f49d0b9a8edc9b5d0b4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Robert=20M=C3=BCller?= <robytemueller@gmail.com>
Date: Mon, 4 Mar 2024 23:29:01 +0100
Subject: [PATCH] Fix HTTP client shutdown and deadlock on request error

Fix HTTP client effectively shutting down by entering the `ERROR` state when a request cannot be added. Now only the invalid request is aborted immediately. The `ERROR` state is now only entered when a curl function fails in a way where recovery is not possible.

Fix occasional deadlock when HTTP client is shutting down after entering the `ERROR` state, by also immediately aborting new requests when the HTTP client is already in the `ERROR` state.

Remove unused `CHttp::EState::STOPPING`.
---
 src/engine/shared/http.cpp | 22 ++++++++++++++++------
 src/engine/shared/http.h   |  1 -
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/src/engine/shared/http.cpp b/src/engine/shared/http.cpp
index af0fb6a27..76bbec657 100644
--- a/src/engine/shared/http.cpp
+++ b/src/engine/shared/http.cpp
@@ -471,7 +471,7 @@ void CHttp::RunLoop()
 		if(PollCode != CURLM_OK)
 		{
 			Lock.lock();
-			log_error("http", "Failed multi wait: %s", curl_multi_strerror(PollCode));
+			log_error("http", "curl_multi_poll failed: %s", curl_multi_strerror(PollCode));
 			m_State = CHttp::ERROR;
 			break;
 		}
@@ -480,7 +480,7 @@ void CHttp::RunLoop()
 		if(PerformCode != CURLM_OK)
 		{
 			Lock.lock();
-			log_error("http", "Failed multi perform: %s", curl_multi_strerror(PerformCode));
+			log_error("http", "curl_multi_perform failed: %s", curl_multi_strerror(PerformCode));
 			m_State = CHttp::ERROR;
 			break;
 		}
@@ -514,23 +514,33 @@ void CHttp::RunLoop()
 
 			CURL *pEH = curl_easy_init();
 			if(!pEH)
+			{
+				log_error("http", "curl_easy_init failed");
 				goto error_init;
+			}
 
 			if(!pRequest->ConfigureHandle(pEH))
-				goto error_configure;
+			{
+				curl_easy_cleanup(pEH);
+				str_copy(pRequest->m_aErr, "Failed to initialize request");
+				pRequest->OnCompletionInternal(nullptr, CURLE_ABORTED_BY_CALLBACK);
+				NewRequests.pop_front();
+				continue;
+			}
 
 			if(curl_multi_add_handle(m_pMultiH, pEH) != CURLM_OK)
+			{
+				log_error("http", "curl_multi_add_handle failed");
 				goto error_configure;
+			}
 
 			m_RunningRequests.emplace(pEH, std::move(pRequest));
 			NewRequests.pop_front();
-
 			continue;
 
 		error_configure:
 			curl_easy_cleanup(pEH);
 		error_init:
-			log_error("http", "failed to start new request");
 			Lock.lock();
 			m_State = CHttp::ERROR;
 			break;
@@ -579,7 +589,7 @@ void CHttp::Run(std::shared_ptr<IHttpRequest> pRequest)
 {
 	std::shared_ptr<CHttpRequest> pRequestImpl = std::static_pointer_cast<CHttpRequest>(pRequest);
 	std::unique_lock Lock(m_Lock);
-	if(m_Shutdown)
+	if(m_Shutdown || m_State == CHttp::ERROR)
 	{
 		str_copy(pRequestImpl->m_aErr, "Shutting down");
 		pRequestImpl->OnCompletionInternal(nullptr, CURLE_ABORTED_BY_CALLBACK);
diff --git a/src/engine/shared/http.h b/src/engine/shared/http.h
index f8d874f3e..87b808be9 100644
--- a/src/engine/shared/http.h
+++ b/src/engine/shared/http.h
@@ -255,7 +255,6 @@ class CHttp : public IHttp
 	{
 		UNINITIALIZED,
 		RUNNING,
-		STOPPING,
 		ERROR,
 	};