From 0ca71446a91a686130b8791a566a6d8b2f4982d2 Mon Sep 17 00:00:00 2001 From: eboasson Date: Mon, 11 Nov 2019 21:28:53 +0100 Subject: [PATCH] Address "Precondition not met" on rmw_create_node (#65) (#66) Cyclone's original code for dds_create_domain() appears to create a domain entity that is automatically deleted when the last attached participant disappears. In reality, it leaks a reference while returning DDS_RETCODE_OK, making it appear as-if it is a regular entity that remains into existence until explicitly deleted. The RMW code assumed that it would be automatically deleted when the last node was destroyed and that a subsequent call to rmw_create_node could create the domain anew. This then fails with "precondition not met". In an upcoming fixed version of dds_create_domain() the domain entities will behave normally (return a handle, require an explicit delete). This commit provides a workaround for the bug in the original implementation: by recovering the handle from the first participant the preceding commits that were intended to future-proof the code will ensure that the entity now gets deleted explicitly. Signed-off-by: Erik Boasson --- rmw_cyclonedds_cpp/src/rmw_node.cpp | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/rmw_cyclonedds_cpp/src/rmw_node.cpp b/rmw_cyclonedds_cpp/src/rmw_node.cpp index 078a03e..35aec0a 100644 --- a/rmw_cyclonedds_cpp/src/rmw_node.cpp +++ b/rmw_cyclonedds_cpp/src/rmw_node.cpp @@ -548,9 +548,6 @@ static void unref_ppant() #if SUPPORT_LOCALHOST static void node_gone_from_domain_locked(dds_domainid_t did) { - /* The initial support for dds_create_domain in Cyclone results in domains that get - automatically deleted when the last participant in it disappears. Later versions - return a handle and leave it in existence. */ CddsDomain & dom = gcdds.domains[did]; assert(dom.n_nodes > 0); if (--dom.n_nodes == 0) { @@ -679,6 +676,21 @@ extern "C" rmw_node_t * rmw_create_node( "rmw_create_node: failed to create DDS participant"); return nullptr; } +#if SUPPORT_LOCALHOST + if (gcdds.domains[did].domain_handle == 0) { + /* Original implementation of dds_create_domain(): returned value is 0 (DDS_RETCODE_OK) + and a reference to the domain is leaked, necessitating an explicit call to + dds_delete(). For this, we must recover the handle from the participant. Yikes! + + Pending fixes to Cyclone (https://github.com/eclipse-cyclonedds/cyclonedds/pull/304): + a handle is returned by dds_create_domain() and dds_delete() must be called (so + entirely in line with other entity types). In this case, we don't have to do this + (though there would be no harm in it either). + + Note that "domains_lock" is held all this time. */ + gcdds.domains[did].domain_handle = dds_get_parent(pp); + } +#endif /* Since ROS2 doesn't require anything fancy from DDS Subscribers or Publishers, create a single pair & reuse that */