`Vector`'s constructor fails to create a reference to the other `Vector` when the other thread writes to that object

kus04e4ek commented 3 months ago

Tested versions

Reproducible in: v3.0 [362774c6176596f87506e3d330dd1d12b0b75e19], master [475248d99df89fc29032a42f1d29ad4cef49c8b5]

System information

Ubuntu 22.04.4 LTS 22.04

Issue description

Causes: #86428. Decided to create another issue as this issue is broader. Can be transformed into a tracker of similar issues.

Vector's constructor fails to create a reference to the other Vector when the other thread writes to that object. Not sure if it's intentional and Vector is just not meant to be thread-safe.

Steps to reproduce

Store an Vector object. Write to it from one thread and read from the other at the same time.

Minimal reproduction project (MRP)

3.0 (run with godot --test thread)

```diff diff --git a/main/tests/test_main.cpp b/main/tests/test_main.cpp index 3d5ca70c439..3dd292d9362 100644 --- a/main/tests/test_main.cpp +++ b/main/tests/test_main.cpp @@ -45,6 +45,7 @@ #include "test_render.h" #include "test_shader_lang.h" #include "test_string.h" +#include "test_thread.h" const char **tests_get_names() { @@ -72,6 +73,11 @@ const char **tests_get_names() { MainLoop *test_main(String p_test, const List &p_args) { + if (p_test == "thread") { + + return TestThread::test(); + } + if (p_test == "string") { return TestString::test(); diff --git a/main/tests/test_thread.cpp b/main/tests/test_thread.cpp new file mode 100644 index 00000000000..c19d066dd81 --- /dev/null +++ b/main/tests/test_thread.cpp @@ -0,0 +1,43 @@ +#include "test_thread.h" + +#include "core/os/os.h" +#include "core/os/thread.h" + +#include + +namespace TestThread { + +std::atomic test_exit; +Vector test_vec; + +void test_func(void*) { + print_line("Running thread"); + while (!test_exit) { + for (int i = 0; i < test_vec.size(); i++) { + test_vec[i] = 0; + } + OS::get_singleton()->delay_usec(1000); + } +} + +MainLoop *test() { + test_exit.store(false); + test_vec.resize(1000); + + Thread* thread = Thread::create(test_func, nullptr); + + for (int i = 0; i < 1000000; i++) { + Vector val = test_vec; + if (val.ptr() == nullptr) { + print_line("test_vec has nullptr pointer"); + break; + } + + OS::get_singleton()->delay_usec(1000); + } + + test_exit.store(true); + Thread::wait_to_finish(thread); + return nullptr; +} +} // namespace TestThread diff --git a/main/tests/test_thread.h b/main/tests/test_thread.h new file mode 100644 index 00000000000..58d53c82e06 --- /dev/null +++ b/main/tests/test_thread.h @@ -0,0 +1,11 @@ +#ifndef TEST_THREAD_H +#define TEST_THREAD_H + +#include "os/main_loop.h" + +namespace TestThread { + +MainLoop *test(); +} + +#endif ```

master (run with godot --test --test-case="*[Thread]*")

