Hello Django Community,
I apologize if I’ve posted this in the wrong section. I couldn’t find a dedicated discussion area for Redis or cache-related bugs.
This topic is about Ticket #35651 and this PR.
I’m working on a pull request to optimize Django’s Redis client by making the connection pool process-global instead of thread-local, as Redis’s connection pool is thread-safe (referencing redis-py#1107 and redis-py#1270). This change reduces redundant pools and lowers Redis idle connections.
However, there’s a potential race condition when multiple threads concurrently create the global pool. To mitigate this, the code uses setdefault() to ensure atomic assignment:
self._pools.setdefault(index, pool_creation_logic)
@carltongibson have raised the question of whether we should add tests to validate that setdefault() correctly prevents race conditions. His concern is valid, but such tests are notoriously difficult to implement, as they require forcing concurrent creation scenarios. Notably, similar patterns (e.g., for database connection pools) exist in Django without explicit race-condition tests, relying instead on thread-safe constructs PR: Added connection pool support for PostgreSQL.
Questions for the community:
-
Is testing this race condition necessary?
- While the pattern is established in Django (e.g., database pools), should Redis’s implementation follow stricter validation?
- Does the risk of untested race conditions outweigh the complexity of writing such tests?
-
If testing is required, how can we approach it?
- Are there existing patterns in Django’s test suite for simulating race conditions (e.g., using threading barriers, mocking, or deliberate delays)?
- Would a “best effort” test (e.g., asserting that repeated concurrent calls return the same pool) suffice, even if it’s not deterministic?
Any insights, references to existing tests, or opinions on balancing correctness vs. test complexity would be greatly appreciated.
Thank you for your time and expertise!
Best regards,
HappyDingning