Repository navigation
Conversation
…ener bleak.BleakClient.start_notify wraps an async notification callback so each notification dispatches via a detached asyncio.create_task, never awaited by the coroutine listen()/listen_payload() callers actually await. A packet callback exception therefore became an unretrieved task exception instead of reaching the caller, which just saw the listener hang. Catch the exception inside the handler, store the first one, and signal the same event the disconnect callback already uses; the poll loop re-raises it from the coroutine the caller awaits. Added tests/test_ble_notification_dispatch.py, which swaps in a duck-typed backend via BleakClient(backend=...) so bleak's real start_notify wrapping/dispatch runs (the existing test_ble_callbacks.py coverage only ever awaited the captured handler directly, a path bleak itself never takes). Updated that existing test to match the new contract: the notification call site itself must not raise, since nothing awaits it in production. Refs #21264
This branch has not been deployed
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.
What
sdks/python/omi/ble.py::listen()(andlisten_payload()) awaited the caller'son_packet/on_payloadcallback result inline inside the notification handler, with notry/except.
bleak.BleakClient.start_notifydetects that handler is a coroutine functionand wraps it so every notification dispatches via a detached
asyncio.create_task(seebleak/__init__.py), never awaited by the coroutine the SDK caller is actually awaiting.So when a packet callback raised, the exception became an unretrieved task exception
logged by asyncio's default handler, and the caller of
listen()/listen_payload()justsaw the listener hang instead of seeing the real failure (#21264).
Fix
Catch the exception inside the handler, store the first one, and set the same
stoppedevent the disconnect callback already uses. The listener's poll loop watches that event
and now re-raises the stored exception from the coroutine the caller is awaiting, instead
of depending on the notification call frame (which nothing awaits in production).
Why this needed a real-dispatch test, not just a handler-level one
The existing
test_ble_callbacks.py::test_callback_exceptions_propagateonly ever awaitedthe captured handler directly from the test body, which is not the path bleak itself
takes in production (it schedules the handler via a detached task) -- so that test gave
false confidence. Added
tests/test_ble_notification_dispatch.py, which swaps in aduck-typed backend via
BleakClient(backend=...)so the realBleakClient.start_notifywrapping/dispatch runs, and updated the handler-level test so it no longer asserts the
call site itself raises (it must not, in production nothing awaits that call).
Testing
mainby reading bleak 0.22.3's actualstart_notifysource (confirms theasyncio.create_taskdispatch) before writing anyfix.
bash sdks/python/test.sh: 56 passed (was 53 before; +3 new regression tests).ble.pywith exactly thereported symptom (
Task exception was never retrieved,TimeoutErroron the caller),and pass once the fix is applied.
bleak's own dispatch/wrapping code is unmodified and still runs.
Failure-Class: new
@TuEmb @mdmohsin7 — BLE/device-link change per repo convention, requesting your review
before merge.
Auto-generated from issue feedback by the mini issues-improver — tested but NOT merged
(BLE exception: requires BLE-owner review per this job's rules).
fixes #21264