Remake resource thread safety and API

* Ensures thread safety when resources are destroyed.
* Simplified API by always forcing `ResourceCache::get_ref`, which needs less hacks and is fully thread safe.
* Removed RWLock for resources because its not possible to use for the new logic. Should not be a problem.

Supersedes #57533

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

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();

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;
 			}
 		}
 

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()) {

editor/dependency_editor.cpp

@@ -487,7 +487,7 @@ void DependencyRemoveDialog::show(const Vector<String> &p_folders, const Vector<
 void DependencyRemoveDialog::ok_pressed() {
 	for (int i = 0; i < files_to_delete.size(); ++i) {
 		if (ResourceCache::has(files_to_delete[i])) {
-			Resource *res = ResourceCache::get(files_to_delete[i]);
+			Ref<Resource> res = ResourceCache::get_ref(files_to_delete[i]);
 			res->set_path("");
 		}
 

editor/editor_file_system.cpp

@@ -1792,9 +1792,9 @@ Error EditorFileSystem::_reimport_group(const String &p_group_file, const Vector
 
 		//if file is currently up, maybe the source it was loaded from changed, so import math must be updated for it
 		//to reload properly
-		if (ResourceCache::has(file)) {
-			Resource *r = ResourceCache::get(file);
+		Ref<Resource> r = ResourceCache::get_ref(file);
 
+		if (r.is_valid()) {
 			if (!r->get_import_path().is_empty()) {
 				String dst_path = ResourceFormatImporter::get_singleton()->get_internal_resource_path(file);
 				r->set_import_path(dst_path);
@@ -2034,9 +2034,8 @@ void EditorFileSystem::_reimport_file(const String &p_file, const HashMap<String
 
 	//if file is currently up, maybe the source it was loaded from changed, so import math must be updated for it
 	//to reload properly
-	if (ResourceCache::has(p_file)) {
-		Resource *r = ResourceCache::get(p_file);
-
+	Ref<Resource> r = ResourceCache::get_ref(p_file);
+	if (r.is_valid()) {
 		if (!r->get_import_path().is_empty()) {
 			String dst_path = ResourceFormatImporter::get_singleton()->get_internal_resource_path(p_file);
 			r->set_import_path(dst_path);

editor/editor_folding.cpp

@@ -193,10 +193,7 @@ void EditorFolding::load_scene_folding(Node *p_scene, const String &p_path) {
 
 	for (int i = 0; i < res_unfolds.size(); i += 2) {
 		String path2 = res_unfolds[i];
-		Ref<Resource> res;
-		if (ResourceCache::has(path2)) {
-			res = Ref<Resource>(ResourceCache::get(path2));
-		}
+		Ref<Resource> res = ResourceCache::get_ref(path2);
 		if (res.is_null()) {
 			continue;
 		}

editor/editor_node.cpp

@@ -875,7 +875,7 @@ void EditorNode::_resources_changed(const Vector<String> &p_resources) {
 
 	int rc = p_resources.size();
 	for (int i = 0; i < rc; i++) {
-		Ref<Resource> res(ResourceCache::get(p_resources.get(i)));
+		Ref<Resource> res = ResourceCache::get_ref(p_resources.get(i));
 		if (res.is_null()) {
 			continue;
 		}
@@ -1005,8 +1005,8 @@ void EditorNode::_resources_reimported(const Vector<String> &p_resources) {
 			continue;
 		}
 		// Reload normally.
-		Resource *resource = ResourceCache::get(p_resources[i]);
-		if (resource) {
+		Ref<Resource> resource = ResourceCache::get_ref(p_resources[i]);
+		if (resource.is_valid()) {
 			resource->reload_from_file();
 		}
 	}
@@ -1719,7 +1719,7 @@ void EditorNode::_save_scene(String p_file, int idx) {
 		// We must update it, but also let the previous scene state go, as
 		// old version still work for referencing changes in instantiated or inherited scenes.
 
-		sdata = Ref<PackedScene>(Object::cast_to<PackedScene>(ResourceCache::get(p_file)));
+		sdata = ResourceCache::get_ref(p_file);
 		if (sdata.is_valid()) {
 			sdata->recreate_state();
 		} else {
@@ -3711,7 +3711,7 @@ Error EditorNode::load_scene(const String &p_scene, bool p_ignore_broken_deps, b
 
 	if (ResourceCache::has(lpath)) {
 		// Used from somewhere else? No problem! Update state and replace sdata.
-		Ref<PackedScene> ps = Ref<PackedScene>(Object::cast_to<PackedScene>(ResourceCache::get(lpath)));
+		Ref<PackedScene> ps = ResourceCache::get_ref(lpath);
 		if (ps.is_valid()) {
 			ps->replace_state(sdata->get_state());
 			ps->set_last_modified_time(sdata->get_last_modified_time());

editor/import/resource_importer_scene.cpp

@@ -1115,7 +1115,7 @@ Ref<Animation> ResourceImporterScene::_save_animation_to_file(Ref<Animation> ani
 	}
 
 	if (ResourceCache::has(p_save_to_path)) {
-		Ref<Animation> old_anim = Ref<Resource>(ResourceCache::get(p_save_to_path));
+		Ref<Animation> old_anim = ResourceCache::get_ref(p_save_to_path);
 		if (old_anim.is_valid()) {
 			old_anim->copy_from(anim);
 			anim = old_anim;
@@ -1711,7 +1711,7 @@ void ResourceImporterScene::_generate_meshes(Node *p_node, const Dictionary &p_m
 				}
 
 				if (!save_to_file.is_empty()) {
-					Ref<Mesh> existing = Ref<Resource>(ResourceCache::get(save_to_file));
+					Ref<Mesh> existing = ResourceCache::get_ref(save_to_file);
 					if (existing.is_valid()) {
 						//if somehow an existing one is useful, create
 						existing->reset_state();

editor/import/resource_importer_texture_atlas.cpp

@@ -306,10 +306,8 @@ Error ResourceImporterTextureAtlas::import_group_file(const String &p_group_file
 
 	//update cache if existing, else create
 	Ref<Texture2D> cache;
-	if (ResourceCache::has(p_group_file)) {
-		Resource *resptr = ResourceCache::get(p_group_file);
-		cache.reference_ptr(resptr);
-	} else {
+	cache = ResourceCache::get_ref(p_group_file);
+	if (!cache.is_valid()) {
 		Ref<ImageTexture> res_cache;
 		res_cache.instantiate();
 		res_cache->create_from_image(new_atlas);

editor/plugins/canvas_item_editor_plugin.cpp

@@ -5455,7 +5455,7 @@ void CanvasItemEditorViewport::_create_nodes(Node *parent, Node *child, String &
 	}
 	child->set_name(name);
 
-	Ref<Texture2D> texture = Ref<Texture2D>(Object::cast_to<Texture2D>(ResourceCache::get(path)));
+	Ref<Texture2D> texture = ResourceCache::get_ref(path);
 
 	if (parent) {
 		editor_data->get_undo_redo().add_do_method(parent, "add_child", child, true);

modules/visual_script/visual_script_flow_control.cpp

@@ -803,7 +803,7 @@ public:
 				//if the script is not in use by anyone, we can safely assume whatever we got is not casting to it.
 				return 1;
 			}
-			Ref<Script> cast_script = Ref<Resource>(ResourceCache::get(script));
+			Ref<Script> cast_script = ResourceCache::get_ref(script);
 			if (!cast_script.is_valid()) {
 				r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD;
 				r_error_str = "Script path is not a script: " + script;

modules/visual_script/visual_script_func_nodes.cpp

@@ -377,7 +377,7 @@ void VisualScriptFunctionCall::_update_method_cache() {
 			}
 
 			if (ResourceCache::has(base_script)) {
-				script = Ref<Resource>(ResourceCache::get(base_script));
+				script = ResourceCache::get_ref(base_script);
 			} else {
 				return;
 			}
@@ -587,7 +587,7 @@ void VisualScriptFunctionCall::_validate_property(PropertyInfo &property) const
 				}
 
 				if (ResourceCache::has(base_script)) {
-					Ref<Script> script = Ref<Resource>(ResourceCache::get(base_script));
+					Ref<Script> script = ResourceCache::get_ref(base_script);
 					if (script.is_valid()) {
 						property.hint = PROPERTY_HINT_METHOD_OF_SCRIPT;
 						property.hint_string = itos(script->get_instance_id());
@@ -1178,7 +1178,7 @@ void VisualScriptPropertySet::_update_cache() {
 				}
 
 				if (ResourceCache::has(base_script)) {
-					script = Ref<Resource>(ResourceCache::get(base_script));
+					script = ResourceCache::get_ref(base_script);
 				} else {
 					return;
 				}
@@ -1338,7 +1338,7 @@ void VisualScriptPropertySet::_validate_property(PropertyInfo &property) const {
 				}
 
 				if (ResourceCache::has(base_script)) {
-					Ref<Script> script = Ref<Resource>(ResourceCache::get(base_script));
+					Ref<Script> script = ResourceCache::get_ref(base_script);
 					if (script.is_valid()) {
 						property.hint = PROPERTY_HINT_PROPERTY_OF_SCRIPT;
 						property.hint_string = itos(script->get_instance_id());
@@ -1864,7 +1864,7 @@ void VisualScriptPropertyGet::_update_cache() {
 				}
 
 				if (ResourceCache::has(base_script)) {
-					script = Ref<Resource>(ResourceCache::get(base_script));
+					script = ResourceCache::get_ref(base_script);
 				} else {
 					return;
 				}
@@ -2044,7 +2044,7 @@ void VisualScriptPropertyGet::_validate_property(PropertyInfo &property) const {
 				}
 
 				if (ResourceCache::has(base_script)) {
-					Ref<Script> script = Ref<Resource>(ResourceCache::get(base_script));
+					Ref<Script> script = ResourceCache::get_ref(base_script);
 					if (script.is_valid()) {
 						property.hint = PROPERTY_HINT_PROPERTY_OF_SCRIPT;
 						property.hint_string = itos(script->get_instance_id());

scene/debugger/scene_debugger.cpp

@@ -701,7 +701,7 @@ void LiveEditor::_res_set_func(int p_id, const StringName &p_prop, const Variant
 		return;
 	}
 
-	Ref<Resource> r = ResourceCache::get(resp);
+	Ref<Resource> r = ResourceCache::get_ref(resp);
 	if (!r.is_valid()) {
 		return;
 	}
@@ -728,7 +728,7 @@ void LiveEditor::_res_call_func(int p_id, const StringName &p_method, const Vari
 		return;
 	}
 
-	Ref<Resource> r = ResourceCache::get(resp);
+	Ref<Resource> r = ResourceCache::get_ref(resp);
 	if (!r.is_valid()) {
 		return;
 	}

scene/resources/resource_format_text.cpp

@@ -528,9 +528,9 @@ Error ResourceLoaderText::load() {
 
 		if (cache_mode == ResourceFormatLoader::CACHE_MODE_REPLACE && ResourceCache::has(path)) {
 			//reuse existing
-			Resource *r = ResourceCache::get(path);
-			if (r && r->get_class() == type) {
-				res = Ref<Resource>(r);
+			Ref<Resource> cache = ResourceCache::get_ref(path);
+			if (cache.is_valid() && cache->get_class() == type) {
+				res = cache;
 				res->reset_state();
 				do_assign = true;
 			}
@@ -539,10 +539,10 @@ Error ResourceLoaderText::load() {
 		MissingResource *missing_resource = nullptr;
 
 		if (res.is_null()) { //not reuse
-			if (cache_mode != ResourceFormatLoader::CACHE_MODE_IGNORE && ResourceCache::has(path)) { //only if it doesn't exist
+			Ref<Resource> cache = ResourceCache::get_ref(path);
+			if (cache_mode != ResourceFormatLoader::CACHE_MODE_IGNORE && cache.is_valid()) { //only if it doesn't exist
 				//cached, do not assign
-				Resource *r = ResourceCache::get(path);
-				res = Ref<Resource>(r);
+				res = cache;
 			} else {
 				//create
 
@@ -652,12 +652,10 @@ Error ResourceLoaderText::load() {
 			return error;
 		}
 
-		if (cache_mode == ResourceFormatLoader::CACHE_MODE_REPLACE && ResourceCache::has(local_path)) {
-			Resource *r = ResourceCache::get(local_path);
-			if (r->get_class() == res_type) {
-				r->reset_state();
-				resource = Ref<Resource>(r);
-			}
+		Ref<Resource> cache = ResourceCache::get_ref(local_path);
+		if (cache_mode == ResourceFormatLoader::CACHE_MODE_REPLACE && cache.is_valid() && cache->get_class() == res_type) {
+			cache->reset_state();
+			resource = cache;
 		}
 
 		MissingResource *missing_resource = nullptr;