Make minject thread safe

[biniona]

Currently, if you try to instantiate two objects with minject in different threads, this can lead to undefined behavior.

This issue captures the task of locking minject (possibly with a reentrant lock) during object instantiation.

[xiyan128]

Seems like potential race conditions can occur when we are concurrently registering objects. There are mainly two ways of registering objects in minject:

1. Explicitly registering an object with Registry.register(...) or Registry._set_by_metadata(...)
   • e.g. setting registry[Foo] = <obj>
   • A race condition might occur due to shared ownership of the indices (Registry._by_meta, Registry._by_name, Registry._by_iface) and object list (Registry._objects)
2. Lazily initializing singleton objects with Registry._register_by_metadata(...)
   • e.g. getting the singleton object foo = registry[Foo], where Foo is not in the registry yet
   • When done concurrently in several threads, this might cause multiple objects to be created, breaking the singleton object invariant.

Therefore, we should (1) lock access to shared data structures and (2) synchronize the lazy initialization procedure.

An RLock would be great since we want to recursively read from the registry, e.g., when constructing objects with bindings.

Since reads are typically much more frequent than writes—reading may occur frequently throughout execution and writing usually occurs during the application startup or occasionally for lazy registrations—we may also want to make the lock a RWLock to allow a number of readers or at most one writer. This will ensure that readers are not blocked by each other.

TODO:

• Synchronize object registration
• Implement thread-safe lazy initialization
• Add thread-safety documentation
• Go through all public methods to ensure they acquire appropriate locks
  • get and getitem methods
  • start and close, etc
• Add stress tests; profile and optimize read operations

[biniona]

Hi @xiyan128! This looks great.

I think you are correct in needing to add a lock to result = registry[foo] and registry[foo] = result.

Could you help me understand what you mean by "lock access to shared data structures?" I think it makes sense to lock the registry instance at singleton initialization time, but once the singletons are initialized I'm not sure why multiple threads couldn't concurrently read an already initialized singleton. I think you touch on this in your last paragraph, but just want to make sure we are on the same page.

Thanks for taking this task!

[xiyan128]

@biniona by locking shared data structures I mean locking the Registry._by_meta, Registry._by_name, and Registry._by_iface maps in the registry. There are two types of accesses (read and write). A race condition might happen when there are concurrent writes OR concurrent read+write.

I'm not sure why multiple threads couldn't concurrently read an already initialized singleton.

Reading the registry should always be fine as long as there's no concurrent writer! However, we do want to prevent a reader from reading the registry while there are concurrent writers. My plan is to solve this using a RWLock, so that readers don't block each other.

[xiyan128]

Saw #20

@biniona does this imply that we don't need to lock object registration at start up time?

[xiyan128]

For now, focusing on making lazy initialization thread safe:

Added a test case to show that sometimes different instances of the singleton class might be returned.

minject/tests/test_registry.py

Lines 465 to 483 in 892e0d5

	def test_concurrent_lazy_init(self):
	num_queries = 1000
	query_per_class = 2
	num_classes = num_queries // query_per_class
	@cache
	def new_type(i):
	return type(f"NewType{i}", (), {})
	def lazy_load_object(i):
	return self.registry[new_type(i % num_classes)]
	with ThreadPoolExecutor(max_workers=query_per_class) as executor:
	futures = [executor.submit(lazy_load_object, i) for i in range(num_queries)]
	results = [future.result() for future in as_completed(futures)]
	# group results by object id, and assert that each type is only instantiated once
	conuter = Counter(map(id, results))
	assert all(count == query_per_class for count in conuter.values())

How to run this test?

To trigger the race condition more reliably, turn on this flag

minject/minject/registry.py

Line 17 in 892e0d5

TEST_LAZY_INIT_RACE_CONDITION = True

Run hatch test. Notice the test fails because some singleton classes may be instantiated twice.

>       assert all(count == query_per_class for count in conuter.values())
E       assert False
E        +  where False = all(<generator object RegistryTestCase.test_concurrent_lazy_init.<locals>.<genexpr> at 0x105578860>)

Now, turn on the flag for the fix

minject/minject/registry.py

Line 18 in 892e0d5

SYNCHRONIZE_LAZY_INIT = False

All tests should reliably pass now.

[biniona]

Re #20 : Let's not relax locking logic for startup initialization.

[xiyan128]

Sounds good. I'll put together a PR that includes test cases for both startup initialization and lazy initialization thread safety.