Consume MQTT ack futures on abandonment to prevent asyncio warnings#98
Merged
Conversation
Fixes #97. _await_ack() (mqtt/connection.py) and the inline subscribe/unsubscribe waits (mqtt/subscriptions.py) shield the AWS CRT acknowledgement future so a timeout/cancellation never propagates into the SDK future. If that shielded future later completed with an exception (e.g. AwsCrtError from clean-session cancellation during reconnect) after the caller had already given up, nobody retrieved the exception and asyncio logged an unhandled 'Future exception was never retrieved' / 'exception in shielded future' warning at garbage-collection time. Attach a done callback whenever a wait is abandoned (cancelled or timed out) so the eventual result/exception is retrieved and logged at debug level, instead of leaking as an asyncio-level warning that integrations had to suppress globally.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR prevents noisy asyncio warnings caused by MQTT acknowledgement futures that outlive their awaiting task (timeout/cancellation), by ensuring the eventual result/exception is still retrieved and logged instead of being left for GC to report as “Future exception was never retrieved”.
Changes:
- Add
_consume_abandoned_future()and wire it into_await_ack()cancellation/timeout paths to consume late exceptions. - Apply the same “consume abandoned ack future” pattern to
subscribe()/unsubscribe()acknowledgement waits (and add missing timeout handling there). - Add regression tests covering both timeout and cancellation abandonment scenarios, and document the fix in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/nwp500/mqtt/connection.py |
Adds consumption of late ack future exceptions when _await_ack() is abandoned. |
src/nwp500/mqtt/subscriptions.py |
Extends the abandonment-consumption pattern to subscribe/unsubscribe ack waits and adds timeout handling. |
tests/test_mqtt_reliability.py |
Adds regression tests to ensure abandoned ack futures don’t leak asyncio warnings. |
CHANGELOG.rst |
Documents the fix under Unreleased. |
Use functools.partial with the static _consume_abandoned_future helper referenced via the class, rather than a lambda closing over self, so an abandoned subscribe/unsubscribe ack future doesn't keep the MqttSubscriptionManager instance alive until it eventually completes.
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.
Fixes #97
Problem
_await_ack()inmqtt/connection.py(used by connect/disconnect/close/subscribe/unsubscribe/publish) and the inline acknowledgement waits inmqtt/subscriptions.py'ssubscribe()/unsubscribe()shield the AWS CRT future so a timeout or task cancellation never propagates into the SDK future — the underlying operation keeps running in the background.If that shielded future later completes with an exception (notably
AwsCrtError/AWS_ERROR_MQTT_CANCELLED_FOR_CLEAN_SESSIONduring a reconnect race) after the awaiting task has already given up, nobody ever retrieves the exception. asyncio then logs an unhandledFuture exception was never retrieved/exception in shielded futurewarning at garbage-collection time, which is exactly the noise that the Home Assistant integration had to suppress with a global loop exception handler.Fix
Attach a
done_callbackto the shielded future whenever a wait is abandoned (cancelled or timed out) so its eventual result/exception is always retrieved. Benign errors are logged at debug level instead of leaking as an unhandled asyncio warning.mqtt/connection.py: new_consume_abandoned_future()helper, wired into both theCancelledErrorandTimeoutErrorbranches of_await_ack().mqtt/subscriptions.py: same pattern applied to the inlinesubscribe()/unsubscribe()waits (also added the previously-missingTimeoutErrorhandling/logging there).Testing
TestAbandonedAckFutureConsumedregression tests intests/test_mqtt_reliability.pycovering both the timeout and cancellation paths. Verified these tests fail (asyncio logs "exception in shielded future") without the fix and pass with it.ruff check/ruff format --check: passmypy src/nwp500: no issuespytest: 611/611 passed