Fix locks in TextureLoader class #1147
Conversation
xezon
commented
Aug 21, 2024
xezon
commented
- Potentially fixes w3dview texture crash #1131
| @@ -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); | |||
There was a problem hiding this comment.
Lock is missing here on write. Could race.
| int time = rts::Get_Time(); | ||
| TextureLoadTaskClass *task; | ||
|
|
||
| while ((task = g_foregroundQueue.Pop_Front()) != nullptr) { |
There was a problem hiding this comment.
Lock exists before already, but scope can be limited. Optimization.
| @@ -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(); | |||
There was a problem hiding this comment.
Lock for g_foregroundQueue is missing here on read.
| @@ -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); | |||
There was a problem hiding this comment.
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
| @@ -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) { | |||
There was a problem hiding this comment.
This does not need locking. g_backgroundQueue address never changes.
| @@ -312,15 +322,14 @@ void TextureLoader::Request_Thumbnail(TextureBaseClass *texture) | |||
| */ | |||
| void TextureLoader::Request_Background_Loading(TextureBaseClass *texture) | |||
| { | |||
| FastCriticalSectionClass::LockClass lock(g_foregroundCritSec); | |||
There was a problem hiding this comment.
Lock scope can be limited. Optimization.
| @@ -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); | |||
There was a problem hiding this comment.
Lock scope can be limited. Optimization.
| @@ -81,9 +81,18 @@ w3dtexture_t Load_Compressed_Texture( | |||
| void LoaderThreadClass::Thread_Function() | |||
| { | |||
| while (m_isRunning) { | |||
| if (!g_backgroundQueue.Empty()) { | |||
There was a problem hiding this comment.
Lock for g_backgroundQueue is missing here on read.
| @@ -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); | |||
There was a problem hiding this comment.
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
force-pushed
the
fix-textureloader-locks
branch
from
e18df69 to
8fbff61
Compare
