Merge pull request godotengine#78000 from RandomShaper/let_users_mess_up

Let user scripts disable thread safety checks
rohanrhu · Jun 10, 2023 · 37d1dfe · 37d1dfe
2 parents 65969dd + 2b001db
commit 37d1dfe
Show file tree

Hide file tree

Showing 6 changed files with 27 additions and 2 deletions.
diff --git a/core/core_bind.cpp b/core/core_bind.cpp
@@ -39,6 +39,7 @@
 #include "core/math/geometry_2d.h"
 #include "core/math/geometry_3d.h"
 #include "core/os/keyboard.h"
+#include "core/os/thread_safe.h"
 #include "core/variant/typed_array.h"
 
 namespace core_bind {
@@ -1259,13 +1260,20 @@ Variant Thread::wait_to_finish() {
 	return r;
 }
 
+void Thread::set_thread_safety_checks_enabled(bool p_enabled) {
+	ERR_FAIL_COND_MSG(::Thread::is_main_thread(), "This call is forbidden on the main thread.");
+	set_current_thread_safe_for_nodes(!p_enabled);
+}
+
 void Thread::_bind_methods() {
 	ClassDB::bind_method(D_METHOD("start", "callable", "priority"), &Thread::start, DEFVAL(PRIORITY_NORMAL));
 	ClassDB::bind_method(D_METHOD("get_id"), &Thread::get_id);
 	ClassDB::bind_method(D_METHOD("is_started"), &Thread::is_started);
 	ClassDB::bind_method(D_METHOD("is_alive"), &Thread::is_alive);
 	ClassDB::bind_method(D_METHOD("wait_to_finish"), &Thread::wait_to_finish);
 
+	ClassDB::bind_static_method("Thread", D_METHOD("set_thread_safety_checks_enabled", "enabled"), &Thread::set_thread_safety_checks_enabled);
+
 	BIND_ENUM_CONSTANT(PRIORITY_LOW);
 	BIND_ENUM_CONSTANT(PRIORITY_NORMAL);
 	BIND_ENUM_CONSTANT(PRIORITY_HIGH);

diff --git a/core/core_bind.h b/core/core_bind.h
@@ -408,6 +408,8 @@ class Thread : public RefCounted {
 	bool is_started() const;
 	bool is_alive() const;
 	Variant wait_to_finish();
+
+	static void set_thread_safety_checks_enabled(bool p_enabled);
 };
 
 namespace special {

diff --git a/core/io/resource_loader.cpp b/core/io/resource_loader.cpp
@@ -358,7 +358,6 @@ void ResourceLoader::_thread_load_function(void *p_userdata) {
 
 	if (load_nesting == 0 && mq_override) {
 		memdelete(mq_override);
-		set_current_thread_safe_for_nodes(false);
 	}
 }
 

diff --git a/core/object/worker_thread_pool.cpp b/core/object/worker_thread_pool.cpp
@@ -31,6 +31,7 @@
 #include "worker_thread_pool.h"
 
 #include "core/os/os.h"
+#include "core/os/thread_safe.h"
 
 void WorkerThreadPool::Task::free_template_userdata() {
 	ERR_FAIL_COND(!template_userdata);
@@ -178,6 +179,9 @@ void WorkerThreadPool::_process_task(Task *p_task) {
 		if (post) {
 			task_available_semaphore.post();
 		}
+
+		// Engine/user tasks can set-and-forget, so we must be sure it's back to normal by the end of the task.
+		set_current_thread_safe_for_nodes(false);
 	}
 }
 

diff --git a/doc/classes/Thread.xml b/doc/classes/Thread.xml
@@ -37,6 +37,19 @@
 				Returns [code]true[/code] if this [Thread] has been started. Once started, this will return [code]true[/code] until it is joined using [method wait_to_finish]. For checking if a [Thread] is still executing its task, use [method is_alive].
 			</description>
 		</method>
+		<method name="set_thread_safety_checks_enabled" qualifiers="static">
+			<return type="void" />
+			<param index="0" name="enabled" type="bool" />
+			<description>
+				Sets whether the thread safety checks the engine normally performs in methods of certain classes (e.g., [Node]) should happen [b]on the current thread[/b].
+				The default, for every thread, is that they are enabled (as if called with [param enabled] being [code]true[/code]).
+				Those checks are conservative. That means that they will only succeed in considering a call thread-safe (and therefore allow it to happen) if the engine can guarantee such safety.
+				Because of that, there may be cases where the user may want to disable them ([param enabled] being [code]false[/code]) to make certain operations allowed again. By doing so, it becomes the user's responsibility to ensure thread safety (e.g., by using [Mutex]) for those objects that are otherwise protected by the engine.
+				[b]Note:[/b] This is an advanced usage of the engine. You are advised to use it only if you know what you are doing and there is no safer way.
+				[b]Note:[/b] This is useful for scripts running on either arbitrary [Thread] objects or tasks submitted to the [WorkerThreadPool]. It doesn't apply to code running during [Node] group processing, where the checks will be always performed.
+				[b]Note:[/b] Even in the case of having disabled the checks in a [WorkerThreadPool] task, there's no need to re-enable them at the end. The engine will do so.
+			</description>
+		</method>
 		<method name="start">
 			<return type="int" enum="Error" />
 			<param index="0" name="callable" type="Callable" />

diff --git a/scene/gui/rich_text_label.cpp b/scene/gui/rich_text_label.cpp
@@ -2722,7 +2722,6 @@ void RichTextLabel::_thread_function(void *p_userdata) {
 	_process_line_caches();
 	updating.store(false);
 	call_deferred(SNAME("thread_end"));
-	set_current_thread_safe_for_nodes(false);
 }
 
 void RichTextLabel::_thread_end() {
-Original file line number
+Diff line change
@@ Expand Up @@
     	if (load_nesting == 0 && mq_override) {
     		memdelete(mq_override);
-    		set_current_thread_safe_for_nodes(false);
     	}
     }
@@ Expand Down @@