Merge pull request #62309 from reduz/remake-resource-thread-safety

Remake ResourceCache thread safety code and API

4 files changed, 80 insertions, 81 deletions

diff --git a/core/io/resource.cpp b/core/io/resource.cpp
index ed7228d0b9..fec5ca5c7b 100644
--- a/core/io/resource.cpp
+++ b/core/io/resource.cpp
@@ -52,41 +52,36 @@ void Resource::set_path(const String &p_path, bool p_take_over) {
 		return;
 	}
 
+	if (p_path.is_empty()) {
+		p_take_over = false; // Can't take over an empty path
+	}
+
+	ResourceCache::lock.lock();
+
 	if (!path_cache.is_empty()) {
-		ResourceCache::lock.write_lock();
 		ResourceCache::resources.erase(path_cache);
-		ResourceCache::lock.write_unlock();
 	}
 
 	path_cache = "";
 
-	ResourceCache::lock.read_lock();
-	bool has_path = ResourceCache::resources.has(p_path);
-	ResourceCache::lock.read_unlock();
+	Ref<Resource> existing = ResourceCache::get_ref(p_path);
 
-	if (has_path) {
+	if (existing.is_valid()) {
 		if (p_take_over) {
-			ResourceCache::lock.write_lock();
-			Resource **res = ResourceCache::resources.getptr(p_path);
-			if (res) {
-				(*res)->set_name("");
-			}
-			ResourceCache::lock.write_unlock();
+			existing->path_cache = String();
+			ResourceCache::resources.erase(p_path);
 		} else {
-			ResourceCache::lock.read_lock();
-			bool exists = ResourceCache::resources.has(p_path);
-			ResourceCache::lock.read_unlock();
-
-			ERR_FAIL_COND_MSG(exists, "Another resource is loaded from path '" + p_path + "' (possible cyclic resource inclusion).");
+			ResourceCache::lock.unlock();
+			ERR_FAIL_MSG("Another resource is loaded from path '" + p_path + "' (possible cyclic resource inclusion).");
 		}
 	}
+
 	path_cache = p_path;
 
 	if (!path_cache.is_empty()) {
-		ResourceCache::lock.write_lock();
 		ResourceCache::resources[path_cache] = this;
-		ResourceCache::lock.write_unlock();
 	}
+	ResourceCache::lock.unlock();
 
 	_resource_path_changed();
 }
@@ -380,7 +375,7 @@ void Resource::set_as_translation_remapped(bool p_remapped) {
 		return;
 	}
 
