Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes thread locking from TaskTrackerMiddleware to eliminate potential GIL contention and improve performance. The change assumes all operations occur within the same event loop thread, making explicit locking unnecessary.
Key Changes:
- Removed
threading.Lockand allwith self._lockblocks - Added explanatory comment documenting the rationale for lock-free approach
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Note: No lock needed because: | ||
| # 1. dispatch() runs in the single-threaded event loop | ||
| # 2. cancel_all_tasks() is called from the same event loop thread | ||
| # 3. Python set.add/discard are atomic operations |
There was a problem hiding this comment.
The claim that 'Python set.add/discard are atomic operations' is misleading in this context. While the underlying C operations may be atomic at the bytecode level, this doesn't guarantee thread-safety for the set as a whole. If cancel_all_tasks() can be called from a different thread (not just the event loop thread), concurrent access to self._active_tasks during iteration in line 50 could cause RuntimeError due to set size changes during iteration, even if individual add/discard operations are atomic.
| # Make a copy to avoid mutation during iteration | ||
| with self._lock: | ||
| tasks_to_cancel = list(self._active_tasks) | ||
| tasks_to_cancel = list(self._active_tasks) |
There was a problem hiding this comment.
Removing the lock around list(self._active_tasks) creates a race condition. If cancel_all_tasks() can be called from outside the event loop thread while dispatch() is modifying _active_tasks, this can raise RuntimeError: Set changed size during iteration. The comment in lines 16-21 assumes cancel_all_tasks() runs in the event loop thread, but this needs verification. If it can be called from signal handlers or shutdown hooks in different threads, the lock removal is unsafe.
| # Note: No lock needed because: | ||
| # 1. dispatch() runs in the single-threaded event loop | ||
| # 2. cancel_all_tasks() is called from the same event loop thread |
There was a problem hiding this comment.
The comment assumes cancel_all_tasks() is always called from the event loop thread, but this assumption should be explicitly validated and enforced in the code. If this method can be invoked during shutdown, from signal handlers, or via external APIs, it may execute in a different thread context. Consider adding an assertion or check to verify this assumption, or document clearly in the method's docstring that it must only be called from the event loop thread.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Opening this PR to start a discussion on whether a lock-free mechanism makes sense here. I'm investigating this hot path as a potential GIL contention area.