Skip to content

Conversation

mxgrey
Copy link

@mxgrey mxgrey commented Nov 2, 2024

I was able to recreate the crash that happens in humble, as discussed here. It does seem that in Humble rcl is doing some questionable things with the rcl_node_t address.

This PR offers an alternative solution that does not require us to Box the rcl_node_t. Instead we initialize the rcl_node_t while it is already positioned inside the NodeHandle. Since the NodeHandle has a fixed memory address throughout its lifetime, the rcl_node_t will also have a fixed address.

The only downside is we need to add another variable to NodeHandle to track whether the rcl_node_t was successfully initialized, otherwise it might attempt to call rcl_node_fini on an invalid rcl_node_t instance which lead to bad and confusing behavior in the tests. I think overall this is less disruptive than adding a layer of indirection to rcl_node_t.

Signed-off-by: Michael X. Grey <[email protected]>
Copy link
Owner

@geoff-imdex geoff-imdex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a much nicer fix than using a Box.

@geoff-imdex geoff-imdex merged commit c35f484 into geoff-imdex:main Nov 4, 2024
0 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants