-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Deprecate connect and disconnect methods #247
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #247 +/- ##
=======================================
- Coverage 84.7% 84.4% -0.4%
=======================================
Files 4 4
Lines 473 483 +10
Branches 88 90 +2
=======================================
+ Hits 401 408 +7
- Misses 45 47 +2
- Partials 27 28 +1
☔ View full report in Codecov by Sentry. |
Ready for review now, I pulled in the functionality from Let me know what you think! 🙂 |
rc = self._client.disconnect() | ||
if rc == mqtt.MQTT_ERR_SUCCESS: | ||
# Wait for acknowledgement | ||
await self._wait_for(self._disconnected, timeout=None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it times out here, the lock will not be released, so my suggestion is to change the release lock to be executed anyway in aexit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking the changes, and good point! 😊 If we release the lock, we can then call __aenter__
while the client is (potentially) still connected. I'm not sure what's best here.
I aimed for having equal logic to before in this PR. I hope that we can simplify the internal state when we remove connect
and disconnect
, so that such things are clearer. Right now it's slightly confusing having _connected
, _disconnected
, and _lock
and what these three should specify.
c76ef7a
to
d83de6e
Compare
Once connected and and Assuming no new messages are coming in that for loop isn't going to be useful in this situation. Is there a special / sentinel message I can somehow stuff onto the queue to get the desired result? If not, is this something the team would consider? |
I did not test this now, but I'd expect the client to do a graceful shutdown when you cancel the surrounding task. So something like: async def test():
with aiomqtt.Client("test.mosquitto.org") as client:
# Subscribe to topics here
async for message in client.messages:
# Process message
async def main():
task = asyncio.create_task(test())
# Do something else
await asyncio.sleep(1)
# Oh no, error!
task.cancel()
try:
await task
except asyncio.CancelledError:
pass Hope that helps! |
As previously discussed in #216, this deprecates calls to
connect
anddisconnect
in favor of__aenter__
and__aexit__
.Hopefully, this will also simplify the internal state in the future. On that matter, Frederik, could you elaborate on what you meant by "Have all internal state inside a tagged union (sum type) in rust-style, concurrency-safe semantics."?