Merge pull request #76345 from reduz/fix-thread-ids

Fix thread IDs.

core/debugger/remote_debugger_peer.cpp

@@ -66,7 +66,9 @@ int RemoteDebuggerPeerTCP::get_max_message_size() const {
 
 void RemoteDebuggerPeerTCP::close() {
 	running = false;
-	thread.wait_to_finish();
+	if (thread.is_started()) {
+		thread.wait_to_finish();
+	}
 	tcp_client->disconnect_from_host();
 	out_buf.clear();
 	in_buf.clear();

core/os/thread.cpp

@@ -38,13 +38,9 @@
 
 Thread::PlatformFunctions Thread::platform_functions;
 
-uint64_t Thread::_thread_id_hash(const std::thread::id &p_t) {
-	static std::hash<std::thread::id> hasher;
-	return hasher(p_t);
-}
+SafeNumeric<uint64_t> Thread::id_counter(1); // The first value after .increment() is 2, hence by default the main thread ID should be 1.
 
-Thread::ID Thread::main_thread_id = _thread_id_hash(std::this_thread::get_id());
-thread_local Thread::ID Thread::caller_id = 0;
+thread_local Thread::ID Thread::caller_id = Thread::UNASSIGNED_ID;
 
 void Thread::_set_platform_functions(const PlatformFunctions &p_functions) {
 	platform_functions = p_functions;
@@ -71,31 +67,23 @@ void Thread::callback(ID p_caller_id, const Settings &p_settings, Callback p_cal
 }
 
 void Thread::start(Thread::Callback p_callback, void *p_user, const Settings &p_settings) {
-	if (id != _thread_id_hash(std::thread::id())) {
-#ifdef DEBUG_ENABLED
-		WARN_PRINT("A Thread object has been re-started without wait_to_finish() having been called on it. Please do so to ensure correct cleanup of the thread.");
-#endif
-		thread.detach();
-		std::thread empty_thread;
-		thread.swap(empty_thread);
-	}
-	std::thread new_thread(&Thread::callback, _thread_id_hash(thread.get_id()), p_settings, p_callback, p_user);
+	ERR_FAIL_COND_MSG(id != UNASSIGNED_ID, "A Thread object has been re-started without wait_to_finish() having been called on it.");
+	id = id_counter.increment();
+	std::thread new_thread(&Thread::callback, id, p_settings, p_callback, p_user);
 	thread.swap(new_thread);
-	id = _thread_id_hash(thread.get_id());
 }
 
 bool Thread::is_started() const {
-	return id != _thread_id_hash(std::thread::id());
+	return id != UNASSIGNED_ID;
 }
 
 void Thread::wait_to_finish() {
-	if (id != _thread_id_hash(std::thread::id())) {
-		ERR_FAIL_COND_MSG(id == get_caller_id(), "A Thread can't wait for itself to finish.");
-		thread.join();
-		std::thread empty_thread;
-		thread.swap(empty_thread);
-		id = _thread_id_hash(std::thread::id());
-	}
+	ERR_FAIL_COND_MSG(id == UNASSIGNED_ID, "Attempt of waiting to finish on a thread that was never started.");
+	ERR_FAIL_COND_MSG(id == get_caller_id(), "Threads can't wait to finish on themselves, another thread must wait.");
+	thread.join();
+	std::thread empty_thread;
+	thread.swap(empty_thread);
+	id = UNASSIGNED_ID;
 }
 
 Error Thread::set_name(const String &p_name) {
@@ -107,11 +95,10 @@ Error Thread::set_name(const String &p_name) {
 }
 
 Thread::Thread() {
-	caller_id = _thread_id_hash(std::this_thread::get_id());
 }
 
 Thread::~Thread() {
-	if (id != _thread_id_hash(std::thread::id())) {
+	if (id != UNASSIGNED_ID) {
 #ifdef DEBUG_ENABLED
 		WARN_PRINT("A Thread object has been destroyed without wait_to_finish() having been called on it. Please do so to ensure correct cleanup of the thread.");
 #endif

core/os/thread.h

@@ -52,6 +52,11 @@ public:
 
 	typedef uint64_t ID;
 
+	enum : ID {
+		UNASSIGNED_ID = 0,
+		MAIN_ID = 1
+	};
+
 	enum Priority {
 		PRIORITY_LOW,
 		PRIORITY_NORMAL,
@@ -74,11 +79,8 @@ public:
 private:
 	friend class Main;
 
-	static ID main_thread_id;
-
-	static uint64_t _thread_id_hash(const std::thread::id &p_t);
-
-	ID id = _thread_id_hash(std::thread::id());
+	ID id = UNASSIGNED_ID;
+	static SafeNumeric<uint64_t> id_counter;
 	static thread_local ID caller_id;
 	std::thread thread;
 
@@ -86,14 +88,22 @@ private:
 
 	static PlatformFunctions platform_functions;
 
+	static void make_main_thread() { caller_id = MAIN_ID; }
+	static void release_main_thread() { caller_id = UNASSIGNED_ID; }
+
 public:
 	static void _set_platform_functions(const PlatformFunctions &p_functions);
 
 	_FORCE_INLINE_ ID get_id() const { return id; }
 	// get the ID of the caller thread
-	_FORCE_INLINE_ static ID get_caller_id() { return caller_id; }
+	_FORCE_INLINE_ static ID get_caller_id() {
+		if (unlikely(caller_id == UNASSIGNED_ID)) {
+			caller_id = id_counter.increment();
+		}
+		return caller_id;
+	}
 	// get the ID of the main thread
-	_FORCE_INLINE_ static ID get_main_id() { return main_thread_id; }
+	_FORCE_INLINE_ static ID get_main_id() { return MAIN_ID; }
 
 	static Error set_name(const String &p_name);
 

main/main.cpp

@@ -471,6 +471,8 @@ void Main::print_help(const char *p_binary) {
 // The order is the same as in `Main::setup()`, only core and some editor types
 // are initialized here. This also combines `Main::setup2()` initialization.
 Error Main::test_setup() {
+	Thread::make_main_thread();
+
 	OS::get_singleton()->initialize();
 
 	engine = memnew(Engine);
@@ -680,6 +682,8 @@ int Main::test_entrypoint(int argc, char *argv[], bool &tests_need_run) {
  */
 
 Error Main::setup(const char *execpath, int argc, char *argv[], bool p_second_phase) {
+	Thread::make_main_thread();
+
 	OS::get_singleton()->initialize();
 
 	engine = memnew(Engine);
@@ -1876,6 +1880,8 @@ Error Main::setup(const char *execpath, int argc, char *argv[], bool p_second_ph
 
 	engine->startup_benchmark_end_measure(); // core
 
+	Thread::release_main_thread(); // If setup2() is called from another thread, that one will become main thread, so preventively release this one.
+
 	if (p_second_phase) {
 		return setup2();
 	}
@@ -1941,7 +1947,9 @@ error:
 	return exit_code;
 }
 
-Error Main::setup2(Thread::ID p_main_tid_override) {
+Error Main::setup2() {
+	Thread::make_main_thread(); // Make whatever thread call this the main thread.
+
 	// Print engine name and version
 	print_line(String(VERSION_NAME) + " v" + get_full_version_string() + " - " + String(VERSION_WEBSITE));
 
@@ -1962,10 +1970,6 @@ Error Main::setup2(Thread::ID p_main_tid_override) {
 	initialize_modules(MODULE_INITIALIZATION_LEVEL_SERVERS);
 	GDExtensionManager::get_singleton()->initialize_extensions(GDExtension::INITIALIZATION_LEVEL_SERVERS);
 
-	if (p_main_tid_override) {
-		Thread::main_thread_id = p_main_tid_override;
-	}
-
 #ifdef TOOLS_ENABLED
 	if (editor || project_manager || cmdline_tool) {
 		EditorPaths::create();

main/main.h

@@ -60,7 +60,7 @@ public:
 
 	static int test_entrypoint(int argc, char *argv[], bool &tests_need_run);
 	static Error setup(const char *execpath, int argc, char *argv[], bool p_second_phase = true);
-	static Error setup2(Thread::ID p_main_tid_override = 0);
+	static Error setup2(); // The thread calling setup2() will effectively become the main thread.
 	static String get_rendering_driver_name();
 #ifdef TESTS_ENABLED
 	static Error test_setup();

platform/android/export/export_plugin.cpp

@@ -3306,6 +3306,8 @@ EditorExportPlatformAndroid::EditorExportPlatformAndroid() {
 EditorExportPlatformAndroid::~EditorExportPlatformAndroid() {
 #ifndef ANDROID_ENABLED
 	quit_request.set();
-	check_for_changes_thread.wait_to_finish();
+	if (check_for_changes_thread.is_started()) {
+		check_for_changes_thread.wait_to_finish();
+	}
 #endif
 }

platform/android/java_godot_lib_jni.cpp

@@ -244,7 +244,7 @@ JNIEXPORT jboolean JNICALL Java_org_godotengine_godot_GodotLib_step(JNIEnv *env,
 	if (step.get() == 0) {
 		// Since Godot is initialized on the UI thread, main_thread_id was set to that thread's id,
 		// but for Godot purposes, the main thread is the one running the game loop
-		Main::setup2(Thread::get_caller_id());
+		Main::setup2();
 		input_handler = new AndroidInputHandler();
 		step.increment();
 		return true;