godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Add annotation for thread safe variables #6213

Open atirut-w opened 1 year ago

atirut-w commented 1 year ago

Describe the project you are working on

None

Describe the problem or limitation you are having in your project

While mutexes can help you, you still have to be very mindful about what you access and when. This can potentially become a problem when, for example, a threaded function wants to access something from a singleton(happened to me a while ago).

With threads already being pretty hard to debug(at least in Godot 4 betas), I think this warrants some sort of system to ensure a accessing a select variable will never cause problems related to race condition.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

This helps prevent errors and problems related to race condition at the cost of some potential overhead when accessing variables annotated with the (as of yet unnamed) annotation. A good example is when you access a global variable.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

When the annoation is added to a variable, the variable will have a mutex associated with it. The mutex will be locked and unlocked automatically when being accessed from a thread.

If this enhancement will not be used often, can it be worked around with a few lines of script?

A wrapper class can probably work, at the cost of sacrificing typed variables.

Is there a reason why this should be core and not an add-on in the asset library?

Convenience.

SlugFiller commented 1 year ago

It sounds like the actual problem you have in your project is poor threaded design. A similar concern/method shouldn't be getting executed from multiple threads. Threads should only be spawned as "work pieces", either working in tandem/parallel on a single data set, or doing some isolated work on a dataset that is fully contained within the thread.

When a thread's work unit is done, it shouldn't pass it on to a handler inside its own execution context, it should pass it to a single-threaded event loop. Here I will admit that the Godot thread API is sorely lacking in safe cross-thread communication primitives. It would be sufficient for a thread to be an emitter for a "finished" signal which can be yielded on the main thread. For more advanced usage, a thread-safe communication channel would be appropriate.

My point is that you shouldn't even be reaching a situation where two different threads are using the same singleton. It's usually a sign that you offloaded to a thread something that shouldn't have been. While Godot can use extra threading tools, they should generally be ones that encourage a correct workflow. Your suggestion is an invitation for deadlocks.

P.S. It sounds like what you actually want is atomic/volatile variables. If you lock and unlock on every access, then you don't actually get an atomic action. For example

volatile_x = volatile_x +1

being translated to

mutex_lock()
var temp = volatile_x
mutex_unlock()
mutex_lock()
volatile_x = temp+1
mutex_unlock()

would actually remove the whole point of having a mutex. What qualifies as "access" of a variable is actually significantly less than what a programmer may be thinking of as a single operation. That's why true atomics add specific operations for it:

atomic_x.atomic_add(1)

Still, you most likely do NOT need atomics, and are simply using incorrect design. The correct way to avoid race conditions is to have a clear concept of which thread is using which resource, and keep it that way.

Incidentally, this is also the design behind Godot's servers. Each server runs in a different thread, but is responsible for its own resources, which are not shared with the other servers. As a result a server's resources do not need individual mutexs. A single one for the whole server is sufficient.