don't use __del__, call it manually

[RichardvdK]

Using __del__ to stop the executor and wait for the thread is seen as bad practice and (The only required property of Python garbage collection is that it happens after all references have been deleted, so this might not necessarily happen right after and might not happen at all.) does_ not work most of the time. I encountered that this node's spin() thread was never cleanly terminated. When calling the renamed shutdown() now (without an exception), the spin() thread can finish cleanly.

This DOES require manually calling the shutdown() method wherever you want to shutdown this node. So after approval of this method, I should rewrite the tests that use this Class.

Another way is to pass a Node to the IntrospectionServer() and IntrospectionClient() classes so they don't make new SingleThreadedExecutors. In this way, even exceptions can be handled better I think.

Another way is maybe to make the spin() thread a Daemon thread, but this resulted in the following error codes/ warnings while running the executive:

terminate called without an active exception
[ros2run]: Aborted

[Timple]

You're not changing where the function is called?

[RichardvdK]

@Timple as stated in the PR description above, I think it is better to first choose 1 of the possible fixes, and when we have chosen 1, I will fix the tests :). (Btw I don't think __del__ is called in the tests as the idea is that this is done "automatically" after the test , but I will check tomorrow )

[alireza-moayyedi]

I completely agree on the fact that relying on del should be avoided as things become real tricky when dealing with threading. And ofcourse what you have suggested is a valid alternative solution. But the main question is who will be using this code? I don't have context over this package, but some things to take into account:

• Will people always know that they need to call this function explicitly?
• What if someone's code would just work fine with the default del behavior?
• Maybe it would be better to keep the del but also have the shutdown function separately. Then you can also include it in del for those who want to rely on Python's default behavior as well as be able to call the shutdown function explicitly for those who want to be explicit about it?

[RichardvdK]

@alireza-moayyedi don't you have the problem that the shutdown can happen twice?

[Timple]

From the issues found, I conclude that this part of the code is not used by much people.
I'd go for a final fix without backwards compatibility unless asked to do so.

[alireza-moayyedi]

@RichardvdK Afaik, in ROS2 it should be fine. I just did the following test and had no problem:

    my_node.destroy_node()
    my_node.destroy_node()
    my_node.destroy_node()
    executor.shutdown()
    executor.shutdown()
    executor.shutdown()
    executor.shutdown()

I would just do what @Timple suggested since I personally do not have any context on this package and he knows who, how, uses it.