Add thread safety to Object signals

* It turns out the majority of this work was done already by AThousandShips as part of #89451. This allows to do lock-less emitting of signals.
* This means, that only the signal map needs to be protected, making the task simple and without risk of deadlocks, or affecting performance.
* Objects can choose to not protect signals for performance (as example Node uses thread guards for protection, so these signals are not thread safe).

core/object/object.cpp

@@ -65,6 +65,23 @@ struct _ObjectDebugLock {
 
 #endif
 
+struct _ObjectSignalLock {
+	Mutex *mutex;
+	_ObjectSignalLock(const Object *const p_obj) {
+		mutex = p_obj->signal_mutex;
+		if (mutex) {
+			mutex->lock();
+		}
+	}
+	~_ObjectSignalLock() {
+		if (mutex) {
+			mutex->unlock();
+		}
+	}
+};
+
+#define OBJ_SIGNAL_LOCK _ObjectSignalLock _signal_lock(this);
+
 PropertyInfo::operator Dictionary() const {
 	Dictionary d;
 	d["name"] = name;
@@ -255,6 +272,9 @@ void Object::_initialize() {
 }
 
 void Object::_postinitialize() {
+	if (_uses_signal_mutex()) {
+		signal_mutex = memnew(Mutex);
+	}
 	notification(NOTIFICATION_POSTINITIALIZE);
 }
 
@@ -1123,6 +1143,9 @@ void Object::get_meta_list(List<StringName> *p_list) const {
 void Object::add_user_signal(const MethodInfo &p_signal) {
 	ERR_FAIL_COND_MSG(p_signal.name.is_empty(), "Signal name cannot be empty.");
 	ERR_FAIL_COND_MSG(ClassDB::has_signal(get_class_name(), p_signal.name), vformat("User signal's name conflicts with a built-in signal of '%s'.", get_class_name()));
+
+	OBJ_SIGNAL_LOCK
+
 	ERR_FAIL_COND_MSG(signal_map.has(p_signal.name), vformat("Trying to add already existing signal '%s'.", p_signal.name));
 	SignalData s;
 	s.user = p_signal;
@@ -1130,6 +1153,8 @@ void Object::add_user_signal(const MethodInfo &p_signal) {
 }
 
 bool Object::_has_user_signal(const StringName &p_name) const {
+	OBJ_SIGNAL_LOCK
+
 	if (!signal_map.has(p_name)) {
 		return false;
 	}
@@ -1137,6 +1162,8 @@ bool Object::_has_user_signal(const StringName &p_name) const {
 }
 
 void Object::_remove_user_signal(const StringName &p_name) {
+	OBJ_SIGNAL_LOCK
+
 	SignalData *s = signal_map.getptr(p_name);
 	ERR_FAIL_NULL_MSG(s, "Provided signal does not exist.");
 	ERR_FAIL_COND_MSG(!s->removable, "Signal is not removable (not added with add_user_signal).");
@@ -1183,6 +1210,13 @@ Error Object::emit_signalp(const StringName &p_name, const Variant **p_args, int
 		return ERR_CANT_ACQUIRE_RESOURCE; //no emit, signals blocked
 	}
 
+	Callable *slot_callables = nullptr;
+	uint32_t *slot_flags = nullptr;
+	uint32_t slot_count = 0;
+
+	{
+		OBJ_SIGNAL_LOCK
+
 		SignalData *s = signal_map.getptr(p_name);
 		if (!s) {
 #ifdef DEBUG_ENABLED
@@ -1200,9 +1234,8 @@ Error Object::emit_signalp(const StringName &p_name, const Variant **p_args, int
 
 		// Ensure that disconnecting the signal or even deleting the object
 		// will not affect the signal calling.
-	Callable *slot_callables = (Callable *)alloca(sizeof(Callable) * s->slot_map.size());
+		slot_callables = (Callable *)alloca(sizeof(Callable) * s->slot_map.size());
-	uint32_t *slot_flags = (uint32_t *)alloca(sizeof(uint32_t) * s->slot_map.size());
+		slot_flags = (uint32_t *)alloca(sizeof(uint32_t) * s->slot_map.size());
-	uint32_t slot_count = 0;
 
 		for (const KeyValue<Callable, SignalData::Slot> &slot_kv : s->slot_map) {
 			memnew_placement(&slot_callables[slot_count], Callable(slot_kv.value.conn.callable));
@@ -1225,6 +1258,7 @@ Error Object::emit_signalp(const StringName &p_name, const Variant **p_args, int
 				_disconnect(p_name, slot_callables[i]);
 			}
 		}
+	}
 
 	OBJ_DEBUG_LOCK
 
@@ -1280,6 +1314,8 @@ void Object::_add_user_signal(const String &p_name, const Array &p_args) {
 	// without access to ADD_SIGNAL in bind_methods
 	// added events are per instance, as opposed to the other ones, which are global
 
+	OBJ_SIGNAL_LOCK
+
 	MethodInfo mi;
 	mi.name = p_name;
 
@@ -1360,6 +1396,8 @@ bool Object::has_signal(const StringName &p_name) const {
 }
 
 void Object::get_signal_list(List<MethodInfo> *p_signals) const {
+	OBJ_SIGNAL_LOCK
+
 	if (!script.is_null()) {
 		Ref<Script> scr = script;
 		if (scr.is_valid()) {
@@ -1379,6 +1417,8 @@ void Object::get_signal_list(List<MethodInfo> *p_signals) const {
 }
 
 void Object::get_all_signal_connections(List<Connection> *p_connections) const {
+	OBJ_SIGNAL_LOCK
+
 	for (const KeyValue<StringName, SignalData> &E : signal_map) {
 		const SignalData *s = &E.value;
 
@@ -1389,6 +1429,8 @@ void Object::get_all_signal_connections(List<Connection> *p_connections) const {
 }
 
 void Object::get_signal_connection_list(const StringName &p_signal, List<Connection> *p_connections) const {
+	OBJ_SIGNAL_LOCK
+
 	const SignalData *s = signal_map.getptr(p_signal);
 	if (!s) {
 		return; //nothing
@@ -1400,6 +1442,7 @@ void Object::get_signal_connection_list(const StringName &p_signal, List<Connect
 }
 
 int Object::get_persistent_signal_connection_count() const {
+	OBJ_SIGNAL_LOCK
 	int count = 0;
 
 	for (const KeyValue<StringName, SignalData> &E : signal_map) {
@@ -1416,6 +1459,8 @@ int Object::get_persistent_signal_connection_count() const {
 }
 
 void Object::get_signals_connected_to_this(List<Connection> *p_connections) const {
+	OBJ_SIGNAL_LOCK
+
 	for (const Connection &E : connections) {
 		p_connections->push_back(E);
 	}
@@ -1423,6 +1468,7 @@ void Object::get_signals_connected_to_this(List<Connection> *p_connections) cons
 
 Error Object::connect(const StringName &p_signal, const Callable &p_callable, uint32_t p_flags) {
 	ERR_FAIL_COND_V_MSG(p_callable.is_null(), ERR_INVALID_PARAMETER, vformat("Cannot connect to '%s': the provided callable is null.", p_signal));
+	OBJ_SIGNAL_LOCK
 
 	if (p_callable.is_standard()) {
 		// FIXME: This branch should probably removed in favor of the `is_valid()` branch, but there exist some classes
@@ -1491,6 +1537,8 @@ Error Object::connect(const StringName &p_signal, const Callable &p_callable, ui
 
 bool Object::is_connected(const StringName &p_signal, const Callable &p_callable) const {
 	ERR_FAIL_COND_V_MSG(p_callable.is_null(), false, vformat("Cannot determine if connected to '%s': the provided callable is null.", p_signal)); // Should use `is_null`, see note in `connect` about the use of `is_valid`.
+	OBJ_SIGNAL_LOCK
+
 	const SignalData *s = signal_map.getptr(p_signal);
 	if (!s) {
 		bool signal_is_valid = ClassDB::has_signal(get_class_name(), p_signal);
@@ -1509,6 +1557,8 @@ bool Object::is_connected(const StringName &p_signal, const Callable &p_callable
 }
 
 bool Object::has_connections(const StringName &p_signal) const {
+	OBJ_SIGNAL_LOCK
+
 	const SignalData *s = signal_map.getptr(p_signal);
 	if (!s) {
 		bool signal_is_valid = ClassDB::has_signal(get_class_name(), p_signal);
@@ -1532,6 +1582,7 @@ void Object::disconnect(const StringName &p_signal, const Callable &p_callable)
 
 bool Object::_disconnect(const StringName &p_signal, const Callable &p_callable, bool p_force) {
 	ERR_FAIL_COND_V_MSG(p_callable.is_null(), false, vformat("Cannot disconnect from '%s': the provided callable is null.", p_signal)); // Should use `is_null`, see note in `connect` about the use of `is_valid`.
+	OBJ_SIGNAL_LOCK
 
 	SignalData *s = signal_map.getptr(p_signal);
 	if (!s) {
@@ -1569,6 +1620,10 @@ bool Object::_disconnect(const StringName &p_signal, const Callable &p_callable,
 	return true;
 }
 
+bool Object::_uses_signal_mutex() const {
+	return true;
+}
+
 void Object::_set_bind(const StringName &p_set, const Variant &p_value) {
 	set(p_set, p_value);
 }
@@ -2204,6 +2259,8 @@ Object::~Object() {
 		ERR_PRINT(vformat("Object '%s' was freed or unreferenced while a signal is being emitted from it. Try connecting to the signal using 'CONNECT_DEFERRED' flag, or use queue_free() to free the object (if this object is a Node) to avoid this error and potential crashes.", to_string()));
 	}
 
+	{
+		OBJ_SIGNAL_LOCK
 		// Drop all connections to the signals of this object.
 		while (signal_map.size()) {
 			// Avoid regular iteration so erasing is safe.
@@ -2233,6 +2290,7 @@ Object::~Object() {
 				connections.pop_front();
 			}
 		}
+	}
 
 	if (_instance_id != ObjectID()) {
 		ObjectDB::remove_instance(this);
@@ -2248,6 +2306,10 @@ Object::~Object() {
 		}
 		memfree(_instance_bindings);
 	}
+
+	if (signal_mutex) {
+		memdelete(signal_mutex);
+	}
 }
 
 bool predelete_handler(Object *p_object) {

core/object/object.h

@@ -608,7 +608,8 @@ private:
 		HashMap<Callable, Slot, HashableHasher<Callable>> slot_map;
 		bool removable = false;
 	};
-
+	friend struct _ObjectSignalLock;
+	mutable Mutex *signal_mutex = nullptr;
 	HashMap<StringName, SignalData> signal_map;
 	List<Connection> connections;
 #ifdef DEBUG_ENABLED
@@ -756,6 +757,8 @@ protected:
 
 	bool _disconnect(const StringName &p_signal, const Callable &p_callable, bool p_force = false);
 
+	virtual bool _uses_signal_mutex() const;
+
 #ifdef TOOLS_ENABLED
 	struct VirtualMethodTracker {
 		void **method;

doc/classes/Object.xml

@@ -521,6 +521,7 @@
 				A signal can only be connected once to the same [Callable]. If the signal is already connected, this method returns [constant ERR_INVALID_PARAMETER] and generates an error, unless the signal is connected with [constant CONNECT_REFERENCE_COUNTED]. To prevent this, use [method is_connected] first to check for existing connections.
 				[b]Note:[/b] If the [param callable]'s object is freed, the connection will be lost.
 				[b]Note:[/b] In GDScript, it is generally recommended to connect signals with [method Signal.connect] instead.
+				[b]Note:[/b] This operation (and all other signal related operations) is thread-safe.
 			</description>
 		</method>
 		<method name="disconnect">

scene/main/node.h

@@ -387,6 +387,8 @@ protected:
 	void _validate_property(PropertyInfo &p_property) const;
 
 protected:
+	virtual bool _uses_signal_mutex() const override { return false; } // Node uses thread guards instead.
+
 	virtual void input(const Ref<InputEvent> &p_event);
 	virtual void shortcut_input(const Ref<InputEvent> &p_key_event);
 	virtual void unhandled_input(const Ref<InputEvent> &p_event);