summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorJuan Linietsky <reduzio@gmail.com>2023-04-22 15:34:16 +0200
committerJuan Linietsky <reduzio@gmail.com>2023-04-24 15:13:58 +0200
commita37c30dfc92d98d79c4a315c58ecb5b2adabf97a (patch)
treec0a91be57bf01b77c5b0aa090233c63f9135eab1
parent24cb43a8741c7b10abbbbc77bb6e2bc188662ce0 (diff)
downloadredot-engine-a37c30dfc92d98d79c4a315c58ecb5b2adabf97a.tar.gz
Fix thread IDs.
On Linux, thread IDs were not properly assigned with the current approach. The line: `std::thread new_thread(&Thread::callback, _thread_id_hash(thread.get_id()), p_settings, p_callback, p_user);` does not work because the thread ID is not assigned until the thread starts. This PR changes the behavior to use manually generated thread IDs. Additionally, if a thread is (or may have been created) outside Godot, the method `Thread::attach_external_thread` was added.
-rw-r--r--core/debugger/remote_debugger_peer.cpp4
-rw-r--r--core/os/thread.cpp39
-rw-r--r--core/os/thread.h24
-rw-r--r--main/main.cpp14
-rw-r--r--main/main.h2
-rw-r--r--platform/android/export/export_plugin.cpp4
-rw-r--r--platform/android/java_godot_lib_jni.cpp2
7 files changed, 47 insertions, 42 deletions
diff --git a/core/debugger/remote_debugger_peer.cpp b/core/debugger/remote_debugger_peer.cpp
index f82600a9a2..81ee09f515 100644
--- a/core/debugger/remote_debugger_peer.cpp
+++ b/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();
diff --git a/core/os/thread.cpp b/core/os/thread.cpp
index 92865576f3..502f82aaef 100644
--- a/core/os/thread.cpp
+++ b/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
diff --git a/core/os/thread.h b/core/os/thread.h
index 6eb21fba65..19e1376ca8 100644
--- a/core/os/thread.h
+++ b/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);
diff --git a/main/main.cpp b/main/main.cpp
index bc309219f4..6b89d96147 100644
--- a/main/main.cpp
+++ b/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();
diff --git a/main/main.h b/main/main.h
index e345589f59..cc0655cd02 100644
--- a/main/main.h
+++ b/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();
diff --git a/platform/android/export/export_plugin.cpp b/platform/android/export/export_plugin.cpp
index 0f0132a5d1..f52edf2b61 100644
--- a/platform/android/export/export_plugin.cpp
+++ b/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
}
diff --git a/platform/android/java_godot_lib_jni.cpp b/platform/android/java_godot_lib_jni.cpp
index 1a0087e18d..f32617e674 100644
--- a/platform/android/java_godot_lib_jni.cpp
+++ b/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;