hardbyte / hardbyte/python-can

Design issue: Using "notifier" inside "with can.Bus(…) as bus" statement has no sufficient clean-up for reader threads

オープン
#1,526 コメント 1 件 リアクション 0 件 担当者 0 名 GitHub で見る
api question
主要言語
Python
スター
1.6k
フォーク
697
PR マージ指標
30日以内にマージされた PR はありません

説明

## Description up to exception list:
There are some community posts, as well as some Python examples, suggest to create a bus object using the "with" statement. See also section "Example usage" in the python-can repository‘s "README.rst" file. The typical form:
```
# create a bus instance using 'with' statement,
# this will cause bus.shutdown() to be called on the block exit;
# many other interfaces are supported as well (see documentation)
with can.Bus(interface='socketcan',
channel='vcan0',
receive_own_messages=True) as bus:
```
The commend describes the reason, namely calling `bus.shutdown()` at block exit.

But now, using an asynchronous `Notifier` inside this `with`-block in conjunction with a serial port!
For example, having `slcan` as interface and the common keyboard termination construct `try: … except KeyboardInterrupt:` shown below:
```
import can
import time

try:
# create a bus instance using 'with' statement,
# this will cause bus.shutdown() to be called on the block exit;
# many other interfaces are supported as well (see documentation)
with can.Bus(interface="slcan", channel="/dev/ttyCMa01", ttyBaudrate=4000000, \
bitrate=500000, sleep_after_open=0.1, rtscts=False) as bus:
#
# Start Asynchronous Notifier with console output
can.Notifier(bus, [can.Printer()]) # Attention: Second parameter must be a list

# Perform a periodic message
message_test = can.Message(arbitration_id=0x0001, is_extended_id=False, data=[])
bus.send_periodic(message_test, period=1.0)

# Sleep some time, while the periodics will be send
time.sleep(10.0)

except KeyboardInterrupt:
print ("KeyboardInterrupt -> shutdown() ...")
```
Running this as a Python script in a command line, nothing get obvious. But running the script in an IDE – **Spyder** in this case – an exception cascade raise up, regardless after regular termination (10 seconds example) or after a keyboard interrupt:

```
runfile('/home/werner/Python/pycan_test_notifier-and-send-periodic.py', wdir='/home/werner/Python')
KeyboardInterrupt -> shutdown() ...
Exception in thread can.notifier for bus "unknown":
Traceback (most recent call last):
File "/home/werner/.local/lib/python3.8/site-packages/can/exceptions.py", line 123, in error_check
yield
File "/home/werner/.local/lib/python3.8/site-packages/can/interfaces/slcan.py", line 164, in _read
in_waiting = self.serialPortOrig.in_waiting
File "/home/werner/.local/lib/python3.8/site-packages/serial/serialposix.py", line 549, in in_waiting
s = fcntl.ioctl(self.fd, TIOCINQ, TIOCM_zero_str)
OSError: [Errno 9] Ungültiger Dateideskriptor

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
File "/usr/lib/python3.8/threading.py", line 932, in _bootstrap_inner
self.run()
File "/usr/lib/python3.8/threading.py", line 870, in run
self._target(*self._args, **self._kwargs)
File "/home/werner/.local/lib/python3.8/site-packages/can/notifier.py", line 122, in _rx_thread
msg = bus.recv(self.timeout)
File "/home/werner/.local/lib/python3.8/site-packages/can/bus.py", line 98, in recv
msg, already_filtered = self._recv_internal(timeout=time_left)
File "/home/werner/.local/lib/python3.8/site-packages/can/interfaces/slcan.py", line 202, in _recv_internal
string = self._read(timeout)
File "/home/werner/.local/lib/python3.8/site-packages/can/interfaces/slcan.py", line 180, in _read
return None
File "/usr/lib/python3.8/contextlib.py", line 131, in __exit__
self.gen.throw(type, value, traceback)
File "/home/werner/.local/lib/python3.8/site-packages/can/exceptions.py", line 128, in error_check
raise exception_type(error_message) from error
can.exceptions.CanOperationError: Could not read from serial device
```
## The issue:
`can.Notifier` get the bus object as first initialization parameter and instantiate an asynchronous reader thread (`_rx_thread`), listening on the serial port, which is given (and owned) by the bus object.

If now the `with`-block ends, the bus object will be destroyed by the clean-up process (including the `bus.shutdown()` call) which also destroys the contained and used serial port (from serialposix.py).
The notifier still exists at this moment and also it‘s asynchronous receiver thread – but the serial port is thrown away underneath, causing the `Exception in thread can.notifier for bus "unknown"`.

An exception should be used as an exception – but not as part of the regular program flow. Therefore a reverse interaction between the bus and notifier objects became necessary, to realize a well ordered clean-up.

## Possible solution:
To open a discussion, here a suggestion how this could be realized:

### In Class `can.BusABC`:
- Adding a member for the reference to a notifier:
```
# Keep track about occupation by a notifier
_notifier: Any = None
```

- Adding some methods to implement administration of notifier relationship:
```
@property
def notifier(self):
"""
Return the current notifier
"""
return self._notifier

def set_notifier(self, new_notifier, replace:bool=False) -> None:
"""
Set the new notifier

:raises ~can.exceptions.CanOperationError:
If the bus still occupied by an other notifier and replace flag is False.
"""
if self._notifier == None:
self._notifier = new_notifier
else:
if replace == False:
raise CanOperationError("Bus still occupied by a notifier.")
else:
self.release_from_notifier()
self._notifier = new_notifier

def release_from_notifier(self):
"""
Remove this bus from current notifier
"""
if self._notifier != None:
# Mypy prefers this over a hasattr(...) check
getattr(self._notifier, "remove_bus", lambda: None)(self)
```

- And finally extent the shutdown method to force release from a notifier:
```
def shutdown(self) -> None:
"""
Called to carry out any interface specific cleanup required
in shutting down a bus.
"""
self.stop_all_periodic_tasks()
self.release_from_notifier()
```
### In Class `can.Notifier`:
- The method add_bus is extended to inform the bus object about the notifier binding:
```
def add_bus(self, bus: BusABC) -> None:
...
# Inform bus about occupation by a notifier
bus.set_notifier(self)
```
- And another method becomes necessary: One to terminate a reader thread and release a bus:
```
def remove_bus(self, bus: BusABC, timeout: float = 5) -> None:
"""Remove a bus from notification.

:param bus:
CAN bus instance.
"""
if len(self._readers) == 1:
self.stop() # single bus, stop listeners too
return

end_time = time.time() + timeout
for reader in self._readers:
if reader == bus:
if isinstance(reader, threading.Thread):
now = time.time()
if now < end_time:
reader.join(end_time - now)
elif self._loop:
# reader is a file descriptor
self._loop.remove_reader(reader)
```
## How it works:
### Instantiate:
Starting to instantiate an asynchronous Notifier with:
```
can.Notifier(bus, [can.Printer()])
--> __init__(self, bus: ...)
--> self.add_bus(each_bus)
--> bus.set_notifier(self)
```

The bus variable is initialized by the with-statement and given as first parameter to the Notifiers class initialization method `__init__`. Inside the method `add_bus` is called, which create a reader (thread) for the bus (or each bus in a list) and append it to the `_readers` list. Now (new) the bus instance’s `set_notifier` method is called, with the notifiers self-reference as parameter. This way the bus get knowledge about the notifier occupation, which is internally tracked by the `_notifier` member variable.

### Clean-up:
```
__exit__(self, ...):
--> self.shutdown()
--> self.stop_all_periodic_tasks()
--> self.release_from_notifier()
--> getattr(self._notifier, "remove_bus", lambda: None)(self)
```

The clean-up process – e.g. on end of the with-block or bail out by an exception – is entered by the bus objects `__exit__` method, which calls the `shutdown` method. The `shutdown` first clean-up all periodic tasks and (new) now also call `release_from_notifier`, which in turn call the related _notifier ‘s `remove_bus` method, which in turn terminate the running reader thread and remove it from the `_readers` list.

If now the bus object is destroyed, no more running tasks exists, which could throw exceptions. The notifier object of course will be destroyed just after the bus, because of it’s creation inside the with-block. But that is always clean, because running reader threads still terminated and removed before.

コントリビューションガイド

コントリビューションガイドを開く

評価

この issue はまだ評価されていません。

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。