```diff diff --git a/tests/test_main.cpp b/tests/test_main.cpp index 3c875797a49..2b7a60f6fa5 100644 --- a/tests/test_main.cpp +++ b/tests/test_main.cpp @@ -126,6 +126,7 @@ #include "tests/scene/test_window.h" #include "tests/servers/rendering/test_shader_preprocessor.h" #include "tests/servers/test_text_server.h" +#include "tests/test_thread.h" #include "tests/test_validate_testing.h" #ifndef _3D_DISABLED diff --git a/tests/test_thread.h b/tests/test_thread.h new file mode 100644 index 00000000000..f25a61ddc67 --- /dev/null +++ b/tests/test_thread.h @@ -0,0 +1,74 @@ +/**************************************************************************/ +/* test_thread.h */ +/**************************************************************************/ +/* This file is part of: */ +/* GODOT ENGINE */ +/* https://godotengine.org */ +/**************************************************************************/ +/* Copyright (c) 2014-present Godot Engine contributors (see AUTHORS.md). */ +/* Copyright (c) 2007-2014 Juan Linietsky, Ariel Manzur. */ +/* */ +/* Permission is hereby granted, free of charge, to any person obtaining */ +/* a copy of this software and associated documentation files (the */ +/* "Software"), to deal in the Software without restriction, including */ +/* without limitation the rights to use, copy, modify, merge, publish, */ +/* distribute, sublicense, and/or sell copies of the Software, and to */ +/* permit persons to whom the Software is furnished to do so, subject to */ +/* the following conditions: */ +/* */ +/* The above copyright notice and this permission notice shall be */ +/* included in all copies or substantial portions of the Software. */ +/* */ +/* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, */ +/* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF */ +/* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. */ +/* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY */ +/* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, */ +/* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE */ +/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ +/**************************************************************************/ + +#ifndef TEST_THREAD_H +#define TEST_THREAD_H + +#include "tests/test_macros.h" + +#include "core/os/os.h" +#include "core/os/thread.h" + +namespace TestThread { + +std::atomic test_exit; +Vector test_vec; + +void test_func(void *) { + print_line("Running thread"); + while (!test_exit) { + for (int i = 0; i < test_vec.size(); i++) { + test_vec.write[i] = 0; + } + OS::get_singleton()->delay_usec(1000); + } +} + +TEST_CASE("[Thread] Example test case") { + test_exit.store(false); + test_vec.resize(1000); + + Thread thread; + thread.start(test_func, nullptr); + + for (int i = 0; i < 1000000; i++) { + Vector val = test_vec; + WARN(val.ptr() != nullptr); + + OS::get_singleton()->delay_usec(1000); + } + + test_exit.store(true); + thread.wait_to_finish(); +} + +} // namespace TestThread + +#endif // TEST_THREAD_H ```

master with CowData (run with godot --test --test-case="*[Thread]*")

```diff diff --git a/tests/test_main.cpp b/tests/test_main.cpp index 3c875797a49..2b7a60f6fa5 100644 --- a/tests/test_main.cpp +++ b/tests/test_main.cpp @@ -126,6 +126,7 @@ #include "tests/scene/test_window.h" #include "tests/servers/rendering/test_shader_preprocessor.h" #include "tests/servers/test_text_server.h" +#include "tests/test_thread.h" #include "tests/test_validate_testing.h" #ifndef _3D_DISABLED diff --git a/tests/test_thread.h b/tests/test_thread.h new file mode 100644 index 00000000000..7f5800caa6b --- /dev/null +++ b/tests/test_thread.h @@ -0,0 +1,74 @@ +/**************************************************************************/ +/* test_thread.h */ +/**************************************************************************/ +/* This file is part of: */ +/* GODOT ENGINE */ +/* https://godotengine.org */ +/**************************************************************************/ +/* Copyright (c) 2014-present Godot Engine contributors (see AUTHORS.md). */ +/* Copyright (c) 2007-2014 Juan Linietsky, Ariel Manzur. */ +/* */ +/* Permission is hereby granted, free of charge, to any person obtaining */ +/* a copy of this software and associated documentation files (the */ +/* "Software"), to deal in the Software without restriction, including */ +/* without limitation the rights to use, copy, modify, merge, publish, */ +/* distribute, sublicense, and/or sell copies of the Software, and to */ +/* permit persons to whom the Software is furnished to do so, subject to */ +/* the following conditions: */ +/* */ +/* The above copyright notice and this permission notice shall be */ +/* included in all copies or substantial portions of the Software. */ +/* */ +/* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, */ +/* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF */ +/* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. */ +/* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY */ +/* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, */ +/* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE */ +/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ +/**************************************************************************/ + +#ifndef TEST_THREAD_H +#define TEST_THREAD_H + +#include "tests/test_macros.h" + +#include "core/os/os.h" +#include "core/os/thread.h" + +namespace TestThread { + +std::atomic test_exit; +CowData test_vec; + +void test_func(void *) { + print_line("Running thread"); + while (!test_exit) { + for (int i = 0; i < test_vec.size(); i++) { + test_vec.ptrw()[i] = 0; + } + OS::get_singleton()->delay_usec(1000); + } +} + +TEST_CASE("[Thread] Example test case") { + test_exit.store(false); + test_vec.resize(1000); + + Thread thread; + thread.start(test_func, nullptr); + + for (int i = 0; i < 1000000; i++) { + CowData val = test_vec; + WARN(val.ptr() != nullptr); + + OS::get_singleton()->delay_usec(1000); + } + + test_exit.store(true); + thread.wait_to_finish(); +} + +} // namespace TestThread + +#endif // TEST_THREAD_H ```

