Honor a timeout of 0 in wait_for and ResultSet.join instead of waiting forever - #10487
Honor a timeout of 0 in wait_for and ResultSet.join instead of waiting forever#10487auvipy merged 9 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This PR aims to make timeout=0 behave consistently across result-waiting APIs by treating it as an immediate (non-blocking) timeout rather than falling back to “wait forever”, and adds unit coverage for the corrected behavior.
Changes:
- Fix BaseBackend.wait_for() to treat timeout=0 as a real timeout condition (instead of being skipped due to falsy checks).
- Fix ResultSet.join() to compute and pass a timeout even when it is 0 (instead of passing None and potentially blocking indefinitely).
- Add unit tests covering timeout=0 for both wait_for() and join().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| celery/backends/base.py | Adjusts timeout handling in backend polling loop to honor timeout=0. |
| celery/result.py | Adjusts join timeout computation so timeout=0 doesn’t default to “no timeout”. |
| t/unit/backends/test_base.py | Adds a regression test asserting wait_for(timeout=0) raises TimeoutError instead of looping. |
| t/unit/tasks/test_result.py | Adds a regression test asserting join(timeout=0) raises TimeoutError instead of blocking indefinitely. |
celery/result.py:816
- join(timeout=0) currently raises TimeoutError before calling result.get(), so it will time out even when all results are already ready. For timeout=0, it should behave like a non-blocking join: return immediately for ready results and only raise if a result isn't ready.
if timeout is not None:
remaining = timeout - (time.monotonic() - time_start)
if remaining <= 0.0:
raise TimeoutError('join operation timed out')
value = result.get(
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #10487 +/- ##
=======================================
Coverage 88.76% 88.76%
=======================================
Files 153 153
Lines 19956 19955 -1
Branches 2334 2333 -1
=======================================
Hits 17713 17713
Misses 1945 1945
+ Partials 298 297 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Sorry, something went wrong.
…nstead of raising, so a ready result returns and only a pending one raises
|
My fix was wrong in the other direction, and your test is what showed it. It computed remaining as negative on the first pass and raised before checking readiness, so timeout=0 always failed even when every result was already done. I'd turned "waits forever" into "always fails", and 0 should mean don't block, not fail. Pushed a fix that clamps remaining to 0 instead of raising, and lets each get() decide. wait_for already returns immediately for a ready task and only raises once a poll finds it pending, so your test passes both ways now. Writing it out as a test made the contract obvious, better than a comment would have. |
Sorry, something went wrong.
… so timeout=0 does not block and a sub-interval timeout is not overshot
|
is there any open or closed issue for this fix? |
Sorry, something went wrong.
AsyncResult.get(timeout=0) never returns. BaseBackend.wait_for guards its timeout check with if timeout and time_elapsed >= timeout, so a timeout of 0 is falsy and the poll loop has no exit. The docstring right above it says TimeoutError is raised "if timeout is not None, and the operation takes longer than timeout seconds", which makes 0 mean give up immediately, not wait forever.
ResultSet.join has the same guard: if timeout: leaves remaining as None, so every result.get() in the loop is called with no timeout at all.
Against a backend that stays PENDING:
timeout=None still waits forever, which is what it means. timeout=1 is unchanged.
The existing test_join_timeout uses timeout=0.0000001 and not 0, which looks like it was written around this.
Two tests added, one per site. Both fail on main by hitting a pytest-timeout instead of raising, since the unfixed loop never exits. t/unit/tasks/test_result.py and t/unit/backends/test_base.py are otherwise unchanged, 242 passing. Two pre-existing failures in test_store_result_parent_id[yaml|msgpack] are unrelated and fail the same way on main.