Update node creation/destruction API documentation. #249

hidmic · 2020-07-07T15:25:35Z

Precisely what the title says.

hidmic · 2020-07-07T15:32:11Z

rmw/include/rmw/rmw.h

- * The security options should always be non-null and encapsulate the
- * essential security configurations for the node and its entities.
+ * \pre The given name must be a valid non-null node name.
+ * \pre The given namespace_ must be a valid non-null node namespace.


API here departs from the rest. Usually, a null argument means RMW_RET_INVALID_ARGUMENT or similar. rmw_create_node would do the same, and then require the caller to ensure name and namespace_ to be valid, precluding null arguments entirely.

I chose to keep the precondition. But we could check instead.

The problem of requiring preconditions that aren't checked at runtime and that the compiler doesn't check is that in the case they aren't satisfied it will probably generate a segfault (in no-release mode, that can be avoided using asserts).
If it's cheap to check, I don't see why not doing that.

Asserts would indeed be required. That still means no checks in production.

If it's cheap to check, I don't see why not doing that.

I can't tell what the cost of validation is.

In this particular case, I don't mind either way as long as it's properly documented.

I can't tell what the cost of validation is.

Yeah, but it does sound like something we can do easily.

In this particular case, I don't mind either way as long as it's properly documented.

I think it was properly documented:

* - context, name, namespace_, or security_options is `NULL`

Correct, the odd thing being that nullity would be checked but validity would be assumed. Here I went for complete assumption. We can go for complete checking. It's the middle ground that I find odd.

See 6c83764 for the alternative.

Correct, the odd thing being that nullity would be checked but validity would be assumed. Here I went for complete assumption. We can go for complete checking. It's the middle ground that I find odd.

I didn't get that part before.
I imagine that for a valid name and namespace you're referring to:

https://github.com/ros2/rmw/blob/master/rmw/include/rmw/validate_node_name.h

https://github.com/ros2/rmw/blob/master/rmw/include/rmw/validate_namespace.h

I think we're using that on rcl but never on the rmw implementations (not sure why the definitions are in rmw, maybe we wanted to use the functions from the rmw implementations (?)).
Repeating the validity check would be probably unnecessary, as rcl already takes care of that.

It's the middle ground that I find odd.

Yeah, it's a bit odd to check "not NULL but maybe invalid".
I'm comfortable with the middle ground though.

So, I'm pushing for a choice because IMO the middle ground makes no sense. If the caller must ensure that names are valid, null names already fails validation. If you get a null name, that means the caller broke the precondition and you're in UB land. Checking for null doesn't make things any better.

If no one strongly disagrees, I'll keep the checks as it's less awkward.

ivanpauno · 2020-07-07T17:23:55Z

rmw/include/rmw/rmw.h

- * The security options should always be non-null and encapsulate the
- * essential security configurations for the node and its entities.
+ * \pre The given name must be a valid non-null node name.
+ * \pre The given namespace_ must be a valid non-null node namespace.


The problem of requiring preconditions that aren't checked at runtime and that the compiler doesn't check is that in the case they aren't satisfied it will probably generate a segfault (in no-release mode, that can be avoided using asserts).
If it's cheap to check, I don't see why not doing that.

rmw/include/rmw/rmw.h

clalancette

I've got a few more suggestions for clarification.

clalancette · 2020-07-07T20:24:58Z

rmw/include/rmw/rmw.h

+ *   - context is not valid i.e. it has been initialized
+ *     by `rmw_init()` but not yet invalidated by
+ *     `rmw_shutdown()`


I'm not sure I understand this sentence. Shouldn't it be more like:

* - context is not valid i.e. it has not yet been initialized * by `rmw_init()` or has been invalidated by * `rmw_shutdown()`

If not, can you clarify further?

Yes! That's what I meant, though not initialized is a slippery slope. We can only safely guard against mismatching implementation identifiers or zero-initialized contexts. See 42bb43b.

clalancette · 2020-07-07T20:25:55Z

rmw/include/rmw/rmw.h

@@ -166,10 +156,9 @@ rmw_get_serialization_format(void);
 * \param[in] name the node name
 * \param[in] namespace_ the node namespace


I know you didn't change this, but I find it odd that namespace_ has a trailing underscore. What do you think about changing that (and updating the documentation to match)?

True. See 42bb43b.

Spoke too soon, 2442051.

