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

Jump to bottom

Avoid dangling lambda-script links when former destroyed earlier #85223

Closed
wants to merge 1 commit into from
Closed

Avoid dangling lambda-script links when former destroyed earlier #85223

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
59 changes: 39 additions & 20 deletions modules/gdscript/gdscript.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1394,38 +1394,38 @@ String GDScript::debug_get_script_name(const Ref<Script> &p_script) {
thread_local GDScript::UpdatableFuncPtr GDScript::func_ptrs_to_update_thread_local;
GDScript::UpdatableFuncPtr *GDScript::func_ptrs_to_update_main_thread = &func_ptrs_to_update_thread_local;

List<GDScript::UpdatableFuncPtrElement>::Element *GDScript::_add_func_ptr_to_update(GDScriptFunction **p_func_ptr_ptr) {
MutexLock lock(func_ptrs_to_update_mutex);
void GDScript::_add_func_ptr_to_update(GDScriptFunction **p_func_ptr_ptr, List<UpdatableFuncPtrElement>::Element **r_element_ptr) {
MutexLock lock(func_ptrs_to_update_thread_local.mutex);
MutexLock lock2(func_ptrs_to_update_mutex);

List<UpdatableFuncPtrElement>::Element *result = func_ptrs_to_update_elems.push_back(UpdatableFuncPtrElement());

{
MutexLock lock2(func_ptrs_to_update_thread_local.mutex);
result->get().element = func_ptrs_to_update_thread_local.ptrs.push_back(p_func_ptr_ptr);
result->get().mutex = &func_ptrs_to_update_thread_local.mutex;

if (likely(func_ptrs_to_update_thread_local.initialized)) {
return result;
}
result->get().element = func_ptrs_to_update_thread_local.ptrs.push_back(p_func_ptr_ptr);
result->get().element_ptr = r_element_ptr;
result->get().thread_mutex = &func_ptrs_to_update_thread_local.mutex;
result->get().script_mutex = &func_ptrs_to_update_mutex;
*r_element_ptr = result;

func_ptrs_to_update_thread_local.initialized = true;
if (likely(func_ptrs_to_update_thread_local.initialized)) {
return;
}

func_ptrs_to_update.push_back(&func_ptrs_to_update_thread_local);
func_ptrs_to_update_thread_local.initialized = true;

return result;
func_ptrs_to_update.push_back(&func_ptrs_to_update_thread_local);
}

void GDScript::_remove_func_ptr_to_update(List<UpdatableFuncPtrElement>::Element *p_func_ptr_element) {
// None of these checks should ever fail, unless there's a bug.
// They can be removed once we are sure they never catch anything.
// Left here now due to extra safety needs late in the release cycle.
ERR_FAIL_NULL(p_func_ptr_element);
MutexLock lock(func_ptrs_to_update_thread_local.mutex);
ERR_FAIL_NULL(p_func_ptr_element->get().element);
ERR_FAIL_NULL(p_func_ptr_element->get().mutex);
MutexLock lock2(*p_func_ptr_element->get().mutex);
ERR_FAIL_NULL(p_func_ptr_element->get().thread_mutex);
ERR_FAIL_NULL(p_func_ptr_element->get().script_mutex);
MutexLock lock(*p_func_ptr_element->get().thread_mutex);
MutexLock lock2(*p_func_ptr_element->get().script_mutex);
RandomShaper marked this conversation as resolved.
Show resolved Hide resolved
p_func_ptr_element->get().element->erase();
*p_func_ptr_element->get().element_ptr = nullptr;
p_func_ptr_element->erase();
}

Expand Down Expand Up @@ -1457,12 +1457,12 @@ void GDScript::_fixup_thread_function_bookkeeping() {
DEV_ASSERT(script->func_ptrs_to_update.find(&func_ptrs_to_update_thread_local));
{
for (List<UpdatableFuncPtrElement>::Element *F = script->func_ptrs_to_update_elems.front(); F; F = F->next()) {
bool is_dying_thread_entry = F->get().mutex == &func_ptrs_to_update_thread_local.mutex;
bool is_dying_thread_entry = F->get().thread_mutex == &func_ptrs_to_update_thread_local.mutex;
if (is_dying_thread_entry) {
// This may lead to multiple main-thread entries, but that's not a problem
// and allows to reuse the element, which is needed, since it's tracked by pointer.
F->get().element = new_E;
F->get().mutex = &func_ptrs_to_update_main_thread->mutex;
F->get().thread_mutex = &func_ptrs_to_update_main_thread->mutex;
}
}
}
Expand Down Expand Up @@ -1521,7 +1521,26 @@ void GDScript::clear(ClearData *p_clear_data) {
*func_ptr_ptr = nullptr;
}
}
func_ptrs_to_update_elems.clear();

{
Mutex *last_thread_mutex = nullptr;
for (UpdatableFuncPtrElement &elem : func_ptrs_to_update_elems) {
if (last_thread_mutex != elem.thread_mutex) {
if (last_thread_mutex) {
last_thread_mutex->unlock();
}
elem.thread_mutex->lock();
last_thread_mutex = elem.thread_mutex;
}

*elem.element_ptr = nullptr;
elem.element->erase();
}
if (last_thread_mutex) {
last_thread_mutex->unlock();
}
func_ptrs_to_update_elems.clear();
}
}

RBSet<GDScript *> must_clear_dependencies = get_must_clear_dependencies();
Expand Down
7 changes: 4 additions & 3 deletions modules/gdscript/gdscript.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,16 +125,17 @@ class GDScript : public Script {
};
struct UpdatableFuncPtrElement {
List<GDScriptFunction **>::Element *element = nullptr;
Mutex *mutex = nullptr;
List<UpdatableFuncPtrElement>::Element **element_ptr = nullptr;
Mutex *thread_mutex = nullptr;
Mutex *script_mutex = nullptr;
};
static thread_local UpdatableFuncPtr func_ptrs_to_update_thread_local;
static thread_local LocalVector<List<UpdatableFuncPtr *>::Element> func_ptrs_to_update_entries_thread_local;
static UpdatableFuncPtr *func_ptrs_to_update_main_thread;
List<UpdatableFuncPtr *> func_ptrs_to_update;
List<UpdatableFuncPtrElement> func_ptrs_to_update_elems;
Mutex func_ptrs_to_update_mutex;

List<UpdatableFuncPtrElement>::Element *_add_func_ptr_to_update(GDScriptFunction **p_func_ptr_ptr);
void _add_func_ptr_to_update(GDScriptFunction **p_func_ptr_ptr, List<UpdatableFuncPtrElement>::Element **r_element_ptr);
static void _remove_func_ptr_to_update(List<UpdatableFuncPtrElement>::Element *p_func_ptr_element);

static void _fixup_thread_function_bookkeeping();
Expand Down
10 changes: 6 additions & 4 deletions modules/gdscript/gdscript_lambda_callable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,13 @@ GDScriptLambdaCallable::GDScriptLambdaCallable(Ref<GDScript> p_script, GDScriptF

h = (uint32_t)hash_murmur3_one_64((uint64_t)this);

updatable_func_ptr_element = p_script->_add_func_ptr_to_update(&function);
p_script->_add_func_ptr_to_update(&function, &updatable_func_ptr_element);
}

GDScriptLambdaCallable::~GDScriptLambdaCallable() {
GDScript::_remove_func_ptr_to_update(updatable_func_ptr_element);
if (updatable_func_ptr_element) {
GDScript::_remove_func_ptr_to_update(updatable_func_ptr_element);
}
}

bool GDScriptLambdaSelfCallable::compare_equal(const CallableCustom *p_a, const CallableCustom *p_b) {
Expand Down Expand Up @@ -276,7 +278,7 @@ GDScriptLambdaSelfCallable::GDScriptLambdaSelfCallable(Ref<RefCounted> p_self, G

GDScript *gds = p_function->get_script();
if (gds != nullptr) {
updatable_func_ptr_element = gds->_add_func_ptr_to_update(&function);
gds->_add_func_ptr_to_update(&function, &updatable_func_ptr_element);
}
}

Expand All @@ -291,7 +293,7 @@ GDScriptLambdaSelfCallable::GDScriptLambdaSelfCallable(Object *p_self, GDScriptF

GDScript *gds = p_function->get_script();
if (gds != nullptr) {
updatable_func_ptr_element = gds->_add_func_ptr_to_update(&function);
gds->_add_func_ptr_to_update(&function, &updatable_func_ptr_element);
}
}

Expand Down
Loading