Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use MutexLock in more places #96166

Merged
merged 1 commit into from
Aug 30, 2024
Merged

Use MutexLock in more places #96166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions core/debugger/remote_debugger_peer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,8 @@ void RemoteDebuggerPeerTCP::_read_in() {
Error err = decode_variant(var, buf, in_pos, &read);
ERR_CONTINUE(read != in_pos || err != OK);
ERR_CONTINUE_MSG(var.get_type() != Variant::ARRAY, "Malformed packet received, not an Array.");
mutex.lock();
MutexLock lock(mutex);
in_queue.push_back(var);
mutex.unlock();
}
}
}
Expand Down
99 changes: 46 additions & 53 deletions core/io/resource.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,32 +60,32 @@ void Resource::set_path(const String &p_path, bool p_take_over) {
p_take_over = false; // Can't take over an empty path
}

ResourceCache::lock.lock();
{
MutexLock lock(ResourceCache::lock);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably rename ResourceCache::lock to ResourceCache::mutex at some point as it's just a recipe for confusion having an incorrect name for it, left it alone in this PR though


if (!path_cache.is_empty()) {
ResourceCache::resources.erase(path_cache);
}
if (!path_cache.is_empty()) {
ResourceCache::resources.erase(path_cache);
}

path_cache = "";
path_cache = "";

Ref<Resource> existing = ResourceCache::get_ref(p_path);
Ref<Resource> existing = ResourceCache::get_ref(p_path);

if (existing.is_valid()) {
if (p_take_over) {
existing->path_cache = String();
ResourceCache::resources.erase(p_path);
} else {
ResourceCache::lock.unlock();
ERR_FAIL_MSG("Another resource is loaded from path '" + p_path + "' (possible cyclic resource inclusion).");
if (existing.is_valid()) {
if (p_take_over) {
existing->path_cache = String();
ResourceCache::resources.erase(p_path);
} else {
ERR_FAIL_MSG("Another resource is loaded from path '" + p_path + "' (possible cyclic resource inclusion).");
}
}
}

path_cache = p_path;
path_cache = p_path;

if (!path_cache.is_empty()) {
ResourceCache::resources[path_cache] = this;
if (!path_cache.is_empty()) {
ResourceCache::resources[path_cache] = this;
}
}
ResourceCache::lock.unlock();

_resource_path_changed();
}
Expand Down Expand Up @@ -486,15 +486,13 @@ void Resource::set_as_translation_remapped(bool p_remapped) {
return;
}

ResourceCache::lock.lock();
MutexLock lock(ResourceCache::lock);

if (p_remapped) {
ResourceLoader::remapped_list.add(&remapped_list);
} else {
ResourceLoader::remapped_list.remove(&remapped_list);
}

ResourceCache::lock.unlock();
}

#ifdef TOOLS_ENABLED
Expand Down Expand Up @@ -564,14 +562,13 @@ Resource::~Resource() {
return;
}

ResourceCache::lock.lock();
MutexLock lock(ResourceCache::lock);
// Only unregister from the cache if this is the actual resource listed there.
// (Other resources can have the same value in `path_cache` if loaded with `CACHE_IGNORE`.)
HashMap<String, Resource *>::Iterator E = ResourceCache::resources.find(path_cache);
if (likely(E && E->value == this)) {
ResourceCache::resources.remove(E);
}
ResourceCache::lock.unlock();
}

HashMap<String, Resource *> ResourceCache::resources;
Expand Down Expand Up @@ -600,18 +597,20 @@ void ResourceCache::clear() {
}

