Collect useless graph's connections

Before this patch, the connections accumulate in the graph's array of
connections and are never destroyed until the graph is destroyed.
However, because it is possible to disconnect the ports of a connection,
it is possible that a graph contains connections that are useless. This
can cause a "leak" of connections, which would be especially experienced
with a graph that contains a source which dynamically adds and removes
many ports that are connected during its lifetime.

The conditions for a connection to be considered useless are:

1. Its reference count is 0. This means only its parent, the graph,
   controls its existence.
2. Its ports are disconnected.
3. All its created notification iterators are finalized.

When all the conditions above are satisfied, it is safe to remove the
connection from its parent graph.

Conditions 2 and 3 can be checked in:

* bt_connection_disconnect_ports(): The connection is dead, detached
  from its ports, and vice versa, thus satisfying condition 2.
* bt_connection_remove_iterator(): The number of notification iterators
  changes; could drop to 0, thus satisfying condition 3.

To accomodate condition 1, a new concept is added to the base object: it
is possible to set a "parent is owner" listener which is called when the
parent becomes the owner of the object (that is, when the object's
reference count drops to 0 and it has a parent). Connection objects use
this callback to possibly remove itself from its parent when all three
conditions above are satisfied.

Signed-off-by: Philippe Proulx <eeppeliteloop@gmail.com>
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>

diff --git a/include/babeltrace/graph/graph-internal.h b/include/babeltrace/graph/graph-internal.h
index 9efd3590120be49c7e4b767dea5a1767ce3a4042..405b8c38e2659d7b7f5d6238e0afa204a64afe2f 100644 (file)
--- a/include/babeltrace/graph/graph-internal.h
+++ b/include/babeltrace/graph/graph-internal.h
@@ -83,4 +83,8 @@ void bt_graph_notify_ports_disconnected(struct bt_graph *graph,
                struct bt_port *upstream_port,
                struct bt_port *downstream_port);
 
+BT_HIDDEN
+void bt_graph_remove_connection(struct bt_graph *graph,
+               struct bt_connection *connection);
+
 #endif /* BABELTRACE_COMPONENT_COMPONENT_GRAPH_INTERNAL_H */

diff --git a/include/babeltrace/object-internal.h b/include/babeltrace/object-internal.h
index e130a81e883d620cba525cffc28a4ffba2f90fcd..545a20aadd21a7f9f9bd3b78971f9e64b0c5182b 100644 (file)
--- a/include/babeltrace/object-internal.h
+++ b/include/babeltrace/object-internal.h
@@ -38,8 +38,10 @@
  */
 struct bt_object {
        struct bt_ref ref_count;
-       /* Class-specific release function. */
+       /* Class-specific, optional release function. */
        bt_object_release_func release;
+       /* Class-specific, optional "parent is owner" notification listener. */
+       bt_object_release_func parent_is_owner_listener;
        /* @see doc/ref-counting.md */
        struct bt_object *parent;
 };
@@ -66,8 +68,20 @@ static inline
 void generic_release(struct bt_object *obj)
 {
        if (obj->parent) {
+               void *parent = obj->parent;
+
+               if (obj->parent_is_owner_listener) {
+                       /*
+                        * Object has a chance to destroy itself here
+                        * under certain conditions and notify its
+                        * parent. At this point the parent is
+                        * guaranteed to exist because it's not put yet.
+                        */
+                       obj->parent_is_owner_listener(obj);
+               }
+
                /* The release function will be invoked by the parent. */
-               bt_put(obj->parent);
+               bt_put(parent);
        } else {
                bt_object_release(obj);
        }
@@ -116,4 +130,12 @@ void bt_object_init(void *ptr, bt_object_release_func release)
        bt_ref_init(&obj->ref_count, generic_release);
 }
 
+static inline
+void bt_object_set_parent_is_owner_listener(void *obj,
+               bt_object_release_func cb)
+{
+       assert(obj);
+       ((struct bt_object *) obj)->parent_is_owner_listener = cb;
+}
+
 #endif /* BABELTRACE_OBJECT_INTERNAL_H */

diff --git a/lib/graph/connection.c b/lib/graph/connection.c
index feed59556247c562c40d2149cf38cae7e20263f7..7bdbee6227ff3656eedc3e9959f2d720fea01644 100644 (file)
--- a/lib/graph/connection.c
+++ b/lib/graph/connection.c
@@ -82,6 +82,45 @@ void bt_connection_destroy(struct bt_object *obj)
        g_free(connection);
 }
 
