Use MutexLock in more places
#96166
Use MutexLock in more places
#96166
Conversation
| @@ -60,32 +60,32 @@ void Resource::set_path(const String &p_path, bool p_take_over) { | |||
| p_take_over = false; // Can't take over an empty path | |||
| } | |||
|
|
|||
| ResourceCache::lock.lock(); | |||
| { | |||
| MutexLock lock(ResourceCache::lock); | |||
There was a problem hiding this comment.
We should probably rename ResourceCache::lock to ResourceCache::mutex at some point as it's just a recipe for confusion having an incorrect name for it, left it alone in this PR though
| baking_navmesh_mutex.lock(); | ||
| generator_task_mutex.lock(); | ||
| MutexLock baking_navmesh_lock(baking_navmesh_mutex); | ||
| MutexLock generator_task_lock(generator_task_mutex); |
There was a problem hiding this comment.
While it doesn't seem to cause any problems I feel that locking two mutexes like this is a recipe for deadlock, so would be good to look into that down the line
|
I'm a fan of consistency as much as I'm a fan of expressivity. So, on the one hand, I feel that we lose something by letting explicit lock-unlock go (because in some places, having the unlock line around helps remembering you're reading a critical section, etc.)., while on the other hand, I feel that trying to use the RAII syntax always reduces cognitive load when maintaining code as you don't need to ask yourself which one to use. I'd say the tradeoff is a bit towards the bad side, but that's a bit subjective, so let's hear more opinions. |
|
Can split up the cases that are trailing etc. and keeping it in trivial cases like a full block especially with newly indicated blocks |
That sounds very reasonable. |
AThousandShips
force-pushed
the
more_lock_raii
branch
from
6ebc237 to
0d0132b
Compare
|
Split cases into three categories:
|
|
Will squash them up unless we expect to revert some and not others |
AThousandShips
force-pushed
the
more_lock_raii
branch
from
0d0132b to
f8815db
Compare
AThousandShips
force-pushed
the
more_lock_raii
branch
from
f8815db to
e33fdb4
Compare
|
Thanks! |
|
Thank you! |
This ensures more safe management of mutexes and makes forgetting to unlock it less likely, most of these cases are trivial though and don't depend on it for safety but I'd prefer to avoid using direct locking unless it's necessary to handle locking between methods etc. or in some cases where using it is just too cluttered
There are some cases which could benefit from a little more of this using the new
temp_unlock/relockmethods ofMutexLockbut kept things simple here