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

Keep Track of Active Notifiers. Make Notifier usable as ContextManager. #1890

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

zariiii9003
Copy link
Collaborator

  • Make Notifier usable as a context manager to automatically call Notifier.stop(). Adapt examples to use with can.Notifier(...)
  • Add a NotifierRegistry class to ensure that a bus is not added to multiple active Notifiers
  • Fixes Unexpected behavior with multiple notifiers #1888

@pierreluctg
Copy link
Collaborator

pierreluctg commented Nov 6, 2024

@zariiii9003 can we make this NotifierRegistry a little more "public"?

For example we could have a Notifier factory (for a given Bus instance give me the existing Notifier or create a new one), etc.

@zariiii9003
Copy link
Collaborator Author

@zariiii9003 can we make this NotifierRegistry a little more "public"?

For example we could have a Notifier factory (for a given Bus instance give me the existing Notifier or create a new one), etc.

This would make things a bit more complicated.

  • We could add a __new__ method to the Notifier where we check if a Notifier already exists and return that. But the Notifier can be instantiated with a list of BusABC objects, these might be already assigned to multiple other notifiers. And the listeners could be different in the existing instance, too. This might get difficult to handle, it's easier to just always raise an error in that case.
  • The user should not directly call the register and unregister methods, so i'd rather keep the registry itself private.

However, a Notifier.find(bus) -> Notifier classmethod or something like that would be easy to add.

What kind of solution did you have in mind?

@bessman
Copy link
Contributor

bessman commented Nov 7, 2024

a Notifier.find(bus) -> Notifier classmethod

This would be great! If I understand correctly, without such a classmethod this PR in its current state does not solve the example I put forth in #1888 (comment). A Notifier with no remaining references won't be unregisterable unless there is some public way to access or search the registry.

can/notifier.py Outdated Show resolved Hide resolved
can/notifier.py Outdated
many listeners carry out flush operations to persist data.


:param bus: A :ref:`bus` or a list of buses to listen to.
:param bus:
A :ref:`bus` or a list of buses to listen to.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
A :ref:`bus` or a list of buses to listen to.
A :ref:`bus` or a list of buses to consume messages from.

can/notifier.py Outdated Show resolved Hide resolved
can/notifier.py Show resolved Hide resolved
can/notifier.py Outdated Show resolved Hide resolved
can/notifier.py Outdated
self._lock = threading.Lock()

self._readers: List[Union[int, threading.Thread]] = []
buses = self.bus if isinstance(self.bus, list) else [self.bus]
for each_bus in buses:
self._bus_list = self.bus if isinstance(self.bus, list) else [self.bus]
Copy link
Owner

Choose a reason for hiding this comment

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

self._bus_list won't be correct after a user adds an additional Bus via add_bus, instead prefer to create an empty bus list here in the initializer, but add them inside the add_bus method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. The same was true for self.bus, i changed that to a property.

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

Successfully merging this pull request may close these issues.

Unexpected behavior with multiple notifiers
4 participants