Skip to content

Feature: Context manager #283

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

Merged
merged 5 commits into from
Jun 4, 2018
Merged

Conversation

edobez
Copy link
Contributor

@edobez edobez commented Mar 20, 2018

Hello everybody,
I am a new user of python-can but I decided to contribute a little to it.

I have started to work on a new feature: the context manager implementation for the Bus class, and I would like to do the same with other classes that require accessing and releasing resources, like Notifier.
In this way it's possible to open and close a bus instance automatically, i.e.:

with can.interface.Bus(bustype='pcan') as bus:
    bus.send(msg)

bus.send(msg)  # raises an exception since bus is closed

The context manager implementation is quite straight forward. What I wonder about is how to implement the exception behaviour.
In order to get an exception when requesting an operation after the bus has been closed, the instance should know about its 'openness' (note that this applies also without the context manager).
At the moment if you try to call a function after the shutdown() call, you'll get an exception specific to one interface implementation.
Relying on this, in my opinion, it's not the best since different interfaces (thus different implementations) can behave in different ways._
For example, the virtual interface doesn't release any resource after its shutdown and continues to work even after the closing call.

My idea is to use a state variable inside the instance that indicates if the bus is open or not, and use this information before trying to do any operation on the bus.
This would require to edit all the current interface implementations by adding some helper functions, something like what I did on the virtual class and that you can see in the PR.

Let me know if this can be a good idea.

@felixdivo
Copy link
Collaborator

Hm, very promising. :) Maybe #235 can help you with the states.

Copy link
Collaborator

@christiansandberg christiansandberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't worry about interface specific behaviour, since that is to be expected on all operations.

can/bus.py Outdated

def __exit__(self, exc_type, exc_val, exc_tb):
for t in self._tx_threads:
t.stop()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. It could even be moved to the .shutdown() method in my opinion.

@@ -169,5 +169,103 @@ def test_basics(self):
notifier.stop()


class Back2BackTestCaseWithContext(unittest.TestCase):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think this is needed. The other test should suffice.

@felixdivo
Copy link
Collaborator

@edobez Any updates on this?

@felixdivo
Copy link
Collaborator

    def __exit__(self, exc_type, exc_val, exc_tb):
>       for thread in self._tx_threads:
E       AttributeError: 'VirtualBus' object has no attribute '_tx_threads'

Uhm, I guess this method should not assume that that attribute exists. It is not documented anywhere! And shouldn't it call shutdown() instead anyways?

@felixdivo
Copy link
Collaborator

I removed the _tx_threads attribute since that might be for another PR/issue.

@felixdivo
Copy link
Collaborator

I will merge it and then combine Back2BackTestCaseWithContext and Back2BackTestCase in another commit since I cannot push directly to this PR.

@felixdivo felixdivo merged commit 7b1026d into hardbyte:develop Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants