sbtinstruments / aiomqtt

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

don't suppress exception on aexit #232

Closed edenhaus closed 11 months ago

edenhaus commented 12 months ago

With #216 the "early out on disconnected" code was move from the function __aexit__ to disconnect. The disconnect function is called inside a try/except block in the function __aexit__, which suppresses the disconnected exception. This results in that the context manager exits without exception also when there was a disconnect-exception.

Example:

expection: I get some sort of exception actual: no exception, only a log entry.

This bug was found on the upgrade from 0.16.1 to 1.0.0. Afterwards the following test was failing https://github.com/DeebotUniverse/client.py/blob/3d7467e3dcf338e9d848dd5fb3ad32a5ee7fab8e/tests/test_mqtt_client.py#L76C1-L76C1 The test test the described example above.

edenhaus commented 12 months ago

For tests I would wait until https://github.com/guillotinaweb/pytest-docker-fixtures/pull/33 is merged, than we have a moquitto docker container for tests. This would also fix #224

Are you fine to merge it without tests? So I can upgrade my lib to the new version and get rid of the log message about the rename

codecov[bot] commented 11 months ago

Codecov Report

Merging #232 (3ef2c23) into main (9366b32) will decrease coverage by 7.1%. The diff coverage is 81.8%.

@@           Coverage Diff           @@
##            main    #232     +/-   ##
=======================================
- Coverage   91.5%   84.5%   -7.1%     
=======================================
  Files          6       4      -2     
  Lines        843     466    -377     
  Branches     179      86     -93     
=======================================
- Hits         772     394    -378     
  Misses        46      46             
- Partials      25      26      +1     
Impacted Files Coverage Δ
aiomqtt/__init__.py 100.0% <ø> (ø)
aiomqtt/error.py 100.0% <ø> (ø)
aiomqtt/types.py 100.0% <ø> (ø)
aiomqtt/client.py 83.1% <81.8%> (ø)

... and 3 files with indirect coverage changes

empicano commented 11 months ago

Hi Robert, I'll ping @frederikaalund here; Maybe this got lost 😉

As a quick fix to getting rid of the log message about the rename you could restrict the version of asyncio-mqtt to <= 0.16.1. Version 0.16.1 is exactly the same as 0.16.2 apart from the log message.

frederikaalund commented 11 months ago

Thanks for the ping, @empicano. Been a busy week at work due to technical issues. :)

In any case, this PR now looks good to me. :+1: Sorry about the delay in my response.

For tests I would wait until https://github.com/guillotinaweb/pytest-docker-fixtures/pull/33 is merged, than we have a moquitto docker container for tests. This would also fix https://github.com/sbtinstruments/aiomqtt/issues/224

Are you fine to merge it without tests? So I can upgrade my lib to the new version and get rid of the log message about the rename

Yes, that's completely fine with me.

Thank you for your contribution to aiomqtt.