Only hold weak internal reference to callbacks

[Flova]

Currently, subscriptions that are not manually destroyed hold a reference to the callback. This can lead to unexpected issues where a class has a callback, which was created internally (not visible to the user creating the class). If all normal references to this class are dropped, it still exists because of the reference through the ROS internals to the subscription. The object could be dropped as only the subscription is referencing it, but it isn't. It would be nice if ROS only holds a weak ref and automatically destroys the subscription if the weak red callback is destroyed. Otherwise, users might create many subscriptions on the same topic without knowing that they are still around.

One could easily add a destroy method which clears all subscriptions in a given object, but the user would need to remember calling it when an object is not used anymore. This contradicts the typical comfort in a garbage collected language and resembles you manually need to free your memory at the correct moment.

The same also might be true for services, actions and other components.

Weak refs are part of the python standard library. But adding them adds overhead and complexity in rclpy.

Bug report

Required Info:

• Operating System:
  • Ubuntu 22.04
• Installation type:
  • Binary
• Version or commit hash:
  • ROS Iron
• DDS implementation:
  • cyclone dds
• Client library (if applicable):
  • rclpy

Steps to reproduce issue

See this case:

import gc

import rclpy
from rclpy.node import Node
from std_msgs.msg import String

rclpy.init()

# Create a node
node = Node("test_node")

# Create a test class that we want to destroy later
class TestClass:
    def __init__(self, n: Node, number: int):
        self.sub = n.create_subscription(
            String,
            "test_topic",
            self.callback,
            10
        )
        self.number = number

    def callback(self, msg):
        print(msg.data)

    def __del__(self):
        print(f"TestClass {self.number} deleted")

# Create an instance of the test class
test = TestClass(node, 0)

# Create another instance of the test class, dereferencing the first one
test = TestClass(node, 1)

# Call the garbage collector to make sure it tries to delete the first instance
gc.collect()

# Spin the node
rclpy.spin(node)

If we cancel the program manually after a few seconds, it destroys both instances. But only at shutdown, not before that.

Output:

KeyboardInterrupt
TestClass 1 deleted
TestClass 0 deleted

Expected behavior

If we don't reference the class in our subscription, we see that the first class is directly destroyed.

import gc

import rclpy
from rclpy.node import Node
from std_msgs.msg import String

rclpy.init()

# Create a node
node = Node("test_node")

# Create a test class that we want to destroy later
class TestClass:
    def __init__(self, n: Node, number: int):
        self.sub = n.create_subscription(
            String,
            "test_topic",
            lambda msg: print(msg.data),
            10
        )
        self.number = number

    def callback(self, msg):
        print(msg.data)

    def __del__(self):
        print(f"TestClass {self.number} deleted")

# Create an instance of the test class
test = TestClass(node, 0)

# Create another instance of the test class, dereferencing the first one
test = TestClass(node, 1)

# Call the garbage collector to make sure it tries to delete the first instance
gc.collect()

# Spin the node
rclpy.spin(node)

Output:

TestClass 0 deleted
KeyboardInterrupt
TestClass 1 deleted

Actual behavior

It is not destroyed

Additional information

---

Feature request

Feature description

It is a bit of both bug and feature imo., so I answered both.

Implementation considerations

This is a very hacky ad hoc implementation of a weak ref subscriber that works with methods of classes as well as standalone functions. It is more a feasability test then an actual implementation suggestion. Just to see some limitations etc. of this.

import gc
import weakref

import rclpy
from rclpy.node import Node
from std_msgs.msg import String

rclpy.init()

# Create a node
node = Node("test_node")


def create_weak_subscription(n: Node, callback) -> rclpy.subscription.Subscription:
    # Create a subscription with a placehoder, because we need the reference in the callback
    sub = node.create_subscription(String, "test_topic", lambda msg: None, 1)

    # Check if callback if a bound method or a function
    if hasattr(callback, "__self__"):
        # Create a weak reference to the bound method
        weak_callback = weakref.WeakMethod(callback)
    else:
        # Create a weak reference to the function
        weak_callback = weakref.ref(callback)

    # Create a callback wrapper that checks if the bound method is still alive and deletes the subscription if not
    def callback(msg):
        # Check if the bound method is still alive
        if weak_callback() is None:
            # Delete the subscription if not
            n.destroy_subscription(sub)
            print("Subscription deleted")
        else:
            # Call the bound method
            weak_callback()(msg)

    # Set the callback if the subscription (the function or weak bound method)
    sub.callback = callback

    return sub

# Create a test class that we want to destroy later
class TestClass:
    def __init__(self, n: Node, number: int):

        self.sub = create_weak_subscription(n, self.callback)
        self.number = number

    def callback(self, msg):
        print(msg, self.number)

    def __del__(self):
        print(f"TestClass {self.number} deleted")

# Create an instance of the test class
test = TestClass(node, 0)

rclpy.spin_once(node)

# Create another instance of the test class, dereferencing the first one
test = TestClass(node, 1)

# Call the garbage collector to make sure it tries to delete the first instance
gc.collect()

# Spin the node
rclpy.spin(node)

Together with a simple publisher on the String topic, it results in this output:

std_msgs.msg.String(data='Test') 0
TestClass 0 deleted
Subscription deleted
std_msgs.msg.String(data='Test') 1
std_msgs.msg.String(data='Test') 1
std_msgs.msg.String(data='Test') 1
^C KeyboardInterrupt
TestClass 1 deleted

[sloretz]

It sounds like the behavior you're looking for is that the subscription would destroy's itself when the callback it was supposed to call is garbage collected. I think that would be a problem when using temporary functions or lambdas for callbacks, like in this example.

I agree there's something weird, but I think it's that the node keeps a reference to the subscription. It would make more sense to me that droping a reference to a subscription would cause it to be destroyed (and in turn cause the callback to be garbage collected if nothing else has a reference to it).