Fix some race conditions that happen during initialization #73793

myaaaaaaaaa · 2023-02-23T00:22:34Z

Needed for #73777

RandomShaper · 2023-02-23T07:57:04Z

core/os/thread.cpp

@@ -50,8 +50,8 @@ void Thread::_set_platform_functions(const PlatformFunctions &p_functions) {
 	platform_functions = p_functions;
 }

-void Thread::callback(Thread *p_self, const Settings &p_settings, Callback p_callback, void *p_userdata) {
-	Thread::caller_id = _thread_id_hash(p_self->thread.get_id());
+void Thread::callback(ID p_caller_id, const Settings &p_settings, Callback p_callback, void *p_userdata) {


I'm curious about why this is needed. I mean, if the pointer was only needed for getting the id, this new version seems to be safer. However, in practice Thread object shouldn't be destroyed before the thread ends.

There's a call to thread.swap(new_thread) right after new_thread gets created, so in the old version, it's ambiguous whether p_self->thread refers to the old thread or the just created new_thread.

Threading is hard 🥲

Besides swap, Thread technically can be destroyed (and will detach thread in this case) before the thread is finished. So it is safer.

RandomShaper · 2023-02-23T08:07:02Z

core/io/ip.cpp

@@ -75,7 +75,7 @@ struct _IP_ResolverPrivate {
 	Semaphore sem;

 	Thread thread;
-	bool thread_abort = false;
+	SafeFlag thread_abort;


The variable is synced with the semaphore. However, I see now that a data race is still possible, if bool doesn't provide atomicity guarantees across platforms. Therefore, thinking about this (and similar cases), my abandoned RelaxedFlag project is relevant (#63189). That one asks the implementation for atomicity, but without acquire-release semantics, which the semaphore is already providing.

Thoughts?

P.S.: What was the sanitizer complaint, by the way?

It was a standard read/write complaint, it seems like all variable accesses (even to scalars like int and bool) are considered data races unless they're explicitly specified as atomic.

Providing it is an IP hostname resolver, it's not performance critical and any overhead for SafeFlag going to be negligible compared to networking.

I was thinking about this along the lines of having an idiom for this kind of cases; namely, every time there's a semaphore-paced loop with an exit flag, just use the atomic-but-relaxed version of the flag, without having to pay further attention to the performance considerations.

That said, that can be discussed separately. These changes already have their merit as they are now.

bruvzg

Unlikely any of these will cause a real issue, but changes seems good.

bruvzg · 2023-02-23T08:34:39Z

core/io/ip.cpp

@@ -75,7 +75,7 @@ struct _IP_ResolverPrivate {
 	Semaphore sem;

 	Thread thread;
-	bool thread_abort = false;
+	SafeFlag thread_abort;


Providing it is an IP hostname resolver, it's not performance critical and any overhead for SafeFlag going to be negligible compared to networking.

bruvzg · 2023-02-23T08:39:21Z

core/os/thread.cpp

@@ -50,8 +50,8 @@ void Thread::_set_platform_functions(const PlatformFunctions &p_functions) {
 	platform_functions = p_functions;
 }

-void Thread::callback(Thread *p_self, const Settings &p_settings, Callback p_callback, void *p_userdata) {
-	Thread::caller_id = _thread_id_hash(p_self->thread.get_id());
+void Thread::callback(ID p_caller_id, const Settings &p_settings, Callback p_callback, void *p_userdata) {


Besides swap, Thread technically can be destroyed (and will detach thread in this case) before the thread is finished. So it is safer.

myaaaaaaaaa · 2023-02-24T00:51:53Z

This should be the last one involving initialization. Should I open a new PR or add it to this one?

diff --git a/core/os/thread.cpp b/core/os/thread.cpp
index 92865576f3..d533b1c1e2 100644
--- a/core/os/thread.cpp
+++ b/core/os/thread.cpp
@@ -58,13 +58,19 @@ void Thread::callback(ID p_caller_id, const Settings &p_settings, Callback p_cal
 	if (platform_functions.init) {
 		platform_functions.init();
 	}
-	ScriptServer::thread_enter(); // Scripts may need to attach a stack.
+
+	// Currently causes a race condition due to the fact that threads are
+	// spawned before ScriptServer is even initialized.
+	// This is currently unused since neither GDScript nor C# require
+	// attaching a stack to new threads, so we just disable for now.
+
+	//ScriptServer::thread_enter();
 	if (platform_functions.wrapper) {
 		platform_functions.wrapper(p_callback, p_userdata);
 	} else {
 		p_callback(p_userdata);
 	}
-	ScriptServer::thread_exit();
+	//ScriptServer::thread_exit();
 	if (platform_functions.term) {
 		platform_functions.term();
 	}

All of the possible workarounds for this one get ugly really fast since ScriptServer isn't always initialized, and any changes to main() that can fix this should probably be centralized in #49362 given the high likelihood of breaking something.

In the meantime, disabling thread_enter/exit() means that ThreadSanitizer can go into CI sooner, which would also aid debugging in case a new language that does need it is ever added.

RandomShaper · 2023-02-24T07:02:20Z

If no scripting language is using thread enter/exit callbacks (and there are no near future plans for it), I'd even remove them.

However, I'm a bit unsure about the needs that language extensions developed separately may have (JS, Nim, Rust, whatever).

I am wondering if maybe we could somehow apply a compromise solution that patches the current situation to keep it functional and non-racey, even if somewhat hacky. I'm considering something like this:

Add a Mutex/SpinLock to ScriptServer so that _language_count and _languages are treated atomically.
On thread_enter(), take a snapshot of the language array and count in TLS.
On thread_exit() notify only the languages which are in both the current languages array and the TLS copy.
Additional thoughts:
- Comparing current vs. snapshot languages by pointer would not be reliable; a possibility to workaround that would be that unregistered languages just leave a null entry in the array so they can be compared by index, or that the ScriptLanguage classes get some sort of serial number that lets differentiate among instances.
- Maybe Vector(), with its COW semantics, can somehow make the new code easier?

myaaaaaaaaa · 2023-02-24T15:14:47Z

Comparing current vs. snapshot languages by pointer would not be reliable; a possibility to workaround that would be that unregistered languages just leave a null entry in the array so they can be compared by index, or that the ScriptLanguage classes get some sort of serial number that lets differentiate among instances.

Maybe Vector(), with its COW semantics, can somehow make the new code easier?

Changing _languages into an array of Ref<ScriptLanguage>s and making a TLS copy in thread_enter() would probably be the cleanest way to follow through with this approach.

Unfortunately, it wouldn't help the main victim of this race condition, WorkerThreadPool. Since WorkerThreadPool threads are initialized early and live for the entire duration of the program, unlucky initializations could result in all uses of WorkerThreadPool not having a stack.

I don't know if there's any decision that can be taken here that doesn't leave something broken, including just leaving it as-is 🤷

core/os/thread.cpp

myaaaaaaaaa · 2023-03-06T17:24:29Z

The ScriptServer workaround has been split off into #74501

akien-mga · 2023-03-06T19:07:08Z

Thanks!

YuriSizov · 2023-03-27T14:44:16Z

Cherry-picked for 4.0.2.

myaaaaaaaaa marked this pull request as ready for review February 23, 2023 00:22

myaaaaaaaaa requested a review from a team as a code owner February 23, 2023 00:22

myaaaaaaaaa changed the title ~~Fix data races in startup/teardown~~ Fix some race conditions that happen during initialization Feb 23, 2023

akien-mga requested a review from RandomShaper February 23, 2023 07:44

akien-mga added bug topic:core labels Feb 23, 2023

akien-mga added this to the 4.1 milestone Feb 23, 2023

akien-mga added the cherrypick:4.0 label Feb 23, 2023

RandomShaper reviewed Feb 23, 2023

View reviewed changes

bruvzg reviewed Feb 23, 2023

View reviewed changes

RandomShaper approved these changes Feb 23, 2023

View reviewed changes

myaaaaaaaaa mentioned this pull request Feb 24, 2023

Add a Linux ThreadSanitizer job to CI #73777

Merged

akien-mga reviewed Mar 2, 2023

View reviewed changes

core/os/thread.cpp Outdated Show resolved Hide resolved

Fix data races in startup/teardown

d337ed1

myaaaaaaaaa mentioned this pull request Mar 6, 2023

Work around a race condition with ScriptServer during initialization #74501

Closed

RandomShaper approved these changes Mar 6, 2023

View reviewed changes

akien-mga merged commit e80ab42 into godotengine:master Mar 6, 2023

myaaaaaaaaa deleted the init-race branch March 6, 2023 20:11

YuriSizov removed the cherrypick:4.0 label Mar 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some race conditions that happen during initialization #73793

Fix some race conditions that happen during initialization #73793

myaaaaaaaaa commented Feb 23, 2023

RandomShaper Feb 23, 2023

myaaaaaaaaa Feb 23, 2023

bruvzg Feb 23, 2023

RandomShaper Feb 23, 2023

myaaaaaaaaa Feb 23, 2023

bruvzg Feb 23, 2023

RandomShaper Feb 23, 2023

bruvzg left a comment

bruvzg Feb 23, 2023

bruvzg Feb 23, 2023

myaaaaaaaaa commented Feb 24, 2023

RandomShaper commented Feb 24, 2023

myaaaaaaaaa commented Feb 24, 2023 •

edited

Loading

myaaaaaaaaa commented Mar 6, 2023

akien-mga commented Mar 6, 2023

YuriSizov commented Mar 27, 2023

Fix some race conditions that happen during initialization #73793

Fix some race conditions that happen during initialization #73793

Conversation

myaaaaaaaaa commented Feb 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruvzg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myaaaaaaaaa commented Feb 24, 2023

RandomShaper commented Feb 24, 2023

myaaaaaaaaa commented Feb 24, 2023 • edited Loading

myaaaaaaaaa commented Mar 6, 2023

akien-mga commented Mar 6, 2023

YuriSizov commented Mar 27, 2023

myaaaaaaaaa commented Feb 24, 2023 •

edited

Loading