bool ResourceCache::has(const String &p_path) {
lock.lock();
Resource **res = nullptr;

Resource **res = resources.getptr(p_path);
{
MutexLock mutex_lock(lock);

if (res && (*res)->get_reference_count() == 0) {
// This resource is in the process of being deleted, ignore its existence.
(*res)->path_cache = String();
resources.erase(p_path);
res = nullptr;
}
res = resources.getptr(p_path);

lock.unlock();
if (res && (*res)->get_reference_count() == 0) {
// This resource is in the process of being deleted, ignore its existence.
(*res)->path_cache = String();
resources.erase(p_path);
res = nullptr;
}
}

if (!res) {
return false;
Expand All @@ -622,28 +621,27 @@ bool ResourceCache::has(const String &p_path) {

Ref<Resource> ResourceCache::get_ref(const String &p_path) {
Ref<Resource> ref;
lock.lock();

Resource **res = resources.getptr(p_path);
{
MutexLock mutex_lock(lock);
Resource **res = resources.getptr(p_path);

if (res) {
ref = Ref<Resource>(*res);
}
if (res) {
ref = Ref<Resource>(*res);
}

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

lock.unlock();

return ref;
}

void ResourceCache::get_cached_resources(List<Ref<Resource>> *p_resources) {
lock.lock();
MutexLock mutex_lock(lock);

LocalVector<String> to_remove;

Expand All @@ -663,14 +661,9 @@ void ResourceCache::get_cached_resources(List<Ref<Resource>> *p_resources) {
for (const String &E : to_remove) {
resources.erase(E);
}

lock.unlock();
}

int ResourceCache::get_cached_resource_count() {
lock.lock();
int rc = resources.size();
lock.unlock();

return rc;
MutexLock mutex_lock(lock);
return resources.size();
}
64 changes: 31 additions & 33 deletions core/io/resource_loader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,28 +227,27 @@ void ResourceFormatLoader::_bind_methods() {

// This should be robust enough to be called redundantly without issues.
void ResourceLoader::LoadToken::clear() {
thread_load_mutex.lock();

WorkerThreadPool::TaskID task_to_await = 0;

// User-facing tokens shouldn't be deleted until completely claimed.
DEV_ASSERT(user_rc == 0 && user_path.is_empty());

if (!local_path.is_empty()) { // Empty is used for the special case where the load task is not registered.
DEV_ASSERT(thread_load_tasks.has(local_path));
ThreadLoadTask &load_task = thread_load_tasks[local_path];
if (load_task.task_id && !load_task.awaited) {
task_to_await = load_task.task_id;
{
MutexLock thread_load_lock(thread_load_mutex);
// User-facing tokens shouldn't be deleted until completely claimed.
DEV_ASSERT(user_rc == 0 && user_path.is_empty());

if (!local_path.is_empty()) { // Empty is used for the special case where the load task is not registered.
DEV_ASSERT(thread_load_tasks.has(local_path));
ThreadLoadTask &load_task = thread_load_tasks[local_path];
if (load_task.task_id && !load_task.awaited) {
task_to_await = load_task.task_id;
}
// Removing a task which is still in progress would be catastrophic.
// Tokens must be alive until the task thread function is done.
DEV_ASSERT(load_task.status == THREAD_LOAD_FAILED || load_task.status == THREAD_LOAD_LOADED);
thread_load_tasks.erase(local_path);
local_path.clear();
}
// Removing a task which is still in progress would be catastrophic.
// Tokens must be alive until the task thread function is done.
DEV_ASSERT(load_task.status == THREAD_LOAD_FAILED || load_task.status == THREAD_LOAD_LOADED);
thread_load_tasks.erase(local_path);
local_path.clear();
}

thread_load_mutex.unlock();

// If task is unused, await it here, locally, now the token data is consistent.
if (task_to_await) {
PREPARE_FOR_WTP_WAIT
Expand All @@ -265,15 +264,14 @@ Ref<Resource> ResourceLoader::_load(const String &p_path, const String &p_origin
const String &original_path = p_original_path.is_empty() ? p_path : p_original_path;
load_nesting++;
if (load_paths_stack.size()) {
thread_load_mutex.lock();
MutexLock thread_load_lock(thread_load_mutex);
const String &parent_task_path = load_paths_stack.get(load_paths_stack.size() - 1);
HashMap<String, ThreadLoadTask>::Iterator E = thread_load_tasks.find(parent_task_path);
// Avoid double-tracking, for progress reporting, resources that boil down to a remapped path containing the real payload (e.g., imported resources).
bool is_remapped_load = original_path == parent_task_path;
if (E && !is_remapped_load) {
E->value.sub_tasks.insert(p_original_path);
}
thread_load_mutex.unlock();
}
load_paths_stack.push_back(original_path);

Expand Down Expand Up @@ -318,13 +316,13 @@ Ref<Resource> ResourceLoader::_load(const String &p_path, const String &p_origin
void ResourceLoader::_run_load_task(void *p_userdata) {
ThreadLoadTask &load_task = *(ThreadLoadTask *)p_userdata;

thread_load_mutex.lock();
if (cleaning_tasks) {
load_task.status = THREAD_LOAD_FAILED;
thread_load_mutex.unlock();
return;
{
MutexLock thread_load_lock(thread_load_mutex);
if (cleaning_tasks) {
load_task.status = THREAD_LOAD_FAILED;
return;
}
}
thread_load_mutex.unlock();

// Thread-safe either if it's the current thread or a brand new one.
CallQueue *own_mq_override = nullptr;
Expand Down Expand Up @@ -1170,17 +1168,17 @@ String ResourceLoader::path_remap(const String &p_path) {
}

void ResourceLoader::reload_translation_remaps() {
ResourceCache::lock.lock();

List<Resource *> to_reload;
SelfList<Resource> *E = remapped_list.first();

while (E) {
to_reload.push_back(E->self());
E = E->next();
}
{
MutexLock lock(ResourceCache::lock);
SelfList<Resource> *E = remapped_list.first();

ResourceCache::lock.unlock();
while (E) {
to_reload.push_back(E->self());
E = E->next();
}
}

//now just make sure to not delete any of these resources while changing locale..
while (to_reload.front()) {
Expand Down
11 changes: 3 additions & 8 deletions core/object/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1904,7 +1904,7 @@ void Object::set_instance_binding(void *p_token, void *p_binding, const GDExtens

void *Object::get_instance_binding(void *p_token, const GDExtensionInstanceBindingCallbacks *p_callbacks) {
void *binding = nullptr;
_instance_binding_mutex.lock();
MutexLock instance_binding_lock(_instance_binding_mutex);
for (uint32_t i = 0; i < _instance_binding_count; i++) {
if (_instance_bindings[i].token == p_token) {
binding = _instance_bindings[i].binding;
Expand Down Expand Up @@ -1935,29 +1935,25 @@ void *Object::get_instance_binding(void *p_token, const GDExtensionInstanceBindi
_instance_binding_count++;
}

_instance_binding_mutex.unlock();

return binding;
}

bool Object::has_instance_binding(void *p_token) {
bool found = false;
_instance_binding_mutex.lock();
MutexLock instance_binding_lock(_instance_binding_mutex);
for (uint32_t i = 0; i < _instance_binding_count; i++) {
if (_instance_bindings[i].token == p_token) {
found = true;
break;
}
}

_instance_binding_mutex.unlock();

return found;
}

void Object::free_instance_binding(void *p_token) {
bool found = false;
_instance_binding_mutex.lock();
MutexLock instance_binding_lock(_instance_binding_mutex);
for (uint32_t i = 0; i < _instance_binding_count; i++) {
if (!found && _instance_bindings[i].token == p_token) {
if (_instance_bindings[i].free_callback) {
Expand All @@ -1976,7 +1972,6 @@ void Object::free_instance_binding(void *p_token) {
if (found) {
_instance_binding_count--;
}
_instance_binding_mutex.unlock();
}

#ifdef TOOLS_ENABLED
Expand Down
3 changes: 1 addition & 2 deletions core/object/object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,15 +667,14 @@ class Object {
_FORCE_INLINE_ bool _instance_binding_reference(bool p_reference) {
bool can_die = true;
if (_instance_bindings) {
_instance_binding_mutex.lock();
MutexLock instance_binding_lock(_instance_binding_mutex);
for (uint32_t i = 0; i < _instance_binding_count; i++) {
if (_instance_bindings[i].reference_callback) {
if (!_instance_bindings[i].reference_callback(_instance_bindings[i].token, _instance_bindings[i].binding, p_reference)) {
can_die = false;
}
}
}
_instance_binding_mutex.unlock();
}
return can_die;
}
Expand Down
Loading
Loading