VideoCommon: add additional locks around asset access / usage #11891
Conversation
| iter != m_assetid_to_asset_map_path.end()) | ||
| { | ||
| const auto& asset_map_path = iter->second; | ||
| const auto asset_map = GetAssetMapForID(asset_id); |
There was a problem hiding this comment.
I chose to prefer a copy instead of holding onto the lock for the whole time that the texture was loaded
iwubcode
force-pushed
the
asset_thread_safety
branch
from
2a388b5
to
cb89a21
Compare
| @@ -50,14 +50,20 @@ class CustomAssetLoader | |||
| std::map<CustomAssetLibrary::AssetID, std::weak_ptr<AssetType>>& asset_map, | |||
| std::shared_ptr<CustomAssetLibrary> library) | |||
| { | |||
| std::lock_guard lk(m_asset_map_lock); | |||
| auto [it, inserted] = asset_map.try_emplace(asset_id); | |||
| if (!inserted) | |||
| return it->second.lock(); | |||
There was a problem hiding this comment.
I'm just noticing this now, but does this actually work correctly for expired weak_ptrs? The weak_ptr doesn't cease to exist when the pointed at shared_ptr no longer exists, it just returns an empty shared_ptr on lock, right? So I would actually expect something like:
if (!inserted)
{
auto shared = it->second.lock();
if (shared)
return shared;
}
There was a problem hiding this comment.
I guess that's what the custom deleter is for, now that I think about it a bit more... but is that completely threadsafe? What if one thread is in the deleter waiting for the m_asset_map_lock while another thread is currently in the LoadOrCreateAsset() function? Can that even happen...?
There was a problem hiding this comment.
When the last shared_ptr gets destroyed, it will clear out the entry from the map, so in theory it should never hit the case of nullptr but it's better to be safe. Ok if I add it to this review?
There was a problem hiding this comment.
Missed your other comment. That's a good point, does seem like it could be racey (seems very unlikely). You'd have to be in the deleter while also adding to that same asset-id
There was a problem hiding this comment.
Does the shared_ptr count as deleted by the time the deleter is called?? I think I need to debug through this with a test case, this is a very odd construct you've built here.
There was a problem hiding this comment.
Alternatively, maybe we just accept that dead weak_ptrs can hang around in the map and remove the map-erase in the deleter, that seems a lot more straightforward.
There was a problem hiding this comment.
Yeah, this approach is a bit of a double edged sword. Nice from a usability perspective but does have some concerns to think through. You were right to call this out. I'm not seeing anything in the std that talks about this. Will have to look a little closer in a little while.
There was a problem hiding this comment.
Alternatively, maybe we just accept that dead weak_ptrs can hang around in the map and remove the map-erase in the deleter, that seems a lot more straightforward.
Yeah, I suppose there aren't going to be that many assets floating around. Maybe I was trying to be too clever..
There was a problem hiding this comment.
I decided to remove the map erase for now. I'm going to investigate this on the side to see if we can still have the auto delete
iwubcode
force-pushed
the
asset_thread_safety
branch
from
cb89a21
to
77409b3
Compare
…sure thread safety
iwubcode
force-pushed
the
asset_thread_safety
branch
from
77409b3
to
9d7ab47
Compare
For the most part, this review adds additional locks in place for thread safety. There were a few places where locks were being held onto too long, that has been updated as well.