Fix two crashes in the aiokafka threaded producer - #759
Conversation
Both were found by the type checker in #758 and left marked `XXX` there because fixing them changes runtime behaviour. `ThreadedProducer._shutdown_thread` was a plain `def` overriding `mode.threads.ServiceThread._shutdown_thread`, which is `async def` and is awaited by `_serve()` in a `finally:`. Every shutdown of the producer thread therefore evaluated `await None` and raised TypeError. The thread only recovered because `_start_thread` catches that exception and calls `set_shutdown()` before re-raising -- so mode's teardown (`on_thread_stop`, stopping children, futures and exit stacks) never ran, and the thread died with a traceback instead of stopping cleanly. The override also scheduled `on_thread_stop()` with `asyncio.run_coroutine_threadsafe` onto `self.thread_loop` -- the loop that was about to stop, and the loop already running `_serve()`. Because the TypeError tore down `run_until_complete` immediately, that coroutine never got a chance to run, so the producer was never flushed or stopped on this path. Make it `async def` and await `super()._shutdown_thread()`, which runs `on_thread_stop()` and the rest of mode's teardown in order. The once-only guard is kept; when shutdown has already been initiated the shutdown event is still set, matching what the old TypeError path ended up doing via `_start_thread`. `ThreadedProducer.publish_message(wait=True)` called `fut.message.channel._on_published(message=..., state=..., producer=...)`. `Topic._on_published` takes the send future as a required *positional* `fut` and reads the result off it, so the call raised `TypeError: Topic._on_published() missing 1 required positional argument`. The waiting branch has no such future -- `send_and_wait` has already resolved -- so complete the message directly instead: report the sensor, set the result, and invoke the callback, which is what `Topic.publish_message(wait=True)` does via `_finalize_message`. The non-waiting branch keeps using `_on_published` as a done-callback, where `add_done_callback` supplies `fut`. `test_publish_message_with_wait` did not catch this because its channel is a bare `Mock`, which accepts any call; the new test uses a real topic and fails with the TypeError above against the previous code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012N2yysiNbzzVcvYhtrpsM7
805596d to
02a772c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #759 +/- ##
==========================================
- Coverage 96.05% 96.04% -0.01%
==========================================
Files 103 103
Lines 11081 11084 +3
Branches 1189 1190 +1
==========================================
+ Hits 10644 10646 +2
- Misses 345 346 +1
Partials 92 92 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I'm not pushing a change for it, because there is no uncovered line to cover. Codecov's own patch check agrees: "All modified and coverable lines are covered by tests." The project delta comes from its summary reporting That is the only file whose numbers move anywhere in the package — 3 new statements, all 3 covered, misses flat. The missing lines are also the same set on both sides, just shifted by two line numbers. So the -0.01% is a single line out of ~11,084 appearing as uncovered in the merged 15-leg report while every individual leg covers it. Worth noting this is structural rather than specific to this PR: the repo has no codecov:
notify:
after_n_builds: 15 # 5 pythons x 2 cython (aiokafka) + 5 confluent.
# Not 16 - the PyPy leg is continue-on-error and
# uploads conditionally, so waiting on it can hang.
coverage:
status:
project:
default:
threshold: 0.5% # tolerate cross-leg varianceHappy to open that as a separate PR if wanted — it doesn't belong in this one. Generated by Claude Code |
Both bugs were surfaced by the type checker in #758 and left marked
XXXthere, because fixing them changes runtime behaviour and that PR was annotation-only. This PR fixes them, each with a regression test.1.
_shutdown_threadraised TypeError on every producer-thread shutdownThreadedProducer._shutdown_threadwas a plaindefoverridingmode.threads.ServiceThread._shutdown_thread, which isasync defand is awaited by_serve():So every shutdown of the thread evaluated
await Noneand raisedTypeError. The thread only appeared to recover because_start_threadcatches the exception and callsset_shutdown()before re-raising — meaning mode's teardown (on_thread_stop, stopping children, futures and exit stacks) never ran, and the thread died with a traceback rather than stopping cleanly.There was a second layer to it. The override scheduled
on_thread_stop()with:thread_loopis both the loop already running_serve()and the loop about to stop. Because theTypeErrortore downrun_until_completeimmediately, that coroutine never got a chance to run — so on this path the producer was never flushed and never stopped.The fix makes it
async defand awaitssuper()._shutdown_thread(), which runson_thread_stop()and the rest of mode's teardown in the right order. The once-only guard is preserved, and when shutdown has already been initiated the shutdown event is still set — matching what the old TypeError path ended up doing via_start_thread, sostop()cannot hang.2.
publish_message(wait=True)could never succeedThe waiting branch called:
Topic._on_publishedtakes the send future as a required positional parameter and reads the result off it:Nothing was passed for
fut, so this raisedTypeError: Topic._on_published() missing 1 required positional argument: 'fut'for any real channel._on_publishedis the done-callback for the non-waiting branch, whereadd_done_callbacksupplies the future positionally. The waiting branch has no such future —send_and_waithas already resolved toret— so it now completes the message directly: report the sensor, set the result, invoke the callback. That is exactly whatTopic.publish_message(wait=True)does via_finalize_message. The non-waiting branch is unchanged.Why the existing test missed it
test_publish_message_with_waitpasses today because its channel is a bareMock(), which accepts any call and swallows the missing argument — it pinned the bug rather than catching it. The new test uses a real topic; against the previous code it fails with theTypeErrorabove.Tests
Four tests added, all verified to fail against the pre-fix code:
test_publish_message_with_wait__completes_the_message— real channel; asserts the future resolves to thesend_and_waitresult, the message callback fires, andon_send_completedis reported.test_shutdown_thread_is_a_coroutine— the structural guarantee mode'sawaitdepends on.test_shutdown_thread__runs_mode_teardown— delegates to the base implementation.test_shutdown_thread__already_initiated_still_sets_shutdown— no doubleon_thread_stop, but the shutdown event is still set.scripts/checkpasses with the pinned toolchain; suites go from 2207 to 2211 passed, 4 skipped.🤖 Generated with Claude Code