Reduce network shutdown latency

[erlend-aasland]

Currently, when we shut down a network, we (among other things) call notifier.stop(), if there's a notifier. The can.Notifier has a pretty long timeout by default; is this needed when we call network.shutdown()?

[erlend-aasland]

Proposal: explicitly stop the notifier using a timeout of zero.

diff --git a/canopen/network.py b/canopen/network.py
index e886b65..99009a7 100644
--- a/canopen/network.py
+++ b/canopen/network.py
@@ -117,7 +117,7 @@ class Network(MutableMapping):
             if hasattr(node, "pdo"):
                 node.pdo.stop()
         if self.notifier is not None:
-            self.notifier.stop()
+            self.notifier.stop(0)
         if self.bus is not None:
             self.bus.shutdown()
         self.bus = None

[erlend-aasland]

Quoting from the can.Notifier.stop docstring:

Max time in seconds to wait for receive threads to finish.
Should be longer than timeout given at instantiation.

As of HEAD at 30601c70808f3feed533caf76da86b515a9a7d48 (over at python-can), the defaults are one second in __init__, and 5 seconds in stop.

• https://github.com/hardbyte/python-can/blob/30601c70808f3feed533caf76da86b515a9a7d48/can/notifier.py#L21-L45
• https://github.com/hardbyte/python-can/blob/30601c70808f3feed533caf76da86b515a9a7d48/can/notifier.py#L89-L96

[erlend-aasland]

When we add a notifier (in canopen.Network.connect()), we explicitly set the timeout to 1 second.

https://github.com/christiansandberg/canopen/blob/78f6dded0b714e542dd0c5bac7af73b8012b499c/canopen/network.py#L108

[acolomb]

What does the "timeout" even do? Why would we need to wait in order to stop anything?

[erlend-aasland]

What does the "timeout" even do? Why would we need to wait in order to stop anything?

It seems to be some kind of guard to ensure the notifier thread worker can complete its current loop iteration.

The can.Notifier is run as a thread (the thread worker method is called _rx_thread). It loops "forever" and tries for each iteration to recv() on the attached bus, using the provided timeout (1 second, as of now).

[erlend-aasland]

1 second is a very long timeout for such a thread; 100 ms or 50 ms would probably be a better choice.

[acolomb]

So it is less of a timeout, but rather a maximum polling interval before a fresh loop iteration is started (which also checks if it should abort)? I wouldn't lower it beyond the 1 seconds in that case, it's just useless thread spinning.

We could just use the notifier.timeout value as a basis, then it's easier to customize without breaking the assumption from the python-can docs. Like self.notifier.timeout + 0.1 for example. Or maybe better to raise this with python-can, so the default is not 500 % of the init default, but e.g. 110 % of the actual value when .stop() is called.

[erlend-aasland]

[...] (which also checks if it should abort)?

Yes.

We could just use the notifier.timeout value as a basis, then it's easier to customize without breaking the assumption from the python-can docs.

Definitely.

Or maybe better to raise this with python-can, so the default is not 500 % of the init default, but e.g. 110 % of the actual value when .stop() is called.

Yep, I'll consider it.

For now, I think what I would like to do, is something like this:

• add a timeout keyword-only parameter to canopen.Network.connect(), and use this when initialising can.Notifier (instead of the currently hardcoded 1 second)
• either add a timeout keyword-only parameter to canopen.Network.disconnect(), or reuse the timeout param from connect().

BTW, currently, we add 1 second to the test suite run-time for every canopen.Network.shutdown() we invoke, and we do a lot of those. We can reduce CI time greatly by allowing the possibility to override these timeouts.

[acolomb]

I get why this is important. However, the timeout parameter on canopen.Network.connect() seems confusing. It should at least be named to better reflect its purpose. And I think it would better be a class variable, thus overridable per instance if required by special circumstances, but otherwise not cluttering the public API.

I'm not sure we really should lower the notifier timeout significantly for the tests though. Catching real-world issues seems less likely if we change the timing behavior.

So the second change would be fine, if based on the actual self.notifier.timeout value. Which I still think makes more sense to add in the lower layer. But we can work around it for now. Question is, what if someone gives a timeout that is too short, just because the parameter is there? Is the notifier a daemon thread? Will it delay interpreter shutdown anyway, even if we don't wait for it to cleanly finish?

[erlend-aasland]

[...] the timeout parameter on canopen.Network.connect() seems confusing. It should at least be named to better reflect its purpose. And I think it would better be a class variable, thus overridable per instance if required by special circumstances, but otherwise not cluttering the public API.

SGTM.

I'm not sure we really should lower the notifier timeout significantly for the tests though. Catching real-world issues seems less likely if we change the timing behavior.

These shutdowns are all happening in the tearDown() methods of the test cases; all real testing happen in the test_ methods. Of course, the test case will fail if the tearDown() method fails.

We do not (yet) have dedicated tests for the canopen.Network.shutdown method; this should be fixed no matter if we decide to make an adjustment or not.

So the second change would be fine, if based on the actual self.notifier.timeout value. Which I still think makes more sense to add in the lower layer. But we can work around it for now.

SGTM.

Question is, what if someone gives a timeout that is too short, just because the parameter is there? Is the notifier a daemon thread? Will it delay interpreter shutdown anyway, even if we don't wait for it to cleanly finish?

Good concerns. The notifiers are not daemon threads, so they should not affect interpreter shutdown¹.

Footnotes

1. IIRC; let me double check later. ↩

[acolomb]

Also note that the 5 second timeout shouldn't affect us in any way. It is the maximum waiting time in .stop(), but the thread will unblock after a maximum of self.notifier.timeout (1 second, or shorter if we were not exactly at the interval start). Only if it is stuck in some user callback, then the delay could be longer. But that should never be the case in the tests, where we control the callback setup.

[erlend-aasland]

Sure. As I said earlier, we're adding 1 second (not 5) for every network shutdown in our tearDown() methods. If I run the test suite locally, it completes at around 12 seconds (instead of one minute) if I force shutdown() to return immediately.

[acolomb]

So the threads will eventually terminate, we just don't wait for them to finish and join? That seems like a sensible behavior during tests, if the default runtime behavior stays the same. Adding a customization is surely in order, which we will then use to speed up the tests. Got it, go ahead!

In the node.local tests, we use setUpClass() and tearDownClass() to handle the network. Would that help, or is the setUp() / tearDown() combo already called only once per TestCase?

[acolomb]

By the way, we seem to not have the same issue with PDO tasks' .stop() method. In the end, it's a sleep() in can.broadcastmanager.ThreadBasedCyclicSendTask._run() that defines the maximum delay, but I can't find a join() call. But there might be other implementations depending on the used interface, and for Windows there is already a workaround in place.

Not joining these threads (for notifier or periodic message task) has the potential of raising exceptions when they try to access the no longer valid BusABC or Network or Node objects. But that seems irrelevant during tests, where we only use virtual busses and therefore fully control what could be sent or received.