Skip to content

Commit ff58e1c

Browse files
authored
Merge pull request #79 from mwolter805/fix/standalone-bugs-and-cleanup
fix: remove broken req_mma, bump DEFAULT_TIMEOUT, guard TypeError, pre-register binary requests
2 parents 5032f81 + fda191d commit ff58e1c

6 files changed

Lines changed: 268 additions & 24 deletions

File tree

src/meshcore/commands/base.py

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ class CommandHandlerBase:
6666
Python 3.9/3.10 compatibility).
6767
"""
6868

69-
DEFAULT_TIMEOUT = 5.0
69+
DEFAULT_TIMEOUT = 15.0
7070

7171
def __init__(self, default_timeout: Optional[float] = None):
7272
self._sender_func: Optional[Callable[[bytes], Coroutine[Any, Any, None]]] = None
@@ -270,18 +270,31 @@ async def send_binary_req(self, dst: DestinationType, request_type: BinaryReqTyp
270270
logger.debug(f"Binary request to {dst_bytes.hex()}")
271271
data = b"\x32" + dst_bytes + request_type.value.to_bytes(1, "little", signed=False) + (data if data else b"")
272272

273-
result = await self.send(data, [EventType.MSG_SENT, EventType.ERROR])
274-
275-
# Register the request with the reader if we have both reader and request_type
276-
if (result.type == EventType.MSG_SENT and
277-
self._reader is not None and
278-
request_type is not None):
279-
280-
exp_tag = result.payload["expected_ack"].hex()
281-
# Use provided timeout or fallback to suggested timeout (with 5s default)
282-
actual_timeout = timeout if timeout is not None and timeout > 0 else result.payload.get("suggested_timeout", 4000) / 800.0
273+
# Pre-register a placeholder binary request before send() to close the race
274+
# window where a BINARY_RESPONSE could arrive between send() returning and
275+
# registration. The placeholder tag is patched to the real tag once MSG_SENT
276+
# returns. If send() fails, the placeholder is cleaned up.
277+
placeholder_tag = None
278+
if self._reader is not None and request_type is not None:
279+
placeholder_tag = f"_pending_{id(data)}"
280+
actual_timeout = timeout if timeout is not None and timeout > 0 else self.default_timeout
283281
actual_timeout = min_timeout if actual_timeout < min_timeout else actual_timeout
284-
self._reader.register_binary_request(pubkey_prefix.hex(), exp_tag, request_type, actual_timeout, context=context)
282+
self._reader.register_binary_request(pubkey_prefix.hex(), placeholder_tag, request_type, actual_timeout, context=context)
283+
284+
result = await self.send(data, [EventType.MSG_SENT, EventType.ERROR])
285+
286+
# Patch the placeholder tag with the real tag from MSG_SENT, or clean up on failure
287+
if placeholder_tag is not None and self._reader is not None:
288+
# Remove the placeholder entry
289+
self._reader.pending_binary_requests.pop(placeholder_tag, None)
290+
if (result.type == EventType.MSG_SENT and
291+
request_type is not None):
292+
exp_tag = result.payload["expected_ack"].hex()
293+
# Use suggested_timeout from the result if available
294+
actual_timeout = timeout if timeout is not None and timeout > 0 else result.payload.get("suggested_timeout", 4000) / 800.0
295+
actual_timeout = min_timeout if actual_timeout < min_timeout else actual_timeout
296+
# Register with the real tag
297+
self._reader.register_binary_request(pubkey_prefix.hex(), exp_tag, request_type, actual_timeout, context=context)
285298

286299
return result
287300

src/meshcore/commands/binary.py

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -73,10 +73,6 @@ async def req_telemetry_sync(self, contact, timeout=0, min_timeout=0):
7373

7474
return telem_event.payload["lpp"] if telem_event else None
7575

76-
async def req_mma(self, contact, timeout=0, min_timeout=0):
77-
logger.error("*** please consider using req_mma_sync instead of req_mma")
78-
return await self.req_mma_sync(contact, start, end, timeout,min_timeout)
79-
8076
async def req_mma_sync(self, contact, start, end, timeout=0,min_timeout=0):
8177
async with self._mesh_request_lock:
8278
req = (

src/meshcore/commands/contact.py

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,17 @@ async def get_contacts(self, lastmod=0, timeout=5) -> Event:
4343
logger.debug("Timeout while getting contacts")
4444
for future in pending: # cancel all futures
4545
future.cancel()
46-
return None
46+
return Event(EventType.ERROR, {"reason": "timeout waiting for contacts"})
4747

4848
for future in done:
4949
event = await future
50-
if event is None or event.type != EventType.NEXT_CONTACT:
51-
for future in pending:
52-
future.cancel()
50+
if event is None:
51+
for f in pending:
52+
f.cancel()
53+
return Event(EventType.ERROR, {"reason": "no event received during contacts retrieval"})
54+
if event.type != EventType.NEXT_CONTACT:
55+
for f in pending:
56+
f.cancel()
5357
return event
5458

5559
futures = []
@@ -64,7 +68,7 @@ async def get_contacts(self, lastmod=0, timeout=5) -> Event:
6468

6569
except asyncio.TimeoutError:
6670
logger.debug(f"Timeout receiving contacts")
67-
return None
71+
return Event(EventType.ERROR, {"reason": "asyncio timeout receiving contacts"})
6872
except Exception as e:
6973
logger.debug(f"Command error: {e}")
7074
return Event(EventType.ERROR, {"error": str(e)})
@@ -116,7 +120,9 @@ async def update_contact(self, contact, path=None, flags=None, path_hash_mode=No
116120
path_hash_mode = int(path.split(":")[1])
117121
path = path.split(":")[0].replace(":","")
118122
else: # use device one by default
119-
path_hash_mode = contact["out_path_len"] >> 6 # would fallback to previous val
123+
# out_path_len is pre-masked (& 0x3F) in reader.py, so high bits are always 0;
124+
# the actual path_hash_mode is fetched from the device query below.
125+
path_hash_mode = 0
120126
res = await self.send_device_query()
121127
if not res is None and res.type != EventType.ERROR:
122128
if "path_hash_mode" in res.payload:

src/meshcore/commands/messaging.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,8 @@ async def set_flood_scope(self, scope):
339339
elif isinstance (scope, bytes): # scope has been sent directly as byte
340340
logger.debug(f"Directly setting scope to {scope}")
341341
scope_key = scope
342+
else:
343+
raise TypeError(f"set_flood_scope: unsupported scope type {type(scope).__name__}")
342344

343345
logger.debug(f"Setting scope to {scope_key.hex()}")
344346

src/meshcore/meshcore.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import asyncio
22
import logging
3-
from typing import Any, Callable, Coroutine, Dict, Optional, Union
3+
from typing import Any, Callable, Dict, Optional, Union
44

55
from .events import Event, EventDispatcher, EventType, Subscription
66
from .reader import MessageReader
@@ -222,7 +222,7 @@ def stop(self):
222222
def subscribe(
223223
self,
224224
event_type: Union[EventType, None],
225-
callback: Callable[[Event], Coroutine[Any, Any, None]],
225+
callback: Callable[[Event], Union[None, asyncio.Future]],
226226
attribute_filters: Optional[Dict[str, Any]] = None,
227227
) -> Subscription:
228228
"""
Lines changed: 227 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,227 @@
1+
"""
2+
Verification tests for standalone bug fixes and cleanup.
3+
"""
4+
5+
import pytest
6+
import asyncio
7+
import inspect
8+
from unittest.mock import AsyncMock, MagicMock, patch
9+
10+
from meshcore.events import Event, EventDispatcher, EventType
11+
from meshcore.commands.base import CommandHandlerBase
12+
from meshcore.commands.binary import BinaryCommandHandler
13+
from meshcore.commands.messaging import MessagingCommands
14+
from meshcore.commands.contact import ContactCommands
15+
from meshcore.meshcore import MeshCore
16+
17+
pytestmark = pytest.mark.asyncio
18+
19+
20+
# ── req_mma removed ──────────────────────────────────────────────────────
21+
22+
def test_req_mma_removed():
23+
"""The broken req_mma method should no longer exist on BinaryCommandHandler."""
24+
assert not hasattr(BinaryCommandHandler, "req_mma"), \
25+
"req_mma should be removed — it had NameError on undefined start/end"
26+
27+
28+
def test_req_mma_sync_still_exists():
29+
"""req_mma_sync should still be present and functional."""
30+
assert hasattr(BinaryCommandHandler, "req_mma_sync"), \
31+
"req_mma_sync should still exist after removing req_mma"
32+
33+
34+
# ── DEFAULT_TIMEOUT bumped ───────────────────────────────────────────────
35+
36+
def test_default_timeout_bumped():
37+
"""DEFAULT_TIMEOUT should be 15.0, not the old 5.0."""
38+
assert CommandHandlerBase.DEFAULT_TIMEOUT == 15.0, \
39+
f"DEFAULT_TIMEOUT is {CommandHandlerBase.DEFAULT_TIMEOUT}, expected 15.0"
40+
41+
42+
def test_instance_default_timeout():
43+
"""Instance default_timeout should inherit the new 15.0 value."""
44+
handler = CommandHandlerBase()
45+
assert handler.default_timeout == 15.0
46+
47+
48+
def test_custom_timeout_still_works():
49+
"""Passing a custom timeout should still override the default."""
50+
handler = CommandHandlerBase(default_timeout=30.0)
51+
assert handler.default_timeout == 30.0
52+
53+
54+
# ── set_flood_scope TypeError guard ──────────────────────────────────────
55+
56+
async def test_set_flood_scope_bad_type_raises():
57+
"""Passing an unsupported type (e.g., int) should raise TypeError."""
58+
handler = MessagingCommands()
59+
with pytest.raises(TypeError, match="unsupported scope type"):
60+
await handler.set_flood_scope(42)
61+
62+
63+
async def test_set_flood_scope_bad_type_bytearray():
64+
"""bytearray is not bytes — should raise TypeError."""
65+
handler = MessagingCommands()
66+
with pytest.raises(TypeError, match="unsupported scope type"):
67+
await handler.set_flood_scope(bytearray(b"\x00" * 16))
68+
69+
70+
async def test_set_flood_scope_none_still_works():
71+
"""None scope should reach send() without TypeError — verifies the None branch still binds scope_key."""
72+
handler = MessagingCommands()
73+
handler._sender_func = AsyncMock()
74+
handler.dispatcher = EventDispatcher()
75+
await handler.dispatcher.start()
76+
try:
77+
# Dispatch an OK event so send() resolves
78+
async def _dispatch_ok():
79+
await asyncio.sleep(0.05)
80+
await handler.dispatcher.dispatch(Event(EventType.OK, {}))
81+
asyncio.ensure_future(_dispatch_ok())
82+
result = await handler.set_flood_scope(None)
83+
assert result.type == EventType.OK
84+
finally:
85+
handler.dispatcher.running = False
86+
87+
88+
async def test_set_flood_scope_str_still_works():
89+
"""String scope should reach send() without TypeError."""
90+
handler = MessagingCommands()
91+
handler._sender_func = AsyncMock()
92+
handler.dispatcher = EventDispatcher()
93+
await handler.dispatcher.start()
94+
try:
95+
async def _dispatch_ok():
96+
await asyncio.sleep(0.05)
97+
await handler.dispatcher.dispatch(Event(EventType.OK, {}))
98+
asyncio.ensure_future(_dispatch_ok())
99+
result = await handler.set_flood_scope("#test")
100+
assert result.type == EventType.OK
101+
finally:
102+
handler.dispatcher.running = False
103+
104+
105+
async def test_set_flood_scope_bytes_still_works():
106+
"""Bytes scope should reach send() without TypeError."""
107+
handler = MessagingCommands()
108+
handler._sender_func = AsyncMock()
109+
handler.dispatcher = EventDispatcher()
110+
await handler.dispatcher.start()
111+
try:
112+
async def _dispatch_ok():
113+
await asyncio.sleep(0.05)
114+
await handler.dispatcher.dispatch(Event(EventType.OK, {}))
115+
asyncio.ensure_future(_dispatch_ok())
116+
result = await handler.set_flood_scope(b"\x01" * 16)
117+
assert result.type == EventType.OK
118+
finally:
119+
handler.dispatcher.running = False
120+
121+
122+
# ── dead path_hash_mode shift removed ────────────────────────────────────
123+
124+
def test_no_shift_in_update_contact():
125+
"""The dead `>> 6` shift on out_path_len should not appear in contact.py."""
126+
import meshcore.commands.contact as contact_mod
127+
source = inspect.getsource(contact_mod.ContactCommands.update_contact)
128+
assert ">> 6" not in source, \
129+
"Dead path_hash_mode = out_path_len >> 6 shift should be removed"
130+
131+
132+
# ── get_contacts returns Event, never None ───────────────────────────────
133+
134+
async def test_get_contacts_timeout_returns_error_event():
135+
"""On timeout (no futures complete), get_contacts should return an Error Event, not None."""
136+
handler = ContactCommands()
137+
handler._sender_func = AsyncMock()
138+
handler._reader = MagicMock()
139+
handler.dispatcher = MagicMock()
140+
# Make wait_for_event always timeout by never returning
141+
handler.dispatcher.wait_for_event = AsyncMock(side_effect=asyncio.TimeoutError)
142+
143+
result = await handler.get_contacts(timeout=0.1)
144+
assert result is not None, "get_contacts should never return None"
145+
assert isinstance(result, Event)
146+
assert result.type == EventType.ERROR
147+
148+
149+
# ── binary request pre-registration ──────────────────────────────────────
150+
151+
async def test_placeholder_registered_before_send():
152+
"""A placeholder binary request should be registered before send() is called."""
153+
from meshcore.packets import BinaryReqType
154+
155+
handler = CommandHandlerBase()
156+
handler._sender_func = AsyncMock()
157+
158+
# Track registration calls
159+
mock_reader = MagicMock()
160+
mock_reader.pending_binary_requests = {}
161+
original_register = MagicMock()
162+
163+
registration_order = []
164+
send_called = False
165+
166+
async def mock_send(data):
167+
nonlocal send_called
168+
# At the point send() is called, a placeholder should already exist
169+
registration_order.append(("send", len(mock_reader.pending_binary_requests)))
170+
send_called = True
171+
172+
handler._sender_func = mock_send
173+
handler._reader = mock_reader
174+
handler.dispatcher = MagicMock()
175+
handler.dispatcher.wait_for_event = AsyncMock(
176+
return_value=Event(EventType.MSG_SENT, {"expected_ack": b"\x01\x02\x03\x04"})
177+
)
178+
179+
# Resolve subscribed events immediately so send() doesn't block.
180+
# Use MSG_SENT with expected_ack because send_binary_req reads that key.
181+
def resolving_subscribe(event_type, cb, attribute_filters=None):
182+
sub = MagicMock()
183+
sub.unsubscribe = MagicMock()
184+
payload = {"expected_ack": b"\x01\x02\x03\x04"} if event_type == EventType.MSG_SENT else {}
185+
asyncio.get_event_loop().call_soon(
186+
cb, Event(event_type, payload)
187+
)
188+
return sub
189+
handler.dispatcher.subscribe = MagicMock(side_effect=resolving_subscribe)
190+
191+
# Call send_binary_req
192+
dst = "aa" * 32 # 32-byte hex pubkey
193+
await handler.send_binary_req(dst, BinaryReqType.MMA)
194+
195+
# Verify register_binary_request was called (at least the placeholder)
196+
assert mock_reader.register_binary_request.call_count >= 1, \
197+
"register_binary_request should be called at least once for the placeholder"
198+
199+
200+
# ── MeshCore.subscribe annotation matches EventDispatcher ────────────────
201+
202+
def test_subscribe_annotation_matches_dispatcher():
203+
"""MeshCore.subscribe callback annotation should match EventDispatcher.subscribe."""
204+
mc_hints = MeshCore.subscribe.__annotations__
205+
ed_hints = EventDispatcher.subscribe.__annotations__
206+
207+
# Both should have 'callback' in their annotations
208+
assert "callback" in mc_hints, "MeshCore.subscribe missing callback annotation"
209+
assert "callback" in ed_hints, "EventDispatcher.subscribe missing callback annotation"
210+
211+
# The callback annotations should be identical
212+
assert mc_hints["callback"] == ed_hints["callback"], (
213+
f"MeshCore.subscribe callback annotation {mc_hints['callback']} "
214+
f"does not match EventDispatcher.subscribe {ed_hints['callback']}"
215+
)
216+
217+
218+
def test_no_coroutine_import_in_meshcore():
219+
"""After widening the annotation, Coroutine should no longer be imported in meshcore.py."""
220+
import meshcore.meshcore as mc_mod
221+
source = inspect.getsource(mc_mod)
222+
# Check the import line specifically — Coroutine should not be in the typing imports
223+
for line in source.splitlines():
224+
if line.startswith("from typing import"):
225+
assert "Coroutine" not in line, \
226+
"Coroutine should be removed from typing imports in meshcore.py"
227+
break

0 commit comments

Comments
 (0)