fix: concurrency TypeError on identical resume times#272
Merged
Conversation
900f762 to
e96b27a
Compare
Contributor
leandrodamascena
left a comment
There was a problem hiding this comment.
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.
Comment on lines
+84
to
+88
| heapq.heappush( | ||
| self._pending_resumes, | ||
| (resume_time, self._schedule_counter, exe_state), | ||
| ) | ||
| self._schedule_counter += 1 |
Contributor
There was a problem hiding this comment.
Suggested change
| 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.
Member
Author
since the class already needs a lock anyway (because of the heap), I figured a |
wangyb-A
approved these changes
Jan 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.