sbtinstruments / aiomqtt

The idiomatic asyncio MQTT client, wrapped around paho-mqtt
https://sbtinstruments.github.io/aiomqtt
BSD 3-Clause "New" or "Revised" License
392 stars 71 forks source link

fix connection connection initialization handling #274

Closed 4s1 closed 4 weeks ago

4s1 commented 5 months ago

Connection errors in aenter() which happen while _wait_for(selt._connected,...) were not catched, leaving a stale _lock behind, which made client non reusable.

Pull waiting for connected event into try/except block, so client is reusable in event of an error while trying to connect.

fixes #269

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (13b44fb) 85.1% compared to head (1d607c0) 85.1%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #274 +/- ## ===================================== Coverage 85.1% 85.1% ===================================== Files 6 6 Lines 425 425 Branches 83 83 ===================================== Hits 362 362 Misses 37 37 Partials 26 26 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

empicano commented 4 months ago

Hi there, thanks for working on this! 😎

As far as I understand, #269 mentioned two different exceptions. First, MqttConnectError thrown by the on_connect callback (which is handled in this PR) and second, MqttError thrown due to a timeout by wait_for (not yet handled).

Currently, the MqttConnectError would be cast to a MqttError which is a breaking change. I think it'd be better to handle and reraise both exceptions in their own try/except block, something like this (not tested):

try:
    await self._wait_for(self._connected, timeout=None)
except MqttError, MqttConnectError:
    # Release lock and reraise any exception raised waiting on CONNACK
    self._lock.release()
    raise

It'd be great if you added test cases for these as well, so we/I don't accidently regress this again. To simulate the MqttConnectError thrown by on_connect you could use client._connected.set_exception(MqttConnectError). There are a few tests that already do something similar with the _disconnected future.

empicano commented 4 weeks ago

I'll close this, because I believe this was fixed with #285.

Thank you nonetheless for this PR and your great comments on the corresponding issue!