+static
+void bt_connection_try_remove_from_graph(struct bt_connection *connection)
+{
+       void *graph = bt_object_borrow_parent(&connection->base);
+
+       if (connection->base.ref_count.count > 0 ||
+                       connection->downstream_port ||
+                       connection->upstream_port ||
+                       connection->iterators->len > 0) {
+               return;
+       }
+
+       /*
+        * At this point we know that:
+        *
+        * 1. The connection is dead (ports were disconnected).
+        * 2. All the notification iterators that this connection
+        *    created, if any, are finalized.
+        * 3. The connection's reference count is 0, so only the
+        *    parent (graph) owns this connection after this call.
+        *
+        * In other words, no other object than the graph knows this
+        * connection.
+        *
+        * It is safe to remove the connection from the graph, therefore
+        * destroying it.
+        */
+       bt_graph_remove_connection(graph, connection);
+}
+
+static
+void bt_connection_parent_is_owner(struct bt_object *obj)
+{
+       struct bt_connection *connection = container_of(obj,
+                       struct bt_connection, base);
+
+       bt_connection_try_remove_from_graph(connection);
+}
+
 struct bt_connection *bt_connection_from_private_connection(
                struct bt_private_connection *private_connection)
 {
@@ -109,6 +148,8 @@ struct bt_connection *bt_connection_create(
        }
 
        bt_object_init(connection, bt_connection_destroy);
+       bt_object_set_parent_is_owner_listener(connection,
+               bt_connection_parent_is_owner);
        connection->iterators = g_ptr_array_new();
        if (!connection->iterators) {
                BT_PUT(connection);
@@ -132,7 +173,8 @@ void bt_connection_disconnect_ports(struct bt_connection *conn)
        struct bt_component *upstream_comp = NULL;
        struct bt_port *downstream_port = conn->downstream_port;
        struct bt_port *upstream_port = conn->upstream_port;
-       struct bt_graph *graph = (void *) bt_object_get_parent(conn);
+       struct bt_graph *graph = (void *) bt_object_borrow_parent(conn);
+       size_t i;
 
        if (downstream_port) {
                downstream_comp = bt_port_get_component(downstream_port);
@@ -160,7 +202,28 @@ void bt_connection_disconnect_ports(struct bt_connection *conn)
                downstream_comp, upstream_port, downstream_port);
        bt_put(downstream_comp);
        bt_put(upstream_comp);
-       bt_put(graph);
+
+       /*
+        * Because this connection is dead, finalize (cancel) each
+        * notification iterator created from it.
+        */
+       for (i = 0; i < conn->iterators->len; i++) {
+               struct bt_notification_iterator *iterator =
+                       g_ptr_array_index(conn->iterators, i);
+
+               bt_notification_iterator_finalize(iterator);
+
+               /*
+                * Make sure this iterator does not try to remove itself
+                * from this connection's iterators on destruction
+                * because this connection won't exist anymore.
+                */
+               bt_notification_iterator_set_connection(iterator,
+                       NULL);
+       }
+
+       g_ptr_array_set_size(conn->iterators, 0);
+       bt_connection_try_remove_from_graph(conn);
 }
 
 struct bt_port *bt_connection_get_upstream_port(
@@ -282,4 +345,5 @@ void bt_connection_remove_iterator(struct bt_connection *conn,
                struct bt_notification_iterator *iterator)
 {
        g_ptr_array_remove(conn->iterators, iterator);
+       bt_connection_try_remove_from_graph(conn);
 }

diff --git a/lib/graph/graph.c b/lib/graph/graph.c
index ab6b14cfb047c296d0db4183932220525ee4c0ec..34e832ad4c3a456c41e15f4404ee95ff8687d4e4 100644 (file)
--- a/lib/graph/graph.c
+++ b/lib/graph/graph.c
@@ -782,3 +782,12 @@ extern bt_bool bt_graph_is_canceled(struct bt_graph *graph)
 {
        return graph ? graph->canceled : BT_FALSE;
 }
+
+BT_HIDDEN
+void bt_graph_remove_connection(struct bt_graph *graph,
+               struct bt_connection *connection)
+{
+       assert(graph);
+       assert(connection);
+       g_ptr_array_remove(graph->connections, connection);
+}