fix(ble): don't hang on a stalled bleak disconnect during teardown - #967
fix(ble): don't hang on a stalled bleak disconnect during teardown#967lalonggone wants to merge 2 commits into
Conversation
BLEInterface.close() could block forever: BLEClient.disconnect() awaited bleak with future.result(None) (no timeout) and BLEClient.close() joined the event-loop thread with no timeout. On some backends (seen on Linux with Python 3.14 + bleak 2.1.1) bleak's disconnect stalls, so close() never returns and the process wedges. A disconnect detected mid-read (BleakDBusError) also left the read buffer `b` unbound, so the following `if not b:` raised UnboundLocalError instead of unwinding the loop cleanly. - Initialize b before the read so a mid-read disconnect unwinds cleanly. - Bound BLEClient.disconnect() with BLE_DISCONNECT_TIMEOUT, swallowing a timeout/BleakError since teardown is best-effort. - Bound BLEClient.close()'s daemon-thread join with the same timeout. Extends the existing "if bleak is hung, don't wait" mitigation already on the receive-thread join in BLEInterface.close(). Adds regression tests. Signed-off-by: Laura Long <laura@longgone.dev>
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe BLE interface now handles mid-read disconnects, bounds teardown operations to five seconds, suppresses teardown errors, and cancels timed-out futures. Tests cover each behavior. ChangesBLE disconnect robustness
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
meshtastic/tests/test_ble_interface.py (1)
89-98: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the bounded-teardown contract.
The patched
BLEClient.async_awaitraisesFutureTimeoutErrorfor every call. It does not verify thetimeoutkeyword. This test passes even ifBLEClient.disconnect()omitsBLE_DISCONNECT_TIMEOUTor uses the wrong value.Capture the mock and assert its
timeoutargument. Add a separate test forBLEClient.close()that covers the bounded join and callback-thread path.Suggested assertion
-from ..ble_interface import BLEClient, BLEInterface +from ..ble_interface import BLEClient, BLEInterface, BLE_DISCONNECT_TIMEOUT ... - with patch.object(BLEClient, "async_await", side_effect=FutureTimeoutError()): + with patch.object( + BLEClient, "async_await", side_effect=FutureTimeoutError() + ) as async_await: client.disconnect() + assert async_await.call_args.kwargs["timeout"] == BLE_DISCONNECT_TIMEOUT🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meshtastic/tests/test_ble_interface.py` around lines 89 - 98, Update test_ble_client_disconnect_swallows_stalled_teardown to capture the patched BLEClient.async_await mock and assert disconnect invokes it with timeout=BLE_DISCONNECT_TIMEOUT. Add a separate BLEClient.close test covering the bounded join and callback-thread path, including verification that teardown uses the configured timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@meshtastic/ble_interface.py`:
- Around line 310-318: Update the BLEClient async_await/disconnect teardown flow
so the submitted future is explicitly cancelled when the bounded wait times out,
before logging and returning. Ensure the timeout path still preserves
BLEInterface.close()’s non-blocking behavior while preventing
bleak_client.disconnect() from remaining queued or running.
- Around line 335-337: The BLE disconnect teardown must not join
BLEClient._eventThread when invoked by its own callback. Update
BLEInterface.disconnect/close and the self._eventThread.join path to detect the
event-loop owner thread and perform teardown asynchronously or skip synchronous
waiting, while preserving normal bounded joining from other threads; add
coverage for the disconnected_callback path.
---
Nitpick comments:
In `@meshtastic/tests/test_ble_interface.py`:
- Around line 89-98: Update test_ble_client_disconnect_swallows_stalled_teardown
to capture the patched BLEClient.async_await mock and assert disconnect invokes
it with timeout=BLE_DISCONNECT_TIMEOUT. Add a separate BLEClient.close test
covering the bounded join and callback-thread path, including verification that
teardown uses the configured timeout.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ffcdc552-1d6d-4d4d-bcac-e5ec6f29bd27
📒 Files selected for processing (2)
meshtastic/ble_interface.pymeshtastic/tests/test_ble_interface.py
| # The event loop thread is a daemon; if it fails to stop promptly we | ||
| # must not block the caller forever, so join with a bounded timeout. | ||
| self._eventThread.join(timeout=BLE_DISCONNECT_TIMEOUT) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)meshtastic/ble_interface\.py$|pyproject|requirements|poetry|setup' || true
echo
echo "ble_interface outline:"
ast-grep outline meshtastic/ble_interface.py --view compact || true
echo
echo "Relevant function excerpts:"
grep -n "def connect\|def disconnect\|def close\|_eventThread\|disconnected_callback\|BLE_DISCONNECT_TIMEOUT" meshtastic/ble_interface.py || true
echo
echo "Line ranges:"
nl -ba meshtastic/ble_interface.py | sed -n '300,380p'
nl -ba meshtastic/ble_interface.py | sed -n '1,120p'
nl -ba meshtastic/ble_interface.py | sed -n '120,220p'Repository: meshtastic/python
Length of output: 1112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Line 1-130:"
cat -n meshtastic/ble_interface.py | sed -n '1,130p'
echo
echo "Line 180-350:"
cat -n meshtastic/ble_interface.py | sed -n '180,350p'
echo
echo "Bleak versions:"
rg -n "bleak|Bleak|Dis" poetry.lock pyproject.toml || trueRepository: meshtastic/python
Length of output: 14707
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "BLEClient async helpers:"
cat -n meshtastic/ble_interface.py | sed -n '345,430p'
echo
echo "asyncio docs for Thread.join current thread in Python:"
python3 - <<'PY'
import threading
import asyncio
eventLoop = asyncio.new_event_loop()
asyncio.set_event_loop(eventLoop)
eventThread = threading.Thread(target=eventLoop.run_forever)
eventThread.start()
thread = threading.current_thread()
try:
thread.join(timeout=0.001)
except Exception as exc:
print(type(exc).__name__, str(exc))
else:
print("no exception")
finally:
eventLoop.call_soon_threadsafe(eventLoop.stop)
eventThread.join(1)
eventLoop.close()
PY
echo
echo "Bleak 0.22.3 callback scheduling references available:"
python3 - <<'PY'
import urllib.request
urls = [
"https://raw.githubusercontent.com/hbldh/bleak/v0.22.3/bleak/__init__.py",
"https://raw.githubusercontent.com/hbldh/bleak/v1.1.1/bleak/__init__.py",
]
for url in urls:
print("URL", url)
try:
text = urllib.request.urlopen(url, timeout=20).read().decode()
for i, line in enumerate(text.splitlines(), 1):
if "disconnected_callback" in line or "run_coroutine_threadsafe" in line or "disconnection_callback" in line:
lo=max(1,i-3); hi=min(len(text.splitlines()), i+5)
lines=text.splitlines()
print(f"line {i}: {lines[i-1]}")
print("\n".join([f"{j}: {lines[j-1]}" for j in range(lo, hi+1) if lines[j-1].strip()]))
print()
except Exception as exc:
print("ERROR", exc)
PY
echo
echo "Bleak 1.1.1 relevant source snippets:"
tmp="$(mktemp -d)"
pip install bleach==1.1.1 -q
python3 - <<'PY'
import inspect
import pkgutil
import uuid
mod = __import__('bleak', fromlist=['__version__'])
print("bleak.__version__", getattr(mod, "__version__", "unknown"))
for name in ["BleakClient"]:
obj = getattr(mod, name, None)
if obj:
print(name)
for src_line in inspect.getsourcelines(obj)[0]:
if "disconnected_callback" in src_line or "run_coroutine_threadsafe" in src_line:
print(src_line.rstrip())
PY
rm -rf "$tmp"Repository: meshtastic/python
Length of output: 3067
🌐 Web query:
Bleak 0.22.3 BleakClient disconnected_callback run_coroutine_threadsafe source
💡 Result:
In Bleak 0.22.3, the disconnected_callback provided to the BleakClient constructor is scheduled in the asyncio event loop when a device disconnects [1][2]. Because operating system-level disconnection events often occur on background threads, Bleak handles the synchronization internally [3]. Key technical details regarding this mechanism include: 1. Thread Safety: When an OS-level disconnect event is received on a background thread, Bleak uses methods like call_soon_threadsafe() internally to ensure that the user's provided disconnected_callback is executed on the correct asyncio event loop thread [4][5][3]. Users generally do not need to manually call run_coroutine_threadsafe when passing a standard callable as the disconnected_callback. 2. Callback Requirements: The disconnected_callback must be a synchronous callable that accepts one argument: the BleakClient instance itself [1][6]. If you attempt to provide an async function (coroutine) as the callback, it will not be awaited automatically and will likely result in a RuntimeWarning [3]. 3. Version 0.22.3 Fixes: Version 0.22.3 specifically addressed an issue where the object passed to the disconnected_callback was not correctly identified as the BleakClient instance in certain backends, ensuring the callback receives the expected client object [4][5]. If you need to perform asynchronous operations (such as awaiting a coroutine) within your disconnect logic, you should wrap the asynchronous task inside your synchronous disconnected_callback using asyncio.create_task() or, if calling from outside the event loop, asyncio.run_coroutine_threadsafe() to bridge the threads safely [7][3].
Citations:
- 1: https://bleak.readthedocs.io/en/stable/api/client.html
- 2: https://bleak.readthedocs.io/en/latest/%5Fmodules/bleak/backends/client.html
- 3: disconnect callback code do not match documentation hbldh/bleak#1301
- 4: https://github.com/hbldh/bleak/blob/HEAD/CHANGELOG.rst
- 5: https://github.com/hbldh/bleak/blob/15945c898d9a544dfe22e40a5f800d24238fd1c3/CHANGELOG.rst
- 6: https://bleak.readthedocs.io/en/latest/api/client.html
- 7: https://www.reddit.com/r/learnpython/comments/ofyjw3/asyncio_with_threading_and_bleak/
🌐 Web query:
Bleak 0.22.3 BleakClient __init__ disconnected_callback source call_soon_threadsafe asyncio
💡 Result:
In Bleak version 0.22.3, the disconnected_callback mechanism is designed to bridge events from backend threads to the asyncio event loop [1][2]. When you provide a disconnected_callback to the BleakClient constructor, Bleak wraps it using functools.partial to ensure the callback receives the BleakClient instance as its first argument [3]. Because the underlying OS-level Bluetooth events often occur on background threads, Bleak must perform thread-safe scheduling to execute your callback within the proper asyncio event loop [1][2][4]. Specifically, in version 0.22.3, a fix was implemented to ensure that the object passed to the disconnected_callback is the BleakClient instance itself, rather than the internal backend object [1][2]. Key technical points regarding this implementation: - Thread-Safety: Bleak uses call_soon_threadsafe (or equivalent mechanisms in backend-specific code) to schedule the callback execution on the main event loop from the background thread where the disconnection event originated [1][2][4]. - Callback Signature: The disconnected_callback must be a synchronous callable that accepts a single argument, which is the BleakClient object [5][6][7]. It cannot be an async function (coroutine), as the executor will not await it, leading to a RuntimeWarning [4]. - Implementation detail: The wrapper logic in bleak/init.py creates a partial function: self._backend = PlatformBleakClient(..., disconnected_callback=(None if disconnected_callback is None else functools.partial(disconnected_callback, self)),...) [3]. This ensures that when the backend triggers the callback, the user's function is invoked with the expected client reference [1][2].
Citations:
- 1: https://github.com/hbldh/bleak/blob/HEAD/CHANGELOG.rst
- 2: https://github.com/hbldh/bleak/blob/a778e1a2edf62e54b9e4894c9a8e27d79c2c3693/CHANGELOG.rst
- 3: https://github.com/hbldh/bleak/blob/develop/bleak/__init__.py
- 4: disconnect callback code do not match documentation hbldh/bleak#1301
- 5: https://bleak.readthedocs.io/en/stable/api/client.html
- 6: https://bleak.readthedocs.io/en/latest/api/client.html
- 7: https://bleak.readthedocs.io/en/develop/api/client.html
🌐 Web query:
Bleak backend disconnect callback call_soon call_soon_threadsafe source 0.22.3
💡 Result:
In Bleak, the disconnected_callback is scheduled within the asyncio event loop to ensure it runs on the main event loop thread, even when the underlying OS-level disconnection signal arrives on a background thread [1]. When an unsolicited disconnection occurs, the Bleak backend uses a mechanism to safely invoke the user-provided callback [1]. Specifically, Bleak uses call_soon_threadsafe (or sometimes a wrapper that ensures proper scheduling) to bridge the gap between the background thread (often where native Bluetooth OS APIs report events) and the asyncio event loop [2][1]. This ensures that your callback logic is executed in a thread-safe manner within the event loop's context [1]. Key points regarding this mechanism in Bleak (including version 0.22.3 and surrounding releases): 1. Implementation: When the operating system notifies the backend of a disconnect, Bleak's internal delegate or monitor detects this event [3]. It then schedules the execution of the user's callback using loop.call_soon_threadsafe to ensure it is handled correctly by the asyncio loop [2][1]. 2. Callback Requirements: The callback function must be a standard, synchronous callable that accepts the client object as its sole argument [4][5][1]. Because Bleak schedules this callback in the event loop, you cannot pass an async function (coroutine) directly; attempting to do so will result in a RuntimeWarning because the coroutine will not be awaited [1]. 3. Purpose of Thread Safety: Since native Bluetooth stacks (such as CoreBluetooth on macOS or others) often operate on their own threads, call_soon_threadsafe is critical for preventing race conditions and maintaining the integrity of the asyncio event loop [2][1]. If you are experiencing issues with the callback not firing or errors during the callback execution, ensure your callback is not an async function and does not perform blocking operations directly, as it runs on the event loop thread [1]. If you need to perform asynchronous work inside the callback, you should use loop.call_soon or create a task from within the callback to schedule that work properly.
Citations:
- 1: disconnect callback code do not match documentation hbldh/bleak#1301
- 2: hbldh/bleak@848b511
- 3: https://bleak.readthedocs.io/en/latest/_modules/bleak/backends/corebluetooth/client.html
- 4: https://bleak.readthedocs.io/en/stable/%5Fmodules/bleak/backends/client.html
- 5: https://bleak.readthedocs.io/en/latest/api/client.html
Do not block or join BLEClient._eventThread on the BLE event-loop thread.
BLEInterface.connect passes lambda _: self.close() as the Bleak disconnected_callback. Unsolicited disconnects are delivered through Bleak’s callback path, so self.client.close() can run on BLEClient._eventThread. The subsequent self._eventThread.join(...) then fails with RuntimeError: cannot join current thread, and cleanup can return before self.client = None and _disconnected() run. Move the teardown path to a non-loop thread, or make disconnect() / close() detect the owner thread and avoid synchronous waiting and self-joining. Add coverage for this disconnect-callback path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@meshtastic/ble_interface.py` around lines 335 - 337, The BLE disconnect
teardown must not join BLEClient._eventThread when invoked by its own callback.
Update BLEInterface.disconnect/close and the self._eventThread.join path to
detect the event-loop owner thread and perform teardown asynchronously or skip
synchronous waiting, while preserving normal bounded joining from other threads;
add coverage for the disconnected_callback path.
Follow-up to the teardown-hang fix: when the bounded disconnect wait times out, cancel the future so a stalled call isn't left running on the event loop. Adds tests for the timeout, the bounded close() join, and the cancel. Signed-off-by: Laura Long <laura@longgone.dev>
|
Thanks for the review! Addressed both points in 7f44a5f: the disconnect future is now cancelled on timeout, with tests added (assert the bounded timeout is passed, cover the close() join, and cover the cancel path). The self-join in the disconnect callback is real but pre-existing and separate from this fix, so I've left it out to keep the PR focused and opened #968 to track it. |
7f44a5f to
2a47f6e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
2 participants
What's wrong
BLEInterface.close()can hang forever. I hit this on Linux (Python 3.14, bleak 2.1.1) with a RAK4631: a--blesession connects and downloads config fine, then never exits. There are two issues in the teardown path:BLEClient.disconnect()awaits bleak withfuture.result(None), andBLEClient.close()joins the event-loop thread with a plainjoin(). If bleak's disconnect stalls (it does on my setup),close()blocks indefinitely and the process wedges.UnboundLocalErroron a mid-read disconnect. In_receiveFromRadioImpl, ifread_gatt_charraisesBleakDBusError(device dropped),bis never assigned, so the followingif not b:raisesUnboundLocalError: cannot access local variable 'b'instead of ending the read loop cleanly. This one can bite anyone whose node drops mid-read, I believe on any platform.The fix
bbefore the read so a mid-read disconnect unwinds through the normal path.BLE_DISCONNECT_TIMEOUT(5s) and apply it toBLEClient.disconnect()(swallowing a timeout /BleakError, since teardown is best-effort) and toBLEClient.close()'s daemon-threadjoin().This is deliberately the same "if bleak is hung, don't wait" approach already used on the receive-thread join in
BLEInterface.close(). I just extended it to the two remaining unbounded waits. The change is additive: if disconnect completes normally the timeout never fires and behavior is unchanged; it only kicks in to stop an otherwise-infinite hang.Testing
pytest meshtastic/tests/test_ble_interface.pypasses, including two new regression tests (one for theUnboundLocalError, one for the disconnect timeout being swallowed).pylint meshtastic/ble_interface.py-> 10.00/10;mypyclean.--ble --infoand a polling logger now exit cleanly instead of hanging.Related
This looks like the same teardown hang reported in #817 (the CLI not terminating after a
--bleoperation), and may also help #491 and #909 (related Linux--blehangs where the command completes but never exits). Leaving it to a maintainer to confirm before closing any of them.Possibly related teardown/cleanup reports (not claiming an identical root cause): #49, #534 - both about connections not closing cleanly so later calls fail.
Full disclosure: this is my own bug on my own hardware and I verified the fix here, but I used AI assistance, so I'd really appreciate a couple of extra sets of eyes on this.
Summary by CodeRabbit