Node::get_effective_namespace returns invalid namespace if sub-namespace is empty

[devrite]

Hi everyone

I tested this on foxy but as you can see from:

rclcpp/rclcpp/src/rclcpp/node.cpp

Lines 81 to 95 in 1fc2d58

	RCLCPP_LOCAL
	std::string
	create_effective_namespace(const std::string & node_namespace, const std::string & sub_namespace)
	{
	// Assumption is that both the node_namespace and sub_namespace are conforming
	// and do not need trimming of `/` and other things, as they were validated
	// in other functions already.
	if (node_namespace.back() == '/') {
	// this is the special case where node_namespace is just `/`
	return node_namespace + sub_namespace;
	} else {
	return node_namespace + "/" + sub_namespace;
	}
	}

it returns an namespace with a slash at the end in both cases. rclcpp::expand_topic_or_service_name will then throw an exception it represents an invalid namespace format (e.g., '/' at the end are not allowed).

Is this still the case? Or rclcpp behaving wrong in this case. But since we usually specify namespaces without slashes almost everywhere I can recall using it I think even if it were permitted we should return the namespace without the slash.

BR, Markus

[fujitatomoya]

i think that question is subordinate node can be created w/o subordinate namespace? i guess no...

that's why it generates exception rclcpp::exceptions::InvalidNamespaceError via create_sub_node("") as following.

terminate called after throwing an instance of 'rclcpp::exceptions::InvalidNamespaceError'
  what():  Invalid namespace: namespace must not end with a '/', unless only a '/':
  '/ns/'
      ^

Thread 1 "rclcpp_1656" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007ffff725e859 in __GI_abort () at abort.c:79
#2  0x00007ffff74e4951 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#3  0x00007ffff74f047c in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#4  0x00007ffff74f04e7 in std::terminate() () from /lib/x86_64-linux-gnu/libstdc++.so.6
#5  0x00007ffff74f0799 in __cxa_throw () from /lib/x86_64-linux-gnu/libstdc++.so.6
#6  0x00007ffff7d23bd9 in rclcpp::Node::Node (this=0x5555555e6c70, other=..., sub_namespace="")
    at /root/ros2_ws/colcon_ws/src/ros2/rclcpp/rclcpp/src/rclcpp/node.cpp:246
#7  0x00007ffff7d25334 in rclcpp::Node::create_sub_node (this=0x55555557f380, sub_namespace="")
    at /root/ros2_ws/colcon_ws/src/ros2/rclcpp/rclcpp/src/rclcpp/node.cpp:600
#8  0x000055555555b709 in main (argc=1, argv=0x7fffffff1638)
    at /root/ros2_ws/colcon_ws/src/ros2/ros2_test_prover/prover_rclcpp/src/rclcpp_1656.cpp:8

i guess current behavior makes sense to me. but it would be nice to add test on this case.

CC: @ivanpauno @wjwwood what do you think ❓

[devrite]

Yep I know, but this

rclcpp/rclcpp/include/rclcpp/node.hpp

Lines 1204 to 1226 in 0659d82

	/// Return the effective namespace that is used when creating entities.
	/**
	* The returned namespace is a concatenation of the node namespace and the
	* accumulated sub-namespaces, which is used as the namespace when creating
	* entities which have relative names.
	*
	* For example, consider:
	*
	* ```cpp
	* auto node = std::make_shared<rclcpp::Node>("my_node", "my_ns");
	* node->get_effective_namespace(); // -> "/my_ns"
	* auto sub_node1 = node->create_sub_node("a");
	* sub_node1->get_effective_namespace(); // -> "/my_ns/a"
	* auto sub_node2 = sub_node1->create_sub_node("b");
	* sub_node2->get_effective_namespace(); // -> "/my_ns/a/b"
	* auto sub_node3 = node->create_sub_node("foo");
	* sub_node3->get_effective_namespace(); // -> "/my_ns/foo"
	* node->get_effective_namespace(); // -> "/my_ns"
	* ```
	*
	* \sa get_namespace()
	* \sa get_sub_namespace()
	* \return the sub-namespace string, not including the node's original namespace

reads to me as the effective namespace used for creating any entities (e.g., if it is a sub_node with the sub_namespace, if it is not just its original namespace), while get_namespace will only give you the original namespace should you be a sub_node (usefully if sub_nodes and nodes communicate on some entities with one another). Especially since it is part of any node's interface it should not return invalid namespace, but rather throw an exception itself it is not intended to be called on a normal node. In this case, to avoid boiler plate code in user code, we either adapt the function or provide a new interface for this.

Hence for me calling get_effectinve_namespace on a standard node should return a well formed namespace (i.e., the original ns for normal nodes). Which as of now it does not. If you try to create a sub_node with no sub_namespace it already throws an exception and does not allow creating such a node as you noted.

[devrite]

Also for a reference you may refer to

rclcpp/rclcpp/src/rclcpp/node.cpp

Lines 193 to 196 in 0659d82

	node_waitables_(new rclcpp::node_interfaces::NodeWaitables(node_base_.get())),
	node_options_(options),
	sub_namespace_(""),
	effective_namespace_(create_effective_namespace(this->get_namespace(), sub_namespace_))

Which is part of

rclcpp/rclcpp/src/rclcpp/node.cpp

Lines 143 to 146 in 0659d82

	Node::Node(
	const std::string & node_name,
	const std::string & namespace_,
	const NodeOptions & options)

the normal nodes interface and creates an invalid effective_namespace_.

[fujitatomoya]

@devrite thanks for the explanation, i see that by itself as effective namespace, it should not include / slash at the end.

[devrite]

@fujitatomoya Your welcome. Took me quite some time to identify the issue, as I was originally faced by it when trying to patch image_transport. And thought I was using the interface wrong (one of the tests failed there, because of it.

Edit: As you already stated, it does not fail now. Missed that first.

[devrite]

@fujitatomoya Could you add a call to get_effective_namespace() to both node and sub node in your prover example and throw an exception if there is a trailing slash? This would already satisfy the request of @wjwwood for a regression test I think.

[fujitatomoya]

Reproducible Sample

https://github.com/fujitatomoya/ros2_test_prover/blob/master/prover_rclcpp/src/rclcpp_1656.cpp

root@134f29e8f25f:~/ros2_ws/colcon_ws# ros2 run prover_rclcpp rclcpp_1656
[INFO] [1620346207.926351517] [ns.my_node]: Parent Node's effective name: /ns/
[INFO] [1620346207.926427278] [ns.my_node]: Child Node's effective name: /ns/sub_ns

Parent Node's cannot have effective name.

[fujitatomoya]

@devrite i think you can add above test case around

rclcpp/rclcpp/test/rclcpp/test_node.cpp

Line 164 in 0659d82

TEST_F(TestNode, subnode_get_name_and_namespace) {

[devrite]

Oh thanks, was not sure where I would add it.

[devrite]

@wjwwood Hi if there is no conclusion yet, I would just change MR to use the old behaviour (no empty sub-namespace) but fix the problem for now. If we want to implement the new behaviour later (empty sub-ns allowed), we can open a new issue for that?