`Vector`'s constructor fails to create a reference to the other `Vector` when the other thread writes to that object about godot HOT 9 OPEN

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

from godot.

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

from godot.

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

from godot.

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

from godot.

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.

from godot.

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

from godot.

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

from godot.

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

from godot.

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)

from godot.