From 7855da1b4bb32f7bb3a1e733d59be40571fd4f8b Mon Sep 17 00:00:00 2001 From: Teejay Date: Thu, 30 Mar 2023 09:58:18 -0700 Subject: [PATCH] Add `__del__` method to `BusABC` (#1489) * Add del method * Add unittest * Satisfy black formatter * Satisfy pylint linter * PR feedback Co-authored-by: Felix Divo <4403130+felixdivo@users.noreply.github.com> * PR feedback Co-authored-by: Felix Divo <4403130+felixdivo@users.noreply.github.com> * 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 <4403130+felixdivo@users.noreply.github.com> * 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 #1519 * fix AttributeError --------- Co-authored-by: TJ Bruno Co-authored-by: Felix Divo <4403130+felixdivo@users.noreply.github.com> Co-authored-by: zariiii9003 <52598363+zariiii9003@users.noreply.github.com> --- can/bus.py | 28 ++++++++++++++--- can/interfaces/canalystii.py | 21 +++++++------ can/interfaces/etas/__init__.py | 1 + can/interfaces/ixxat/canlib.py | 1 + can/interfaces/socketcand/socketcand.py | 2 +- test/back2back_test.py | 7 +++-- test/test_bus.py | 9 ++++++ test/test_interface.py | 42 +++++++++++++++++++++++++ test/test_interface_canalystii.py | 6 +--- 9 files changed, 94 insertions(+), 23 deletions(-) create mode 100644 test/test_interface.py diff --git a/can/bus.py b/can/bus.py index 292c754fb..32a26ebcb 100644 --- a/can/bus.py +++ b/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: """ diff --git a/can/interfaces/canalystii.py b/can/interfaces/canalystii.py index 7150a60bd..1e6a7fea4 100644 --- a/can/interfaces/canalystii.py +++ b/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 # diff --git a/can/interfaces/etas/__init__.py b/can/interfaces/etas/__init__.py index 3a203a50d..03dc42bc1 100644 --- a/can/interfaces/etas/__init__.py +++ b/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) diff --git a/can/interfaces/ixxat/canlib.py b/can/interfaces/ixxat/canlib.py index a20e4f59b..01f754115 100644 --- a/can/interfaces/ixxat/canlib.py +++ b/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 diff --git a/can/interfaces/socketcand/socketcand.py b/can/interfaces/socketcand/socketcand.py index 32b9a0edf..3f4e2ac86 100644 --- a/can/interfaces/socketcand/socketcand.py +++ b/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() diff --git a/test/back2back_test.py b/test/back2back_test.py index ce09b6179..48c98bf59 100644 --- a/test/back2back_test.py +++ b/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,7 +314,7 @@ 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() @@ -321,7 +322,7 @@ def test_unique_message_instances(self): try: bus_class = can.interface._get_class_for_interface("etas") TEST_INTERFACE_ETAS = True -except can.exceptions.CanInterfaceNotImplementedError: +except CanInterfaceNotImplementedError: pass diff --git a/test/test_bus.py b/test/test_bus.py index e11d829d3..24421b2fd 100644 --- a/test/test_bus.py +++ b/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() diff --git a/test/test_interface.py b/test/test_interface.py new file mode 100644 index 000000000..271e90b1b --- /dev/null +++ b/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() diff --git a/test/test_interface_canalystii.py b/test/test_interface_canalystii.py index 4d1d3eb84..4f3033e10 100755 --- a/test/test_interface_canalystii.py +++ b/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