Skip to content
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

fix connection connection initialization handling #274

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

4s1
Copy link

@4s1 4s1 commented Feb 5, 2024

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

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

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

fixes sbtinstruments#269
Copy link

codecov bot commented Feb 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@          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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@empicano
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Impossible to reuse client in case of connection timeout
2 participants