summaryrefslogtreecommitdiff
path: root/core
diff options
context:
space:
mode:
authorreduz <reduzio@gmail.com>2022-06-22 13:46:46 +0200
committerreduz <reduzio@gmail.com>2022-06-22 13:46:46 +0200
commite772b65d92dbd5b36fb003458d7fe0fd528abcea (patch)
treeaca74602fef05e8f80689345af9ba2e33afe846d /core
parentc18d0f20357a11bd9cfa2f57b8b9b500763413bc (diff)
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
Diffstat (limited to 'core')
-rw-r--r--core/io/resource.cpp96
-rw-r--r--core/io/resource.h4
-rw-r--r--core/io/resource_format_binary.cpp10
-rw-r--r--core/io/resource_loader.cpp51
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()) {