From 0d5e9325a06a14ef8cde5b1e7440e2eeb46aab18 Mon Sep 17 00:00:00 2001
From: Craig Edwards <craigedwards@brainbox.cc>
Date: Wed, 4 Dec 2024 12:50:20 +0000
Subject: [PATCH] fix: https request queues still need a removals queue, it can
 be simplified but needs to still exist, there is a chicken-and-egg situation
 with the pointer for the request

---
 include/dpp/queues.h  | 10 ++++++++++
 src/dpp/queues.cpp    | 42 ++++++++++++++++++++++++++++++------------
 src/dpp/sslclient.cpp | 16 ++++++++++------
 3 files changed, 50 insertions(+), 18 deletions(-)

diff --git a/include/dpp/queues.h b/include/dpp/queues.h
index f54cb9c7a9..a58754e08c 100644
--- a/include/dpp/queues.h
+++ b/include/dpp/queues.h
@@ -383,6 +383,11 @@ class DPP_EXPORT http_request {
 
 	/** @brief Returns true if the request is complete */
 	bool is_completed();
+
+	/**
+	 * @brief Get the HTTPS client used to perform this request, or nullptr if there is none
+	 */
+	https_client* get_client() const;
 };
 
 /**
@@ -470,6 +475,11 @@ class DPP_EXPORT request_concurrency_queue {
 	 */
 	std::vector<std::unique_ptr<http_request>> requests_in;
 
+	/**
+	 * @brief Requests to remove after a set amount of time has passed
+	 */
+	std::vector<std::unique_ptr<http_request>> removals;
+
 	/**
 	 * @brief Timer callback
 	 * @param index Index ID for this timer
diff --git a/src/dpp/queues.cpp b/src/dpp/queues.cpp
index 51964e5787..a92e230d4f 100644
--- a/src/dpp/queues.cpp
+++ b/src/dpp/queues.cpp
@@ -107,11 +107,10 @@ http_request::http_request(const std::string &_url, http_completion_event comple
 {
 }
 
-http_request::~http_request() = default;
+http_request::~http_request()  = default;
 
 void http_request::complete(const http_request_completion_t &c) {
-	/* Call completion handler only if the request has been completed */
-	if (is_completed() && complete_handler) {
+	if (complete_handler) {
 		complete_handler(c);
 	}
 }
@@ -154,6 +153,11 @@ bool http_request::is_completed()
 	return completed;
 }
 
+https_client* http_request::get_client() const
+{
+	return cli.get();
+}
+
 /* Execute a HTTP request */
 http_request_completion_t http_request::run(request_concurrency_queue* processor, cluster* owner) {
 
@@ -241,7 +245,6 @@ http_request_completion_t http_request::run(request_concurrency_queue* processor
 				}
 				populate_result(_url, owner, result, *client);
 				/* Set completion flag */
-				completed = true;
 
 				bucket_t newbucket;
 				newbucket.limit = result.ratelimit_limit;
@@ -257,13 +260,17 @@ http_request_completion_t http_request::run(request_concurrency_queue* processor
 				processor->buckets[this->endpoint] = newbucket;
 
 				/* Transfer it to completed requests */
-				owner->queue_work(0, [this, result]() {
-					/* Manually release this unique_ptr now, to keep memory consumption and file descriptor consumption low.
-					 * Note we do this BEFORE we call complete(), becasue if user code throws an exception we need to be sure
-					 * we freed this first to avoid dangling pointer leaks.
-					 */
-					this->cli.reset();
-					complete(result);
+				owner->queue_work(0, [owner, this, result, hci, _url]() {
+					try {
+						complete(result);
+					}
+					catch (const std::exception& e) {
+						owner->log(ll_error, "Uncaught exception thrown in HTTPS callback for " + std::string(request_verb[method]) + " "  + hci.hostname + ":" + std::to_string(hci.port) + _url + ": " + std::string(e.what()));
+					}
+					catch (...) {
+						owner->log(ll_error, "Uncaught exception thrown in HTTPS callback for " + std::string(request_verb[method]) + " "  + hci.hostname + ":" + std::to_string(hci.port) + _url + ": <non exception value>");
+					}
+					completed = true;
 				});
 			}
 		);
@@ -296,6 +303,17 @@ request_concurrency_queue::request_concurrency_queue(class cluster* owner, class
 {
 	in_timer = creator->start_timer([this](auto timer_handle) {
 		tick_and_deliver_requests(in_index);
+		/* Clear pending removals in the removals queue */
+		if (time(nullptr) % 90 == 0) {
+			std::scoped_lock lock1{in_mutex};
+			for (auto it = removals.cbegin(); it != removals.cend();) {
+				if ((*it)->is_completed()) {
+					it = removals.erase(it);
+				} else {
+					++it;
+				}
+			}
+		}
 	}, 1);
 }
 
@@ -385,7 +403,7 @@ void request_concurrency_queue::tick_and_deliver_requests(uint32_t index)
 				for (auto it = begin; it != end; ++it) {
 					if (it->get() == request_view) {
 						/* Grab and remove */
-						request_view->stash_self(std::move(*it));
+						removals.emplace_back(std::move(*it));
 						requests_in.erase(it);
 						break;
 					}
diff --git a/src/dpp/sslclient.cpp b/src/dpp/sslclient.cpp
index 0fb6af11b5..f9bb713334 100644
--- a/src/dpp/sslclient.cpp
+++ b/src/dpp/sslclient.cpp
@@ -548,18 +548,22 @@ bool ssl_client::handle_buffer(std::string &buffer)
 
 void ssl_client::close()
 {
-	if (!plaintext && ssl->ssl) {
+	if (!plaintext) {
 		std::lock_guard<std::mutex> lock(ssl_mutex);
-		SSL_free(ssl->ssl);
-		ssl->ssl = nullptr;
+		if (ssl != nullptr && ssl->ssl != nullptr) {
+			SSL_free(ssl->ssl);
+			ssl->ssl = nullptr;
+		}
 	}
 	connected = tcp_connect_done = false;
 	client_to_server_length = client_to_server_offset = 0;
 	last_tick = time(nullptr);
 	bytes_in = bytes_out = 0;
-	owner->socketengine->delete_socket(sfd);
-	close_socket(sfd);
-	sfd = INVALID_SOCKET;
+	if (sfd != INVALID_SOCKET) {
+		owner->socketengine->delete_socket(sfd);
+		close_socket(sfd);
+		sfd = INVALID_SOCKET;
+	}
 	obuffer.clear();
 	buffer.clear();
 }