Even though it's an argument name and it is within an external C linkage block, the compiler barfs at namespace.

In rcl we are using node_namespace to avoid that problem, which looks a bit nicer that the trailing underscore.
That change sounds a bit unrelated to the PR though.

It's slightly redundant though.

clalancette · 2020-07-07T20:27:37Z

rmw/include/rmw/rmw.h

+ * \pre All publishers, subscribers, services, and clients created from this node must
+ *   have been destroyed upon call. Some rmw implementations may choose to verify this,
+ *   returning `RMW_RET_ERROR` and setting a human readable error message if any entity
+ *   created from this node has not yet been destroyed.


Suggested change

* \pre All publishers, subscribers, services, and clients created from this node must

* have been destroyed upon call. Some rmw implementations may choose to verify this,

* returning `RMW_RET_ERROR` and setting a human readable error message if any entity

* created from this node has not yet been destroyed.

* \pre All publishers, subscribers, services, and clients created from this node must

* have been destroyed prior to this call. Some rmw implementations may verify this,

* returning `RMW_RET_ERROR` and setting a human readable error message if any entity

* created from this node has not yet been destroyed. However, this is not guaranteed and so

* callers should ensure that this is the case before calling this function.

See 42bb43b.

clalancette

Looking good to me, thanks for the updates.

ivanpauno · 2020-07-07T21:01:08Z

rmw/include/rmw/rmw.h

@@ -166,10 +156,9 @@ rmw_get_serialization_format(void);
 * \param[in] name the node name
 * \param[in] namespace_ the node namespace


In rcl we are using node_namespace to avoid that problem, which looks a bit nicer that the trailing underscore.
That change sounds a bit unrelated to the PR though.

hidmic · 2020-07-07T21:41:17Z

One thing I changed in RMW implementations but didn't reflect here yet. Should we specify how memory is to be allocated?

rmw_context_t comes with an rcutils_allocator_t in options (which I went along with it naturally in some places). But there's also rmw_allocate() and friends, and C++ new / delete operators. I don't mind rolling back some changes, but I think having three different allocators that can be used interchangeably is a mess. CC @wjwwood .

clalancette · 2020-07-08T13:24:31Z

rmw_context_t comes with an rcutils_allocator_t in options (which I went along with it naturally in some places). But there's also rmw_allocate() and friends, and C++ new / delete operators. I don't mind rolling back some changes, but I think having three different allocators that can be used interchangeably is a mess. CC @wjwwood .

I agree that this is kind of messy. But it seems orthogonal to this PR; should we go ahead and merge this one, and open an issue for further discussion of this topic?

hidmic · 2020-07-13T18:53:42Z

But it seems orthogonal to this PR; should we go ahead and merge this one, and open an issue for further discussion of this topic?

Yeah, I guess. I do think that the way memory is allocated should be consistent and documented though.

wjwwood · 2020-07-13T20:30:01Z

I would agree with merge then fix in a follow up, just because there's so much inconsistency already.

I think the "right way" to do this is to always use the passed rcutils allocator, or pass one if there's not one being passed atm. We should deprecate the rmw_* allocator functions, and we should convert the rcutils allocator into a C++ allocator if we must use new/delete, otherwise placement new can be used in combination with the rcutils allocator.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2020-07-14T12:49:37Z

Rebased to reflect #248 API changes.

hidmic · 2020-07-14T12:54:11Z

Running CI up to test_rmw_implementation and all RMW implementations:

Linux
Linux-aarch64
macOS
Windows

clalancette

One really minor nit; I'll approve, you can choose whether to accept it or not.

clalancette · 2020-07-14T13:24:03Z

rmw/include/rmw/rmw.h

- * created from this node has not yet been destroyed.
+ * This function will return early if a logical error, such as `RMW_RET_INVALID_ARGUMENT`
+ * or `RMW_RET_INCORRECT_RMW_IMPLEMENTATION`, ensues, leaving the given node handle unchanged.
+ * Otherwise, it will proceed despite errors, freeing as much resources as it can, including


Suggested change

* Otherwise, it will proceed despite errors, freeing as much resources as it can, including

* Otherwise, it will proceed despite errors, freeing as many resources as it can, including

Done in 79648b3.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2020-07-15T13:22:39Z

Alright, all green. Merging.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic requested review from clalancette and ivanpauno July 7, 2020 15:25

