From aa3ef20fcbeb268f411da1969d3f80cebf92de00 Mon Sep 17 00:00:00 2001
From: Fabio Alessandrelli <fabio.alessandrelli@gmail.com>
Date: Sat, 29 Jan 2022 01:33:29 +0100
Subject: [PATCH] [Net] Simplify IP resolution code, fix caching.

First, we should not insert into cache if the hostname resolution has
failed (as it might be a temporary internet issue), second, the async
resolver should also properly insert into cache.

Took the chance to remove some duplicate code with critical section in
it at the cost of little performance when calling the blocking
resolve_hostname function.

(cherry picked from commit 49297d937ce6128b2c9f1fb09199dd7d4f2404b7)
---
 core/io/ip.cpp           | 42 ++++++++++++----------------------------
 drivers/unix/ip_unix.cpp |  2 +-
 2 files changed, 13 insertions(+), 31 deletions(-)

diff --git a/core/io/ip.cpp b/core/io/ip.cpp
index 51ffcc723f2..2ee0268b63a 100644
--- a/core/io/ip.cpp
+++ b/core/io/ip.cpp
@@ -96,6 +96,11 @@ struct _IP_ResolverPrivate {
 			if (queue[i].status.get() != IP::RESOLVER_STATUS_WAITING) {
 				continue;
 			}
+			// We might be overriding another result, but we don't care as long as the result is valid.
+			if (response.size()) {
+				String key = get_cache_key(hostname, type);
+				cache[key] = response;
+			}
 			queue[i].response = response;
 			queue[i].status.set(response.empty() ? IP::RESOLVER_STATUS_ERROR : IP::RESOLVER_STATUS_DONE);
 		}
@@ -118,30 +123,8 @@ struct _IP_ResolverPrivate {
 };
 
 IP_Address IP::resolve_hostname(const String &p_hostname, IP::Type p_type) {
-	List<IP_Address> res;
-	String key = _IP_ResolverPrivate::get_cache_key(p_hostname, p_type);
-
-	resolver->mutex.lock();
-	if (resolver->cache.has(key)) {
-		res = resolver->cache[key];
-	} else {
-		// This should be run unlocked so the resolver thread can keep
-		// resolving other requests.
-		resolver->mutex.unlock();
-		_resolve_hostname(res, p_hostname, p_type);
-		resolver->mutex.lock();
-		// We might be overriding another result, but we don't care (they are the
-		// same hostname).
-		resolver->cache[key] = res;
-	}
-	resolver->mutex.unlock();
-
-	for (int i = 0; i < res.size(); ++i) {
-		if (res[i].is_valid()) {
-			return res[i];
-		}
-	}
-	return IP_Address();
+	const Array addresses = resolve_hostname_addresses(p_hostname, p_type);
+	return addresses.size() ? addresses[0].operator IP_Address() : IP_Address();
 }
 
 Array IP::resolve_hostname_addresses(const String &p_hostname, Type p_type) {
@@ -157,17 +140,16 @@ Array IP::resolve_hostname_addresses(const String &p_hostname, Type p_type) {
 		resolver->mutex.unlock();
 		_resolve_hostname(res, p_hostname, p_type);
 		resolver->mutex.lock();
-		// We might be overriding another result, but we don't care (they are the
-		// same hostname).
-		resolver->cache[key] = res;
+		// We might be overriding another result, but we don't care as long as the result is valid.
+		if (res.size()) {
+			resolver->cache[key] = res;
+		}
 	}
 	resolver->mutex.unlock();
 
 	Array result;
 	for (int i = 0; i < res.size(); ++i) {
-		if (res[i].is_valid()) {
-			result.push_back(String(res[i]));
-		}
+		result.push_back(String(res[i]));
 	}
 	return result;
 }
diff --git a/drivers/unix/ip_unix.cpp b/drivers/unix/ip_unix.cpp
index de8afd9c8e0..f37ebd2df7e 100644
--- a/drivers/unix/ip_unix.cpp
+++ b/drivers/unix/ip_unix.cpp
@@ -115,7 +115,7 @@ void IP_Unix::_resolve_hostname(List<IP_Address> &r_addresses, const String &p_h
 			continue;
 		}
 		IP_Address ip = _sockaddr2ip(next->ai_addr);
-		if (!r_addresses.find(ip)) {
+		if (ip.is_valid() && !r_addresses.find(ip)) {
 			r_addresses.push_back(ip);
 		}
 		next = next->ai_next;