-
Notifications
You must be signed in to change notification settings - Fork 7
fix: concurrency TypeError on identical resume times #272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
900f762 to
e96b27a
Compare
leandrodamascena
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just taking a look in the repo and saw this PR.
Minor suggestion: consider using itertools.count() instead of the manual counter - it's what the heapq docs recommend for this exact pattern, and next() is atomic so you wouldn't need the lock for the increment. But totally optional, what you have works fine.
| heapq.heappush( | ||
| self._pending_resumes, | ||
| (resume_time, self._schedule_counter, exe_state), | ||
| ) | ||
| self._schedule_counter += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| heapq.heappush( | |
| self._pending_resumes, | |
| (resume_time, self._schedule_counter, exe_state), | |
| ) | |
| self._schedule_counter += 1 | |
| heapq.heappush(self._pending_resumes, (resume_time, next(self._counter), exe_state)) |
e96b27a to
631764e
Compare
When multiple tasks are scheduled with identical resume_time values, heapq.heappush attempts to compare ExecutableWithState objects to break the tie, causing a TypeError since ExecutableWithState doesn't implement `__lt__`. This fix adds a monotonically increasing counter as a tie-breaker in the heap tuple structure. Changes: - Update _pending_resumes type hint to include counter: tuple[float, int, ExecutableWithState] - Add _schedule_counter initialization in `__init__` - Modify schedule_resume() to include counter in heap tuple and increment after each schedule - Update heappop unpacking in _timer_loop to handle 3-tuple - Add missing _shutdown Event initialization The counter ensures deterministic FIFO ordering when timestamps collide, preventing object comparison while maintaining predictable execution order. Tests added to verify: - No TypeError with identical timestamps - Counter increments correctly - FIFO ordering is maintained for same-timestamp tasks closes #271
631764e to
b59b71a
Compare
Have set up GitHub repo permissions so only appropriate reviewers can approve/merge on main.
since the class already needs a lock anyway (because of the heap), I figured a |
When multiple tasks are scheduled with identical resume_time values, heapq.heappush attempts to compare ExecutableWithState objects to break the tie, causing a TypeError since ExecutableWithState doesn't implement
__lt__. This fix adds a monotonically increasing counter as a tie-breaker in the heap tuple structure.Changes:
__init__The counter ensures deterministic FIFO ordering when timestamps collide, preventing object comparison while maintaining predictable execution order.
Tests added to verify:
closes #271
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.