Nabla icon indicating copy to clipboard operation
Nabla copied to clipboard

Add thread safe mutability for IAsset and eliminate a possible "double load" bug

Open devshgraphicsprogramming opened this issue 4 years ago • 1 comments

Describe the bug

Prevent Double Loads of assets caused by multiple threads deciding to load an asset because all have queried the asset cache before either was done loading

Description of the related problem

Thread Safety

Assets should have a lock which should prevent read-accesses while an asset is being modified, and more than one thread/fiber modifying an asset.

Double Asset Loads

There is a possibility of a double load of an asset occuring if following events occur in this order:

  1. Thread A checks our thread-safe asset cache for path "huge.obj"
  2. Thread A decides to load the OBJ
  3. Thread B checks the asset cache for the same path
  4. Thread B decides to load the OBJ
  5. Thread A inserts loaded OBJ into cache
  6. Thread B inserts loaded OBJ into cache

Because the Cache is a multimap, both assets which are identical sit in the cache. This order of events is not the only one that produces the bug, other orders such as 123465, 123546, 132456, etc. will result in this problem.

Solution proposal

Thread Safety

Improve the Readers-Writers Lock in FW_Mutex to be more in line with std::recursive_lock APIs, at least the reading lock should be able to be recursive (writer lock not so much).

There should be an explicit method to:

  • acquire/release a reader lock
  • acquire/release a writer lock
  • upgrade/downgrade an already acquired reader lock to/from a writer lock without getting preempted

If writer lock has not been acquired then all the mutators of IAsset should assert.

However const-qualified functions should acquire the reader lock implicitly.

Double Asset Loads

We need to improve our cache container to be able to take "reservations" for keys, that is:

  1. If no concrete cached value or reservation has been made, proceed 1a. Reserve a key value and place a dummy value indicating key has been reserved 1b. Load the asset 1c. Swap the reservation for the loaded asset
  2. If reservation has been already made, then wait until the asset has been loaded and reservation changes
  3. If concrete value is present then just return the asset

This might require a readers/writers lock, and/or sempahores/condition variables.

We need to analyze this design for the possibility of dead-locks because assets are DAGs and during the process of loading a single thread could put down multiple reservations.

a reader-parallel hashmap / CObjectCache would be useful too https://github.com/buildaworldnet/IrrlichtBAW/issues/364