AThousandShips commented 3 months ago

Vector is copy-on-write, meaning it doesn't share with other modified instances of Vector, but to save on memory it shares until one is modified, this is fully thread safe, and ptr is nullptr if the vector is empty, see here (the page in general is outdated but this part is up-to-date)

Will test this specific case when I can

AThousandShips commented 3 months ago

I can confirm and it looks like a race condition somewhere, it might be hard to pin down and fix, and unsure how reasonably likely this is to occur under normal conditions

CC @RandomShaper

kus04e4ek commented 3 months ago

Unsure how reasonably likely this is to occur under normal conditions

It does occur here: #86428. This can only be prevented if we ensure that reading and writing never happen at the same time, so it's not that hard to work around in this particular issue, something like this can be randomly happening in other places though

AThousandShips commented 3 months ago

That sounds like general thread safety being applied, so it can be avoided with fundamental thread safety in code, which should be the case IMO, but finding some potential race condition here is good to work out as well, pinning down the specifics here

AThousandShips commented 3 months ago

Though in this case it could also indicate that the code in question uses Vector inefficiently, I'll take a look at that PR because it might be that that code does too much reading and writing in a way that increases the risk of collisions by not working efficiently as well

For example prefer to use ptrw over a loop with write in it

In this particular case I'd say it seems like the use of Vector is not optimal, but since we need to return a Vector it's a bit unavoidable, but it should really be a LocalVector for data management if shared

kus04e4ek commented 3 months ago

In this particular case I'd say it seems like the use of Vector is not optimal, but since we need to return a Vector it's a bit unavoidable, but it should really be a LocalVector for data management if shared

We don't really need to return a Vector as this method is unexposed and can be just const LocalVector&or get_value(int index), I would like to keep it still compatible with GDScript though as AudioDriver might be exposed in the future (I mean it would make sense since there're plans to expose OS and RenderingDriver)

AThousandShips commented 3 months ago

Let's not limit ourselves by forward planning, so I'd say to try with LocalVector and see if it helps

But regardless the fetching of the data should be done thread safely and I'm not sure the specific issue is necessarily due to the thread safety of Vector itself but due to unsafe usage generally, you shouldn't rely on atomicity IMO for this kind of producer/consumer setup

kus04e4ek commented 3 months ago

Let's not limit ourselves by forward planning, so I'd say to try with LocalVector and see if it helps

I tried with const Vector& and it worked, but I do agree that LocalVector would be better while it's internal

But regardless the fetching of the data should be done thread safely and I'm not sure the specific issue is necessarily due to the thread safety of Vector itself but due to unsafe usage generally

I would say it's worth it to make it unsafe for speed (there could be audio issues if we make it wait too long as data is being processed in real-time), there are no issues with reading values as it's writing data to places that we already read, it wouldn't be that hard to just make buffer bigger if any issues are discovered.

We kinda got off-topic though, we should probably move to the PR: #92969.

AThousandShips commented 3 months ago

So to summarize what I mean:

There's some kind of race condition occurring in Vector that should be investigated
This code shouldn't be affected by this as it should be thread safe regardless when doing producer/consumer
The code would be better to use with LocalVector for performance reasons alone

So agreed, just signing off with a summary and let's focus on the thread issue here and the implementation on the PR, I'd suggest LocalVector for that case probably but haven't studied the specific details, someone can give their thoughts there as well

godotengine / godot