Lock shaped_owner access in TextServerAdvanced. #76585
Conversation
Fixes godotengine#32081. _free_rid takes the THREAD_SAFE_METHOD lock first, then locks FontAdvanced::mutex second. Many functions lock FontAdvanced::mutex first, then call _shaped_text_shape which takes the THREAD_SAFE_METHOD lock second. TSAN flags this as "lock-order-inversion (potential deadlock)". As _shaped_text_shape and _free_rid seem to call THREAD_SAFE_METHOD to guard access to the shaped_owner method, I'm assuming that other methods accessing shaped_owner also ought to take a lock. Doing this ensures that the locks are always taken in a consistent order, avoiding a deadlock between _free_rid and the various _shaped_text_* functions. The full TSAN output can be found at godotengine#32081 (comment).
There was a problem hiding this comment.
This will essentially make TextServer single-threaded only. Providing it's TSAN messages, not an actual issues, I'm not sure if it's a good idea.
Only RID owner (not the whole TS) need to be locked for a moment when new font or text buffer is added and removed (but this in turn will need making internal structs ref counted, since it can be freed if font is reloaded in from another thread). I have attempted to implement it in #72646, but it has some performance impact on a singe threaded use.
If TSAN can hit it, I'm pretty sure it's possible (just less likely) to hit without TSAN, so I think it's an "actual" issue in that sense. It sounds like #72646 is a better approach though, so I'll close in favor of that. |
Fixes #32081.
_free_rid takes the THREAD_SAFE_METHOD lock first, then locks FontAdvanced::mutex second.
Many functions lock FontAdvanced::mutex first, then call _shaped_text_shape which takes the THREAD_SAFE_METHOD lock second.
TSAN flags this as "lock-order-inversion (potential deadlock)".
As _shaped_text_shape and _free_rid seem to call THREAD_SAFE_METHOD to guard access to the shaped_owner method, I'm assuming that other methods accessing shaped_owner also ought to take a lock. Doing this ensures that the locks are always taken in a consistent order, avoiding a deadlock between _free_rid and the various shaped_text* functions.
The full TSAN output can be found at #32081 (comment).