-
Notifications
You must be signed in to change notification settings - Fork 73
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
don't suppress exception on aexit #232
Conversation
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.
Hi Robert, thanks for opening this pull request. 👍 Let me have a look. :)
You're right. This seems like a regression. Indeed, the client should raise (the Exception that caused a disconnect) on exit. The current implementation erroneously suppresses (and logs) this disconnect exception. That's my bad; I missed it in the code review. Good thing that you had a test case for it. If you don't mind, could you implement a similar test case here in aiomqtt? 👍
In any case, I like this PR overall. I only have requests for minor changes. See my review for details. Let me know if you have any questions. Also, you need to fix the formatting/style issues (see the CI runs for details).
Thanks again for opening this pull request.
Best regards,
~Frederik
For tests I would wait until guillotinaweb/pytest-docker-fixtures#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 Report
@@ 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
|
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 |
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. 👍 Sorry about the delay in my response.
Yes, that's completely fine with me. Thank you for your contribution to aiomqtt. |
With #216 the "early out on disconnected" code was move from the function
__aexit__
todisconnect
. Thedisconnect
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
to1.0.0
. Afterwards the following test was failing https://github.com/DeebotUniverse/client.py/blob/3d7467e3dcf338e9d848dd5fb3ad32a5ee7fab8e/tests/test_mqtt_client.py#L76C1-L76C1The test test the described example above.