Add __del__ method to BusABC (hardbyte#1489)

* Add del method

* Add unittest

* Satisfy black formatter

* Satisfy pylint linter

* PR feedback

Co-authored-by: Felix Divo <[email protected]>

* PR feedback

Co-authored-by: Felix Divo <[email protected]>

* Move to cls attribute

* Add unittest

* Call parent shutdown from socketcand

* Wrap del in try except

* Call parent shutdown from ixxat

* Black & pylint

* PR feedback

Co-authored-by: Felix Divo <[email protected]>

* Remove try/except & fix ordering

* Fix unittest

* Call parent shutdown from etas

* Add warning filter

* Make multicast_udp back2back test more specific

* clean up test_interface_canalystii.py

* carry over from hardbyte#1519

* fix AttributeError

---------

Co-authored-by: TJ Bruno <[email protected]>
Co-authored-by: Felix Divo <[email protected]>
Co-authored-by: zariiii9003 <[email protected]>

Author: 4 people

can/bus.py

@@ -1,7 +1,7 @@
 """
 Contains the ABC bus implementation and its documentation.
 """
-
+import contextlib
 from typing import cast, Any, Iterator, List, Optional, Sequence, Tuple, Union
 
 import can.typechecking
@@ -44,12 +44,14 @@ class BusABC(metaclass=ABCMeta):
     #: Log level for received messages
     RECV_LOGGING_LEVEL = 9
 
+    _is_shutdown: bool = False
+
     @abstractmethod
     def __init__(
         self,
         channel: Any,
         can_filters: Optional[can.typechecking.CanFilters] = None,
-        **kwargs: object
+        **kwargs: object,
     ):
         """Construct and open a CAN bus instance of the specified type.
 
@@ -301,6 +303,10 @@ def stop_all_periodic_tasks(self, remove_tasks: bool = True) -> None:
         :param remove_tasks:
             Stop tracking the stopped tasks.
         """
+        if not hasattr(self, "_periodic_tasks"):
+            # avoid AttributeError for partially initialized BusABC instance
+            return
+
         for task in self._periodic_tasks:
             # we cannot let `task.stop()` modify `self._periodic_tasks` while we are
             # iterating over it (#634)
@@ -415,9 +421,15 @@ def flush_tx_buffer(self) -> None:
 
     def shutdown(self) -> None:
         """
-        Called to carry out any interface specific cleanup required
-        in shutting down a bus.
+        Called to carry out any interface specific cleanup required in shutting down a bus.
+
+        This method can be safely called multiple times.
         """
+        if self._is_shutdown:
+            LOG.debug("%s is already shut down", self.__class__)
+            return
+
+        self._is_shutdown = True
         self.stop_all_periodic_tasks()
 
     def __enter__(self):
@@ -426,6 +438,14 @@ def __enter__(self):
     def __exit__(self, exc_type, exc_val, exc_tb):
         self.shutdown()
 
+    def __del__(self) -> None:
+        if not self._is_shutdown:
+            LOG.warning("%s was not properly shut down", self.__class__)
+            # We do some best-effort cleanup if the user
+            # forgot to properly close the bus instance
+            with contextlib.suppress(AttributeError):
+                self.shutdown()
+
     @property
     def state(self) -> BusState:
         """

can/interfaces/canalystii.py

@@ -1,11 +1,11 @@
-import collections
+from collections import deque
 from ctypes import c_ubyte
 import logging
 import time
 from typing import Any, Dict, Optional, Deque, Sequence, Tuple, Union
 
 from can import BitTiming, BusABC, Message, BitTimingFd
-from can.exceptions import CanTimeoutError, CanInitializationError
+from can.exceptions import CanTimeoutError
 from can.typechecking import CanFilters
 from can.util import deprecated_args_alias, check_or_adjust_timing_clock
 
@@ -50,11 +50,12 @@ def __init__(
             If set, software received message queue can only grow to this many
             messages (for all channels) before older messages are dropped
         """
-        super().__init__(channel=channel, can_filters=can_filters, **kwargs)
-
         if not (bitrate or timing):
             raise ValueError("Either bitrate or timing argument is required")
 
+        # Do this after the error handling
+        super().__init__(channel=channel, can_filters=can_filters, **kwargs)
+
         if isinstance(channel, str):
             # Assume comma separated string of channels
             self.channels = [int(ch.strip()) for ch in channel.split(",")]
@@ -63,23 +64,23 @@ def __init__(
         else:  # Sequence[int]
             self.channels = list(channel)
 
-        self.rx_queue = collections.deque(
-            maxlen=rx_queue_size
-        )  # type: Deque[Tuple[int, driver.Message]]
+        self.rx_queue: Deque[Tuple[int, driver.Message]] = deque(maxlen=rx_queue_size)
 
         self.channel_info = f"CANalyst-II: device {device}, channels {self.channels}"
 
         self.device = driver.CanalystDevice(device_index=device)
-        for channel in self.channels:
+        for single_channel in self.channels:
             if isinstance(timing, BitTiming):
                 timing = check_or_adjust_timing_clock(timing, valid_clocks=[8_000_000])
-                self.device.init(channel, timing0=timing.btr0, timing1=timing.btr1)
+                self.device.init(
+                    single_channel, timing0=timing.btr0, timing1=timing.btr1
+                )
             elif isinstance(timing, BitTimingFd):
                 raise NotImplementedError(
                     f"CAN FD is not supported by {self.__class__.__name__}."
                 )
             else:
-                self.device.init(channel, bitrate=bitrate)
+                self.device.init(single_channel, bitrate=bitrate)
 
     # Delay to use between each poll for new messages
     #

can/interfaces/etas/__init__.py

@@ -248,6 +248,7 @@ def flush_tx_buffer(self) -> None:
         OCI_ResetQueue(self.txQueue)
 
     def shutdown(self) -> None:
+        super().shutdown()
         # Cleanup TX
         if self.txQueue:
             OCI_DestroyCANTxQueue(self.txQueue)

can/interfaces/ixxat/canlib.py

@@ -147,6 +147,7 @@ def _send_periodic_internal(self, msgs, period, duration=None):
         return self.bus._send_periodic_internal(msgs, period, duration)
 
     def shutdown(self) -> None:
+        super().shutdown()
         self.bus.shutdown()
 
     @property

can/interfaces/socketcand/socketcand.py

@@ -183,5 +183,5 @@ def send(self, msg, timeout=None):
         self._tcp_send(ascii_msg)
 
     def shutdown(self):
-        self.stop_all_periodic_tasks()
+        super().shutdown()
         self.__socket.close()

test/back2back_test.py

@@ -12,6 +12,7 @@
 import pytest
 
 import can
+from can import CanInterfaceNotImplementedError
 from can.interfaces.udp_multicast import UdpMulticastBus
 
 from .config import (
@@ -294,7 +295,7 @@ class BasicTestUdpMulticastBusIPv4(Back2BackTestCase):
     CHANNEL_2 = UdpMulticastBus.DEFAULT_GROUP_IPv4
 
     def test_unique_message_instances(self):
-        with self.assertRaises(NotImplementedError):
+        with self.assertRaises(CanInterfaceNotImplementedError):
             super().test_unique_message_instances()
 
 
@@ -313,15 +314,15 @@ class BasicTestUdpMulticastBusIPv6(Back2BackTestCase):
     CHANNEL_2 = HOST_LOCAL_MCAST_GROUP_IPv6
 
     def test_unique_message_instances(self):
-        with self.assertRaises(NotImplementedError):
+        with self.assertRaises(CanInterfaceNotImplementedError):
             super().test_unique_message_instances()
 
 
 TEST_INTERFACE_ETAS = False
 try:
     bus_class = can.interface._get_class_for_interface("etas")
     TEST_INTERFACE_ETAS = True
-except can.exceptions.CanInterfaceNotImplementedError:
+except CanInterfaceNotImplementedError:
     pass
 
 

test/test_bus.py

@@ -1,3 +1,4 @@
+import gc
 from unittest.mock import patch
 
 import can
@@ -12,3 +13,11 @@ def test_bus_ignore_config():
 
         _ = can.Bus(interface="virtual")
         assert can.util.load_config.called
+
+
+@patch.object(can.bus.BusABC, "shutdown")
+def test_bus_attempts_self_cleanup(mock_shutdown):
+    bus = can.Bus(interface="virtual")
+    del bus
+    gc.collect()
+    mock_shutdown.assert_called()

test/test_interface.py

@@ -0,0 +1,42 @@
+import importlib
+from unittest.mock import patch
+
+import pytest
+
+import can
+from can.interfaces import BACKENDS
+
+
+@pytest.fixture(params=(BACKENDS.keys()))
+def constructor(request):
+    mod, cls = BACKENDS[request.param]
+
+    try:
+        module = importlib.import_module(mod)
+        constructor = getattr(module, cls)
+    except:
+        pytest.skip("Unable to load interface")
+
+    return constructor
+
+
+@pytest.fixture
+def interface(constructor):
+    class MockInterface(constructor):
+        def __init__(self):
+            pass
+
+        def __del__(self):
+            pass
+
+    return MockInterface()
+
+
+@patch.object(can.bus.BusABC, "shutdown")
+def test_interface_calls_parent_shutdown(mock_shutdown, interface):
+    try:
+        interface.shutdown()
+    except:
+        pass
+    finally:
+        mock_shutdown.assert_called()

test/test_interface_canalystii.py

@@ -1,11 +1,7 @@
 #!/usr/bin/env python
 
-"""
-"""
-
-import time
 import unittest
-from unittest.mock import Mock, patch, call
+from unittest.mock import patch, call
 from ctypes import c_ubyte
 
 import canalystii as driver  # low-level driver module, mock out this layer