hidmic commented Jul 7, 2020

View reviewed changes

ivanpauno reviewed Jul 7, 2020

View reviewed changes

clalancette reviewed Jul 7, 2020

View reviewed changes

hidmic mentioned this pull request Jul 7, 2020

Ensure compliant node construction/destruction API. ros2/rmw_fastrtps#408

Merged

clalancette approved these changes Jul 7, 2020

View reviewed changes

ivanpauno approved these changes Jul 7, 2020

View reviewed changes

This was referenced Jul 7, 2020

Ensure compliant node construction/destruction API. ros2/rmw_cyclonedds#206

Merged

Ensure compliant node construction/destruction API. ros2/rmw_connext#439

Merged

hidmic requested review from clalancette and ivanpauno July 13, 2020 21:26

hidmic added 5 commits July 14, 2020 09:47

Update node creation/destruction API documentation.

fd3fd5a

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Address peer review comments.

bef9236

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Address more peer review comments.

6317019

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Cannot use namespace as argument name.

cd485f4

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Clarify rmw_destroy_node() side effects.

d9930ed

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic force-pushed the hidmic/update-node-docs branch from 6498e2f to d9930ed Compare July 14, 2020 12:49

clalancette approved these changes Jul 14, 2020

View reviewed changes

Address last peer review comment.

79648b3

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic merged commit 63b5c6d into master Jul 15, 2020

delete-merged-branch bot deleted the hidmic/update-node-docs branch July 15, 2020 13:22

ahcorde pushed a commit that referenced this pull request Oct 13, 2020

Update node creation/destruction API documentation. (#249)

011ffc1

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

ahcorde pushed a commit that referenced this pull request Oct 13, 2020

Update node creation/destruction API documentation. (#249)

8fc0348

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update node creation/destruction API documentation. #249

Update node creation/destruction API documentation. #249

hidmic commented Jul 7, 2020

hidmic Jul 7, 2020 •

edited

Loading

ivanpauno Jul 7, 2020

hidmic Jul 7, 2020 •

edited

Loading

ivanpauno Jul 7, 2020

hidmic Jul 7, 2020 •

edited

Loading

hidmic Jul 7, 2020

ivanpauno Jul 7, 2020

hidmic Jul 7, 2020

ivanpauno Jul 7, 2020

clalancette left a comment

clalancette Jul 7, 2020

hidmic Jul 7, 2020 •

edited

Loading

clalancette Jul 7, 2020

hidmic Jul 7, 2020

hidmic Jul 7, 2020 •

edited

Loading

ivanpauno Jul 7, 2020

hidmic Jul 7, 2020

clalancette Jul 7, 2020

hidmic Jul 7, 2020

clalancette left a comment

ivanpauno Jul 7, 2020

hidmic commented Jul 7, 2020 •

edited

Loading

clalancette commented Jul 8, 2020

hidmic commented Jul 13, 2020

wjwwood commented Jul 13, 2020

hidmic commented Jul 14, 2020

hidmic commented Jul 14, 2020 •

edited

Loading

clalancette left a comment

clalancette Jul 14, 2020

hidmic Jul 14, 2020

hidmic commented Jul 15, 2020

		@@ -166,10 +156,9 @@ rmw_get_serialization_format(void);
		* \param[in] name the node name
		* \param[in] namespace_ the node namespace

	* Otherwise, it will proceed despite errors, freeing as much resources as it can, including
	* Otherwise, it will proceed despite errors, freeing as many resources as it can, including

Update node creation/destruction API documentation. #249

Update node creation/destruction API documentation. #249

Conversation

hidmic commented Jul 7, 2020

hidmic Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Jul 7, 2020 • edited Loading

clalancette commented Jul 8, 2020

hidmic commented Jul 13, 2020

wjwwood commented Jul 13, 2020

hidmic commented Jul 14, 2020

hidmic commented Jul 14, 2020 • edited Loading

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Jul 15, 2020

hidmic Jul 7, 2020 •

edited

Loading

hidmic Jul 7, 2020 •

edited

Loading

hidmic Jul 7, 2020 •

edited

Loading

hidmic Jul 7, 2020 •

edited

Loading

hidmic Jul 7, 2020 •

edited

Loading

hidmic commented Jul 7, 2020 •

edited

Loading

hidmic commented Jul 14, 2020 •

edited

Loading