Store the node pointer in publisher and subscription

[nnmm]

The rcl_publisher_fini() and rcl_subscription_fini() function now check that the correct node was given to it.

Part 1 of #986.

Signed-off-by: Nikolai Morin [email protected]

[fujitatomoya]

this makes sense to me. some minor fixes need to be addressed.

[nnmm]

Actually, @fujitatomoya, would it be okay to ignore the node argument to the fini() functions entirely (and stating that it is unused in the docs)?

[fujitatomoya]

I think we should keep it. since it explicitly requires that application is responsible to keep the valid node handle object until finalizing publisher(s) or subscription(s).

[nnmm]

I think we should keep it. since it explicitly requires that application is responsible to keep the valid node handle object until finalizing publisher(s) or subscription(s).

Some background: In the Rust client library, I'd like to implement the destructor (drop function) for rcl_publisher_t. This function only has access to the type it's implemented on, so I can't provide a rcl_node_t there. There is a workaround which we have in place, but it would be nice to get rid of it. When I mentioned this in the client library WG, @sloretz remarked that the node argument was unnecessary in the first place and should instead be stored. Then we could create something like rcl_publisher_fini2(rcl_publisher_t *), without the node argument, and this PR was going to be the first step towards that.

I realized we could avoid this ugly "overloaded" function by instead modifying the existing fini functions to accept a nullptr in the second argument, hence my suggestion.

As for the extra argument expressing a lifetime requirement, I think it would be better to just clearly document this requirement. Other types also don't express lifetime requirements this way, e.g. rcl_wait_set_fini() and rcl_node_fini() don't take a context argument.

So, to summarize: ignoring the node argument would be useful, and, in my opiniion, make the fini functions more consistent. Let me know what you think :)

[fujitatomoya]

I realized we could avoid this ugly "overloaded" function by instead modifying the existing fini functions to accept a nullptr in the second argument, hence my suggestion.

agree,

As for the extra argument expressing a lifetime requirement, I think it would be better to just clearly document this requirement.

okay, i am good to go with it.

Other types also don't express lifetime requirements this way, e.g. rcl_wait_set_fini() and rcl_node_fini() don't take a context argument.

you are right on this.

@ivanpauno @wjwwood any thoughts?

[wjwwood]

I think a new error code is order, so I recommended that.

I think this change is good, but I wouldn't go further and remove it, as I said here: #986 (comment)

[wjwwood]

Some background: In the Rust client library, I'd like to implement the destructor (drop function) for rcl_publisher_t. This function only has access to the type it's implemented on, so I can't provide a rcl_node_t there.

You should use rust to ensure that the node is never dropped before the publisher/subscription. So I think the tension you are running into should not be worked around. So actually, this argument has achieved exactly what I intended it to, it made us discuss relative lifetimes of these objects.

You need to use the lifetime feature to ensure this. I know that can be really difficult in dependent types, but its the "right thing to do" imo. If you can ensure that you could create a new accessor and call the fini like:

rcl_publisher_fini(pub, rcl_publisher_get_node(pub));

But I wouldn't recommend that style of thing unless you use the high level language to enforce the order of destruction.

[nnmm]

Okay! Below I'm quote-replying to @wjwwood about some general points, but if nothing new comes up I'm fine with leaving it like it is now (modulo the new error type).

You should use rust to ensure that the node is never dropped before the publisher/subscription.

Definitely, the publisher and subscription have shared pointers of the node. And the wait set and node have shared pointers of the context.

So I think the tension you are running into should not be worked around. So actually, this argument has achieved exactly what I intended it to, it made us discuss relative lifetimes of these objects.

Fair point, though I shouldn't have called it a workaround: we're not avoiding doing things properly, it's just that the code is a bit more complicated than what we'd do if fini() only took one argument.

Also, this pattern isn't used consistently across rcl, so if a "parent" object is not an input to fini(), I'm not sure if it doesn't need to be kept alive, or the function doesn't follow this pattern.

Code-as-documentation also not applicable everywhere. For instance, how long does a subscription that was added to a wait set need to live? It wouldn't be practical to add all the subscriptions as extra parameters to rcl_wait_set_fini() or rcl_wait_set_clear(). Similarly, does the node name string passed to rcl_node_init() get stored or copied?

This is why I think lifetime requirements should be explained via the API docs. In my dream world, lifetimes would be clarified by a combination of a central API conventions doc that states things like "char* are always copied, never stored", and a note about the lifetime of each function parameter not covered by these conventions, for each function.

You need to use the lifetime feature to ensure this. I know that can be really difficult in dependent types, but its the "right thing to do" imo.

If you're suggesting the publisher should hold a reference to the node – we're using shared pointers because the lifetime parameters that result from references are infectious and often result in bad ergonomics. Maybe we'll do an experiment to validate whether it's workable, but my guess is it's not. Though that topic is entirely orthogonal to the rest of the discussion, since the lifetime requirements are the same either way. Sorry for going down a tangent, I guess I just can't leave anything uncommented, especially Rust things :)

[nnmm]

@fujitatomoya @wjwwood I added a new return code and made the fini functions return it, as suggested.

[fujitatomoya]

lgtm

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[nnmm]

@wjwwood Do the latest changes match what you requested?

[nnmm]

Friendly ping @wjwwood

[nnmm]

@wjwwood It's been a few weeks, could you take another look and check if your "changes requested" still applies?

[nnmm]

@wjwwood ping

[nnmm]

I'm closing this and opening a new PR since the "changes requested" is blocking this PR.

[clalancette]

I'm closing this and opening a new PR since the "changes requested" is blocking this PR.

In general, this isn't the way to proceed. Opening new PRs just means all context from the previous one is lost, and it doesn't necessarily mean you'll get a faster response. I'll suggest in the future that you just ping on the current PR rather than opening a new one.

[fujitatomoya]

@nnmm can you reopen this one instead of #1015?

i think #987 (comment) has been addressed by 0e4dce8, so we just need one more maintainer review.

[nnmm]

Okay, sorry if that was bad style. Isn't it the case that one "changes requested" unconditionally blocks the MR from being merged? I thought that was the case, and I had little hope of reaching William after asking for a review four times over a span of more than four months.

[clalancette]

Okay, sorry if that was bad style. Isn't it the case that one "changes requested" unconditionally blocks the MR from being merged?

Not necessarily. The default GitHub UI doesn't enforce this, though you can add rules to make this the case. We haven't done this on the ROS repositories.

Regardless, we still need to have whatever it is resolved. If we have enough consensus, we can move forward without @wjwwood 's approval (or we can eventually get it).

[nnmm]

Thanks for clarifying! I assumed that it acted as a blocker, I wouldn't have closed this otherwise.

[fujitatomoya]

@nnmm can you reopen this?

[nnmm]

It doesn't look like I can, the button is greyed out.

[clalancette]

Yes, same here. I'm guessing the source branch was deleted or something like that.

At this point, please just go ahead and open a new one. In the future we can just leave them open.

[nnmm]

It's probably that I squashed the commit history for the new PR.