Skip to content

network: Avoid notifier re-creating in connect function#621

Open
xndcn wants to merge 1 commit intocanopen-python:masterfrom
xndcn:patch-1
Open

network: Avoid notifier re-creating in connect function#621
xndcn wants to merge 1 commit intocanopen-python:masterfrom
xndcn:patch-1

Conversation

@xndcn
Copy link
Copy Markdown

@xndcn xndcn commented Dec 9, 2025

No description provided.

@xndcn
Copy link
Copy Markdown
Author

xndcn commented Mar 26, 2026

ping, thanks

@xndcn
Copy link
Copy Markdown
Author

xndcn commented Mar 26, 2026

If network connect again without disconnect, current implementation will not re-create bus, but notifier will always be re-created. I think it is better to check the notifier in connect() and release it in disconnect, as well as the bus object.

@bizfsc
Copy link
Copy Markdown

bizfsc commented Apr 28, 2026

The fix is correct – without this patch, calling connect() again without a prior disconnect() always re-creates the notifier, leaking the old one. The guard on self.bus already existed, so this makes the notifier handling symmetrical. Nice catch!

Two suggestions:

1. Simplify try/except/finally in disconnect()

The except Exception as e: raise e is a no-op and can be removed. This simplifies to:

try:
    self.check()
finally:
    self.notifier = None

2. Add regression tests

Consider adding tests to test/test_network.py to verify the new behavior:

def test_network_connect_does_not_recreate_notifier(self):
    self.network.connect(interface="virtual")
    self.addCleanup(self.network.disconnect)
    notifier1 = self.network.notifier
    self.assertIsNotNone(notifier1)
    # Calling connect() again should reuse the existing notifier
    self.network.connect(interface="virtual")
    self.assertIs(self.network.notifier, notifier1)

def test_network_disconnect_releases_notifier(self):
    self.network.connect(interface="virtual")
    self.assertIsNotNone(self.network.notifier)
    self.network.disconnect()
    self.assertIsNone(self.network.notifier)

def test_network_disconnect_releases_notifier_on_exception(self):
    self.network.connect(interface="virtual")

    class Custom(Exception):
        pass

    self.network.notifier.exception = Custom("fake")
    with self.assertRaises(Custom):
        with self.assertLogs(level=logging.ERROR):
            self.network.disconnect()
    # Notifier must be released even when check() raises
    self.assertIsNone(self.network.notifier)

I verified locally that all three tests pass with the patched code.

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.

2 participants