fix: MQTT threading model and event emitter correctness#93
Merged
Conversation
- Marshal MQTT message parse/dispatch from the AWS CRT network thread onto the asyncio event loop; keeps the SDK callback trivial, prevents blocking user callbacks from stalling MQTT processing, and eliminates the handler-registry iteration race during reconnect/resubscribe - Iterate snapshots of handler registries so handlers can subscribe/unsubscribe during dispatch - Isolate individual handler failures: catch Exception per handler with logging instead of only (TypeError, AttributeError, KeyError) - Make the unit system preference process-wide instead of a ContextVar; tasks scheduled from CRT threads never inherited the caller's context, so --unit-system was silently ignored for all MQTT-delivered data - Remove once-listeners before invoking them: raising callbacks no longer stay registered forever and concurrent emits no longer double-fire - Clean up wait_for() listeners in a finally block so cancellation does not leak them; fix wait_for docstring examples that broke user code
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability and correctness of the library’s MQTT and eventing layers by ensuring message parsing/dispatch occurs on the asyncio event loop (not the AWS CRT network thread), hardening callback isolation, and fixing EventEmitter once/wait-for race/leak behaviors. It also adjusts unit-system configuration semantics so real-time MQTT-driven model validation consistently honors the configured preference across threads/tasks.
Changes:
- Move MQTT message parsing + handler dispatch off the AWS CRT callback thread and onto the event loop, with snapshot-based registry iteration and per-handler error isolation.
- Fix EventEmitter once-listener and
wait_for()cleanup races/leaks; update doc/examples to match actualwait_for()return shape. - Replace unit-system
ContextVarstorage with a process-wide setting; add regression tests covering threading/ordering and event emitter behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_threading_model.py | Adds regression coverage for MQTT threading dispatch, handler isolation, unit-system visibility across threads, and EventEmitter once/wait_for fixes. |
| src/nwp500/unit_system.py | Changes unit-system preference storage from context-local to process-wide to work correctly with cross-thread MQTT scheduling. |
| src/nwp500/mqtt/subscriptions.py | Makes the AWS CRT message callback trivial and marshals parse/dispatch onto the event loop; dispatch iterates registry snapshots and isolates handler failures. |
| src/nwp500/mqtt_events.py | Fixes wait_for() example usage to match its actual return value (args tuple only). |
| src/nwp500/events.py | Removes once-listeners before invocation to prevent double-fire/retention on exceptions; ensures wait_for() always unregisters listeners via finally. |
| CHANGELOG.rst | Documents the fixed threading, event emitter, unit-system, and documentation issues under Unreleased. |
added 2 commits
July 5, 2026 09:36
# Conflicts: # CHANGELOG.rst
Addresses review feedback: the preference is process-wide, so a teardown-only reset let the first test in this module inherit values set by earlier tests in the session.
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.
Fourth batch from the comprehensive repo evaluation: moves MQTT message handling off the AWS CRT network thread and fixes event emitter races. Two of these bugs were verified with minimal reproductions during the evaluation.
Bugs fixed
All message handling ran on the AWS CRT network thread (
mqtt/subscriptions.py): the raw awscrt callback did JSON decode, pydanticmodel_validate, and invoked user callbacks directly on the SDK's network thread. Consequences:resubscribe_all) while the CRT thread iterated it →RuntimeError: dictionary changed size during iteration, dropping the message — most likely during the reconnect window when queued broker messages are deliveredThe CRT callback is now trivial: it marshals the raw payload onto the event loop via the existing
schedule_coroutinemechanism; parse + dispatch run there, iterating snapshots of the registries. Message ordering is preserved (call_soon_threadsafeis FIFO from a single CRT thread).One raising handler aborted delivery to the rest: dispatch caught only
(TypeError, AttributeError, KeyError); a callback raisingValueError(or anything else) skipped the remaining handlers. Failures are now logged per handler and isolated (matching the documentedEventEmitterresilience rationale).--unit-systemsilently ignored for MQTT data (unit_system.py): the preference was aContextVarset in the caller's task. Tasks scheduled from CRT threads get a fresh context, soDeviceStatuscomputed fields fell back to the device's native unit —nwp-cli --unit-system metric monitorwrote Fahrenheit values labeled °C into logs and CSVs. The preference is now process-wide (module global), visible from every task and thread. Per-task unit isolation was a documented footgun nobody could actually rely on for real-time data.Once-listeners fired more than once (
events.py, empirically verified):emit()removed once-listeners after invoking them inside thetry, so a raising callback stayed registered forever, and two overlapping emits both snapshotted the list before either removed. Removal now happens synchronously before invocation, in the same event-loop slice as the snapshot.wait_for()leaked its listener on cancellation (events.py, empirically verified): onlyTimeoutErrortriggered cleanup; a cancelled waiter left a once-listener that would later fire and set a result on a dead future. Cleanup moved tofinally.wait_for()docstring examples broke user code (events.py,mqtt_events.py): examples showedargs, _ = await wait_for(...)but it returns just the args tuple.Tests
New
tests/test_threading_model.py(13 tests): message from a foreign thread dispatches on the loop thread with parsed payload; registry mutation during dispatch is safe; raising handler doesn't block others; invalid JSON logged not raised; unit system visible in tasks scheduled from foreign threads and in plain threads; raising once-listener removed; concurrent emits single-fire;wait_forcleanup on cancellation/timeout/success.Validation
pytest: 521 passed (2 consecutive runs)ruff check/ruff format --check: cleanpyright src/nwp500: 0 errorsmypy src/nwp500: no issues