-	ResourceCache::lock.write_lock();
+	ResourceCache::lock.lock();
 
 	if (p_remapped) {
 		ResourceLoader::remapped_list.add(&remapped_list);
@@ -388,7 +383,7 @@ void Resource::set_as_translation_remapped(bool p_remapped) {
 		ResourceLoader::remapped_list.remove(&remapped_list);
 	}
 
-	ResourceCache::lock.write_unlock();
+	ResourceCache::lock.unlock();
 }
 
 bool Resource::is_translation_remapped() const {
@@ -455,9 +450,9 @@ Resource::Resource() :
 
 Resource::~Resource() {
 	if (!path_cache.is_empty()) {
-		ResourceCache::lock.write_lock();
+		ResourceCache::lock.lock();
 		ResourceCache::resources.erase(path_cache);
-		ResourceCache::lock.write_unlock();
+		ResourceCache::lock.unlock();
 	}
 	if (owners.size()) {
 		WARN_PRINT("Resource is still owned.");
@@ -469,7 +464,7 @@ HashMap<String, Resource *> ResourceCache::resources;
 HashMap<String, HashMap<String, String>> ResourceCache::resource_path_cache;
 #endif
 
-RWLock ResourceCache::lock;
+Mutex ResourceCache::lock;
 #ifdef TOOLS_ENABLED
 RWLock ResourceCache::path_cache_lock;
 #endif
@@ -491,46 +486,67 @@ void ResourceCache::reload_externals() {
 }
 
 bool ResourceCache::has(const String &p_path) {
-	lock.read_lock();
-	bool b = resources.has(p_path);
-	lock.read_unlock();
+	lock.lock();
+
+	Resource **res = resources.getptr(p_path);
 
-	return b;
+	if (res && (*res)->reference_get_count() == 0) {
+		// This resource is in the process of being deleted, ignore its existence.
+		(*res)->path_cache = String();
+		resources.erase(p_path);
+		res = nullptr;
+	}
+
+	lock.unlock();
+
+	if (!res) {
+		return false;
+	}
+
+	return true;
 }
 
-Resource *ResourceCache::get(const String &p_path) {
-	lock.read_lock();
+Ref<Resource> ResourceCache::get_ref(const String &p_path) {
+	Ref<Resource> ref;
+	lock.lock();
 
 	Resource **res = resources.getptr(p_path);
 
-	lock.read_unlock();
+	if (res) {
+		ref = Ref<Resource>(*res);
+	}
 
-	if (!res) {
-		return nullptr;
+	if (res && !ref.is_valid()) {
+		// This resource is in the process of being deleted, ignore its existence
+		(*res)->path_cache = String();
+		resources.erase(p_path);
+		res = nullptr;
 	}
 
-	return *res;
+	lock.unlock();
+
+	return ref;
 }
 
 void ResourceCache::get_cached_resources(List<Ref<Resource>> *p_resources) {
-	lock.read_lock();
+	lock.lock();
 	for (KeyValue<String, Resource *> &E : resources) {
 		p_resources->push_back(Ref<Resource>(E.value));
 	}
-	lock.read_unlock();
+	lock.unlock();
 }
 
 int ResourceCache::get_cached_resource_count() {
-	lock.read_lock();
+	lock.lock();
 	int rc = resources.size();
-	lock.read_unlock();
+	lock.unlock();
 
 	return rc;
 }
 
 void ResourceCache::dump(const char *p_file, bool p_short) {
 #ifdef DEBUG_ENABLED
-	lock.read_lock();
+	lock.lock();
 
 	HashMap<String, int> type_count;
 
@@ -562,7 +578,7 @@ void ResourceCache::dump(const char *p_file, bool p_short) {
 		}
 	}
 
-	lock.read_unlock();
+	lock.unlock();
 #else
 	WARN_PRINT("ResourceCache::dump only with in debug builds.");
 #endif
diff --git a/core/io/resource.h b/core/io/resource.h
index a45bc6e1e4..a2cde87990 100644
--- a/core/io/resource.h
+++ b/core/io/resource.h
@@ -153,7 +153,7 @@ public:
 class ResourceCache {
 	friend class Resource;
 	friend class ResourceLoader; //need the lock
-	static RWLock lock;
+	static Mutex lock;
 	static HashMap<String, Resource *> resources;
 #ifdef TOOLS_ENABLED
 	static HashMap<String, HashMap<String, String>> resource_path_cache; // Each tscn has a set of resource paths and IDs.
@@ -166,7 +166,7 @@ class ResourceCache {
 public:
 	static void reload_externals();
 	static bool has(const String &p_path);
-	static Resource *get(const String &p_path);
+	static Ref<Resource> get_ref(const String &p_path);
 	static void dump(const char *p_file = nullptr, bool p_short = false);
 	static void get_cached_resources(List<Ref<Resource>> *p_resources);
 	static int get_cached_resource_count();
diff --git a/core/io/resource_format_binary.cpp b/core/io/resource_format_binary.cpp
index 2469e1a4be..b1c50e829c 100644
--- a/core/io/resource_format_binary.cpp
+++ b/core/io/resource_format_binary.cpp
@@ -693,7 +693,7 @@ Error ResourceLoaderBinary::load() {
 			}
 
 			if (cache_mode == ResourceFormatLoader::CACHE_MODE_REUSE && ResourceCache::has(path)) {
-				Ref<Resource> cached = ResourceCache::get(path);
+				Ref<Resource> cached = ResourceCache::get_ref(path);
 				if (cached.is_valid()) {
 					//already loaded, don't do anything
 					error = OK;
@@ -717,10 +717,10 @@ Error ResourceLoaderBinary::load() {
 
 		if (cache_mode == ResourceFormatLoader::CACHE_MODE_REPLACE && ResourceCache::has(path)) {
 			//use the existing one
-			Resource *r = ResourceCache::get(path);
-			if (r->get_class() == t) {
-				r->reset_state();
-				res = Ref<Resource>(r);
+			Ref<Resource> cached = ResourceCache::get_ref(path);
+			if (cached->get_class() == t) {
+				cached->reset_state();
+				res = cached;
 			}
 		}
 
diff --git a/core/io/resource_loader.cpp b/core/io/resource_loader.cpp
index fb21db1a19..2cd455475c 100644
--- a/core/io/resource_loader.cpp
+++ b/core/io/resource_loader.cpp
@@ -335,23 +335,15 @@ Error ResourceLoader::load_threaded_request(const String &p_path, const String &
 				thread_load_mutex->unlock();
 				ERR_FAIL_V_MSG(ERR_INVALID_PARAMETER, "Attempted to load a resource already being loaded from this thread, cyclic reference?");
 			}
-			//lock first if possible
-			ResourceCache::lock.read_lock();
-
-			//get ptr
-			Resource **rptr = ResourceCache::resources.getptr(local_path);
-
-			if (rptr) {
-				Ref<Resource> res(*rptr);
-				//it is possible this resource was just freed in a thread. If so, this referencing will not work and resource is considered not cached
-				if (res.is_valid()) {
-					//referencing is fine
-					load_task.resource = res;
-					load_task.status = THREAD_LOAD_LOADED;
-					load_task.progress = 1.0;
-				}
+
+			Ref<Resource> existing = ResourceCache::get_ref(local_path);
+
+			if (existing.is_valid()) {
+				//referencing is fine
+				load_task.resource = existing;
+				load_task.status = THREAD_LOAD_LOADED;
+				load_task.progress = 1.0;
 			}
-			ResourceCache::lock.read_unlock();
 		}
 
 		if (!p_source_resource.is_empty()) {
@@ -530,27 +522,18 @@ Ref<Resource> ResourceLoader::load(const String &p_path, const String &p_type_hi
 		}
 
 		//Is it cached?
-		ResourceCache::lock.read_lock();
-
-		Resource **rptr = ResourceCache::resources.getptr(local_path);
 
-		if (rptr) {
-			Ref<Resource> res(*rptr);
+		Ref<Resource> existing = ResourceCache::get_ref(local_path);
 
-			//it is possible this resource was just freed in a thread. If so, this referencing will not work and resource is considered not cached
-			if (res.is_valid()) {
-				ResourceCache::lock.read_unlock();
-				thread_load_mutex->unlock();
-
-				if (r_error) {
-					*r_error = OK;
-				}
+		if (existing.is_valid()) {
+			thread_load_mutex->unlock();
 
-				return res; //use cached
+			if (r_error) {
+				*r_error = OK;
 			}
-		}
 
-		ResourceCache::lock.read_unlock();
+			return existing; //use cached
+		}
 
 		//load using task (but this thread)
 		ThreadLoadTask load_task;
@@ -867,7 +850,7 @@ String ResourceLoader::path_remap(const String &p_path) {
 }
 
 void ResourceLoader::reload_translation_remaps() {
-	ResourceCache::lock.read_lock();
+	ResourceCache::lock.lock();
 
 	List<Resource *> to_reload;
 	SelfList<Resource> *E = remapped_list.first();
@@ -877,7 +860,7 @@ void ResourceLoader::reload_translation_remaps() {
 		E = E->next();
 	}
 
-	ResourceCache::lock.read_unlock();
+	ResourceCache::lock.unlock();
 
 	//now just make sure to not delete any of these resources while changing locale..
 	while (to_reload.front()) {