juergenH87 / python-can-j1939

The package provides SAE J1939 support for Python developers
MIT License
90 stars 51 forks source link

ElectronicControlUnit.disconnect() explodes if called twice #55

Open grant-allan-ctct opened 10 months ago

grant-allan-ctct commented 10 months ago

I am using a unittest-based enviroment for some exploratory coding for the purpose of learning how python-can-j1939 works. In the tearDown of my test class, I am making a call to ecu.disconnect() in order to perform housekeeping / clean-up tasks after each test runs. I accidentally discovered that if my testcase code itself calls ecu.disconnect() during the test, the tearDown explodes. This happens because the implementation of disconnect doesn't check for None-ness of the _bus before calling its shutdown() method. Calling the below function twice will cause an error.

    def disconnect(self):
        """Disconnect from the CAN bus.

        Must be overridden in a subclass if a custom interface is used.
        """
        self._notifier.stop()
        self._bus.shutdown()
        self._bus = None

I think that we should change this function so that it can be called safely multiple times. (This would match the philosophy used by the BusABC.shutdown() method itself, see documentation here.)

If we make a change to add safety to disconnect(), then someone can call that function in an error-handler / housekeeping situation, or in unit test tear-down, etc., and it will always work effortlessly - regardless of whether disconnect() happens to have been called successfully earlier or not.

If we do not make a change, the following workaround may help someone. It involves subclassing the ElectronicControlUnit class, something like this. Wherever you would normally use ElectronicControlUnit, use the subclass instead:

class RobustECU(j1939.ElectronicControlUnit):

  def disconnect(self):
    # workaround for parent class not being robust against multiple disconnect calls
    if self._bus is not None:
      super().disconnect()