Fix possible race condition in topic common#769
Conversation
vgvoleg
force-pushed
the
fix_potentional_race_condition
branch
from
4269d25 to
a635376
Compare
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix a race condition in the shared event loop initialization code in ydb/_topic_common/common.py by replacing a concurrent.futures.Future-based synchronization mechanism with a threading.Event-based approach. Additionally, it updates the flake8 testing dependency from version 3.9.2 to 5.0.0+.
Changes:
- Replaced
concurrent.futures.Futurewiththreading.Eventfor signaling when the shared event loop is ready - Moved the global event loop assignment into the event loop thread and added it before the ready signal
- Updated flake8 from a pinned version (3.9.2) to a minimum version constraint (>=5.0.0)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| ydb/_topic_common/common.py | Refactored event loop initialization to use threading.Event instead of concurrent.futures.Future for synchronization |
| test-requirements.txt | Updated flake8 version constraint from ==3.9.2 to >=5.0.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This reverts commit ed47dee.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -60,7 +65,11 @@ def start_event_loop(): | |||
| ) | |||
| t.start() | |||
|
|
|||
| _shared_event_loop = event_loop_set_done.result() | |||
| loop_ready.wait() | |||
|
|
|||
| if _shared_event_loop is None: | |||
| raise RuntimeError("Event loop was not properly initialized") | |||
|
|
|||
| return _shared_event_loop | |||
There was a problem hiding this comment.
There's a potential data race between the unsynchronized read of _shared_event_loop at line 39 and the write at line 55 (which happens in a different thread outside the lock). While this is likely safe in CPython due to the GIL making reference assignments atomic, it's not guaranteed to be safe in all Python implementations and violates proper synchronization patterns.
Consider capturing the event loop reference in a local variable within the lock and returning that, while still setting the global for future callers. This would eliminate the data race and make the code more robust.
| def __eq__(self, other: object) -> bool: | ||
| return ( | ||
| type(self) == type(other) and isinstance(other, YdbRetryOperationSleepOpt) and self.timeout == other.timeout | ||
| type(self) is type(other) and isinstance(other, YdbRetryOperationSleepOpt) and self.timeout == other.timeout |
There was a problem hiding this comment.
The isinstance check is redundant after type(self) is type(other). If type(self) is type(other) is True, then other must be an instance of YdbRetryOperationSleepOpt (assuming self is). The isinstance check can be safely removed without changing the behavior.
| return ( | ||
| type(self) == type(other) | ||
| type(self) is type(other) | ||
| and isinstance(other, YdbRetryOperationFinalResult) |
There was a problem hiding this comment.
The isinstance check is redundant after type(self) is type(other). If type(self) is type(other) is True, then other must be an instance of YdbRetryOperationFinalResult (assuming self is). The isinstance check can be safely removed without changing the behavior.
2 participants
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information