Skip to content

Commit

Permalink
Fix thread IDs.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
reduz committed Apr 24, 2023
1 parent 24cb43a commit 61b3cde
Show file tree
Hide file tree
Showing 7 changed files with 47 additions and 42 deletions.
4 changes: 3 additions & 1 deletion core/debugger/remote_debugger_peer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
39 changes: 13 additions & 26 deletions core/os/thread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand All @@ -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
Expand Down
24 changes: 17 additions & 7 deletions core/os/thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ class Thread {

typedef uint64_t ID;

enum {
UNASSIGNED_ID = 0,
MAIN_ID = 1
};

enum Priority {
PRIORITY_LOW,
PRIORITY_NORMAL,
Expand All @@ -74,26 +79,31 @@ class Thread {
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;

static void callback(ID p_caller_id, const Settings &p_settings, Thread::Callback p_callback, void *p_userdata);

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);

Expand Down
14 changes: 9 additions & 5 deletions main/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -1941,7 +1947,9 @@ Error Main::setup(const char *execpath, int argc, char *argv[], bool p_second_ph
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));

Expand All @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion main/main.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class Main {

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();
Expand Down
4 changes: 3 additions & 1 deletion platform/android/export/export_plugin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
2 changes: 1 addition & 1 deletion platform/android/java_godot_lib_jni.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit 61b3cde

Please sign in to comment.