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

release lock only if acquired #249

Merged
merged 3 commits into from
Sep 19, 2023
Merged

Conversation

spacemanspiff2007
Copy link
Contributor

@spacemanspiff2007 spacemanspiff2007 commented Sep 18, 2023

If connection fails calling __aexit__ will result in an Lock-exception.
This small check prevents that

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Patch coverage: 100.0% and no project coverage change.

Comparison is base (5f0c94c) 84.7% compared to head (971afe2) 84.7%.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #249   +/-   ##
=====================================
  Coverage   84.7%   84.7%           
=====================================
  Files          4       4           
  Lines        472     473    +1     
  Branches      87      88    +1     
=====================================
+ Hits         400     401    +1     
  Misses        45      45           
  Partials      27      27           
Files Changed Coverage Δ
aiomqtt/client.py 83.4% <100.0%> (+<0.1%) ⬆️

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

@empicano
Copy link
Owner

empicano commented Sep 18, 2023

Good catch, and thank you for the PR with a fix! 😊

I wasn't sure if we should allow calling __aexit__ without a prior __aenter__ (or an unsuccessful __aenter__ in this case).

I tested open() and contextlib.ExitStack(), which don't throw exceptions when calling __exit__ without __enter__. PEP343 also states that "[...] __exit__() methods should avoid raising errors unless they have actually failed." 👍

Could you add a test case for this? (Probably sufficient to test if we can call __aexit__ without __aenter__.) Then we'll merge it straight away 🙂

@spacemanspiff2007
Copy link
Contributor Author

Like this?

I wasn't sure if we should allow calling __aexit__ without a prior __aenter__ (or an unsuccessful __aenter__ in this case).

Imho it only makes sense, there is nothing to gain if it raises an error.
Also manually calling both methods is the only way to use this library if using a context manager is not an option.

@vvanglro
Copy link
Contributor

vvanglro commented Sep 19, 2023

Using a context manager is the only recommended method and the main direction of subsequent maintenance, because the removal of the connect and disconnect methods is already planned #247.

If connection fails calling __aexit__ will result in an Lock-exception. This small check prevents that

This does not happen if you use a context manager.

Based on PEP343, this PR is great.

@spacemanspiff2007
Copy link
Contributor Author

Using a context manager is the only recommended method and the main direction of subsequent maintenance, because the removal of the connect and disconnect methods is already planned #247.

I'm aware of that and the docs make this clear.
However I have a central connection manager in my application that manages multiple connection lifecycles.
Using the context manager would mean a complete refactor of the way the application handles connections and loosing features that would have to be implemented in another way..
Instead I just call the context manager methods , e.g. aenter or already with a local fix aexit and everything works as expected 🤷
If you have a good idea how I can have feature parity with a context manager I'm happy to hear it. 👍

@empicano
Copy link
Owner

For your use case, using __aenter__ and __aexit__ directly sounds like a great escape hatch 👍

The test looks good. Thank you for your contribution to aiomqtt! 😎

@empicano empicano merged commit aedaf14 into empicano:main Sep 19, 2023
11 checks passed
spacemanspiff2007 added a commit to spacemanspiff2007/aiomqtt that referenced this pull request Jan 8, 2024
empicano pushed a commit that referenced this pull request Jan 10, 2024
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.

3 participants