Fix locks in TextureLoader class #1147

xezon · 2024-08-21T14:17:34Z

Potentially fixes w3dview texture crash #1131

xezon · 2024-08-21T14:18:25Z

src/w3d/renderer/textureloader.cpp

@@ -503,6 +542,7 @@ void TextureLoader::Begin_Load_And_Queue(TextureLoadTaskClass *task)

    // If we can't start the load of either a dds or tga for the filename in the task set it to missing.
    if (task->Begin_Load()) {
+        FastCriticalSectionClass::LockClass lock(g_backgroundCritSec);
        g_backgroundQueue.Push_Front(task);


Lock is missing here on write. Could race.

xezon · 2024-08-21T14:19:03Z

src/w3d/renderer/textureloader.cpp

        int time = rts::Get_Time();
        TextureLoadTaskClass *task;

-        while ((task = g_foregroundQueue.Pop_Front()) != nullptr) {


Lock exists before already, but scope can be limited. Optimization.

xezon · 2024-08-21T14:20:58Z

src/w3d/renderer/textureloader.cpp

@@ -385,9 +408,17 @@ void TextureLoader::Request_Foreground_Loading(TextureBaseClass *texture)
 */
 bool TextureLoader::Queues_Not_Empty()
 {
-    FastCriticalSectionClass::LockClass lock(g_backgroundCritSec);
-
-    return !g_backgroundQueue.Empty() && !g_foregroundQueue.Empty();


Lock for g_foregroundQueue is missing here on read.

xezon · 2024-08-21T14:23:25Z

src/w3d/renderer/textureloader.cpp

@@ -344,32 +352,47 @@ void TextureLoader::Request_Foreground_Loading(TextureBaseClass *texture)
    TextureLoadTaskClass *thumb_task = texture->Get_Thumbnail_Load_Task();

    if (!TextureLoader::Is_DX8_Thread()) {
-        FastCriticalSectionClass::LockClass lock2(g_backgroundCritSec);


Originally takes both locks at the same time. This can be risky and introduce ABA deadlock problem. This can be avoided by limiting scope.

Could perhaps deadlock with LoaderThreadClass::Thread_Function

xezon · 2024-08-21T14:23:48Z

src/w3d/renderer/textureloader.cpp

@@ -344,32 +352,47 @@ void TextureLoader::Request_Foreground_Loading(TextureBaseClass *texture)
    TextureLoadTaskClass *thumb_task = texture->Get_Thumbnail_Load_Task();

    if (!TextureLoader::Is_DX8_Thread()) {
-        FastCriticalSectionClass::LockClass lock2(g_backgroundCritSec);
        if (thumb_task != nullptr) {
            thumb_task->Set_Load_State(TextureLoadTaskClass::STATE_FOREGROUND_OVERRIDE);
        }

        if (task != nullptr) {
            if (task->Get_Parent() == &g_backgroundQueue) {


This does not need locking. g_backgroundQueue address never changes.

xezon · 2024-08-21T14:27:06Z

src/w3d/renderer/textureloader.cpp

@@ -312,15 +322,14 @@ void TextureLoader::Request_Thumbnail(TextureBaseClass *texture)
 */
 void TextureLoader::Request_Background_Loading(TextureBaseClass *texture)
 {
-    FastCriticalSectionClass::LockClass lock(g_foregroundCritSec);


Lock scope can be limited. Optimization.

xezon · 2024-08-21T14:27:28Z

src/w3d/renderer/textureloader.cpp

@@ -282,21 +291,22 @@ w3dsurface_t TextureLoader::Load_Surface_Immediate(const StringClass &texture, W
 */
 void TextureLoader::Request_Thumbnail(TextureBaseClass *texture)
 {
-    FastCriticalSectionClass::LockClass lock(g_foregroundCritSec);


Lock scope can be limited. Optimization.

xezon · 2024-08-21T14:28:36Z

src/w3d/renderer/textureloader.cpp

@@ -81,9 +81,18 @@ w3dtexture_t Load_Compressed_Texture(
 void LoaderThreadClass::Thread_Function()
 {
    while (m_isRunning) {
-        if (!g_backgroundQueue.Empty()) {


Lock for g_backgroundQueue is missing here on read.

xezon · 2024-08-21T14:31:44Z

src/w3d/renderer/textureloader.cpp

@@ -81,9 +81,18 @@ w3dtexture_t Load_Compressed_Texture(
 void LoaderThreadClass::Thread_Function()
 {
    while (m_isRunning) {
-        if (!g_backgroundQueue.Empty()) {
+        bool backgroundQueueEmpty;
+        {
            FastCriticalSectionClass::LockClass background_lock(g_backgroundCritSec);


Originally takes both locks at the same time. This can be risky and introduce ABA deadlock problem. This can be avoided by limiting scope.

Could perhaps deadlock with TextureLoader::Request_Foreground_Loading.

xezon added the vanilla-bug label Aug 21, 2024

xezon commented Aug 21, 2024

View reviewed changes

xezon added the fixing label Aug 21, 2024

xezon added 2 commits September 21, 2024 07:37

Fix locks in TextureLoader class

8a0ae72

Additional opt

8fbff61

xezon force-pushed the fix-textureloader-locks branch from e18df69 to 8fbff61 Compare September 21, 2024 05:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix locks in TextureLoader class #1147

Fix locks in TextureLoader class #1147

xezon commented Aug 21, 2024

xezon Aug 21, 2024

xezon Aug 21, 2024

xezon Aug 21, 2024

xezon Aug 21, 2024

xezon Aug 21, 2024

xezon Aug 21, 2024

xezon Aug 21, 2024

xezon Aug 21, 2024

xezon Aug 21, 2024

Fix locks in TextureLoader class #1147

Are you sure you want to change the base?

Fix locks in TextureLoader class #1147

Conversation

xezon commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment