Skip to content

Commit

Permalink
Avoid dangling lambda-script links when former destroyed earlier
Browse files Browse the repository at this point in the history
Additionally, mutex usage is made more correct.
  • Loading branch information
RandomShaper committed Nov 22, 2023
1 parent c2f8fb3 commit 537035b
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 27 deletions.
59 changes: 39 additions & 20 deletions modules/gdscript/gdscript.cpp
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);
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
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
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

0 comments on commit 537035b

Please sign in to comment.