Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix some race conditions that happen during initialization #73793

Merged
merged 1 commit into from
Mar 6, 2023
Merged

Fix some race conditions that happen during initialization #73793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions core/io/ip.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ struct _IP_ResolverPrivate {
Semaphore sem;

Thread thread;
bool thread_abort = false;
SafeFlag thread_abort;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


void resolve_queues() {
for (int i = 0; i < IP::RESOLVER_MAX_QUERIES; i++) {
Expand Down Expand Up @@ -111,7 +111,7 @@ struct _IP_ResolverPrivate {
static void _thread_function(void *self) {
_IP_ResolverPrivate *ipr = static_cast<_IP_ResolverPrivate *>(self);

while (!ipr->thread_abort) {
while (!ipr->thread_abort.is_set()) {
ipr->sem.wait();
ipr->resolve_queues();
}
Expand Down Expand Up @@ -343,12 +343,12 @@ IP::IP() {
singleton = this;
resolver = memnew(_IP_ResolverPrivate);

resolver->thread_abort = false;
resolver->thread_abort.clear();
resolver->thread.start(_IP_ResolverPrivate::_thread_function, resolver);
}

IP::~IP() {
resolver->thread_abort = true;
resolver->thread_abort.set();
resolver->sem.post();
resolver->thread.wait_to_finish();

Expand Down
6 changes: 3 additions & 3 deletions core/os/thread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🥲

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Thread::caller_id = p_caller_id;
if (platform_functions.set_priority) {
platform_functions.set_priority(p_settings.priority);
}
Expand Down Expand Up @@ -79,7 +79,7 @@ void Thread::start(Thread::Callback p_callback, void *p_user, const Settings &p_
std::thread empty_thread;
thread.swap(empty_thread);
}
std::thread new_thread(&Thread::callback, this, p_settings, p_callback, p_user);
std::thread new_thread(&Thread::callback, _thread_id_hash(thread.get_id()), p_settings, p_callback, p_user);
thread.swap(new_thread);
id = _thread_id_hash(thread.get_id());
}
Expand Down
2 changes: 1 addition & 1 deletion core/os/thread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class Thread {
static thread_local ID caller_id;
std::thread thread;

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

static PlatformFunctions platform_functions;

Expand Down