Skip to content

Conversation

@yaythomas
Copy link
Member

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

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@yaythomas yaythomas requested a review from wangyb-A as a code owner January 27, 2026 09:13
@yaythomas yaythomas force-pushed the concurrency-exact-same-time-heap-compare branch 2 times, most recently from 900f762 to e96b27a Compare January 27, 2026 09:51
Copy link
Contributor

@leandrodamascena leandrodamascena left a 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.

Comment on lines +84 to +88
heapq.heappush(
self._pending_resumes,
(resume_time, self._schedule_counter, exe_state),
)
self._schedule_counter += 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

@yaythomas yaythomas force-pushed the concurrency-exact-same-time-heap-compare branch from e96b27a to 631764e Compare January 27, 2026 10:12
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
@yaythomas yaythomas force-pushed the concurrency-exact-same-time-heap-compare branch from 631764e to b59b71a Compare January 27, 2026 10:52
Have set up GitHub repo permissions so only appropriate
reviewers can approve/merge on main.
@yaythomas
Copy link
Member Author

so you wouldn't need the lock for the increment. But totally optional, what you have works fine.

since the class already needs a lock anyway (because of the heap), I figured a next doesn't particularly save on that and counter is maybe a bit more straightforward.

@yaythomas yaythomas merged commit 4cf8806 into main Jan 27, 2026
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: concurrency with exact same resume_time

3 participants