Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Dynamic Subscription (BONUS: Allocators): rosidl #737

Merged
merged 4 commits into from
Apr 11, 2023
Merged

Dynamic Subscription (BONUS: Allocators): rosidl #737

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dd4cce9
Add zero init for message type support handle
methylDragon Apr 9, 2023
d8f40fa
Fix memory leaks in type description utils
methylDragon Apr 9, 2023
8e63a73
Refactor serialization support to use allocators and refs
methylDragon Apr 10, 2023
a07d7e8
Check if allocator is valid
methylDragon Apr 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions rosidl_runtime_c/include/rosidl_runtime_c/message_type_support_struct.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ struct rosidl_message_type_support_t
rosidl_message_get_type_description_sources_function get_type_description_sources_func;
};

/// Return a rosidl_message_type_support_t struct with members set to `NULL`.
ROSIDL_GENERATOR_C_PUBLIC
rosidl_message_type_support_t rosidl_get_zero_initialized_message_type_support_handle(void);

/// Get the message type support handle specific to this identifier.
/**
* The handle's message typesupport identifier function is returned or if the parameters are NULL
Expand Down
13 changes: 13 additions & 0 deletions rosidl_runtime_c/src/message_type_support.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,19 @@
#include <stdio.h>
#include <string.h>

rosidl_message_type_support_t rosidl_get_zero_initialized_message_type_support_handle(void)
{
static rosidl_message_type_support_t null_message_type_support = {
.typesupport_identifier = NULL,
.data = NULL,
.func = NULL,
.get_type_hash_func = NULL,
.get_type_description_func = NULL,
.get_type_description_sources_func = NULL
};
return null_message_type_support;
}

const rosidl_message_type_support_t * get_message_typesupport_handle(
const rosidl_message_type_support_t * handle, const char * identifier)
{
Expand Down
9 changes: 8 additions & 1 deletion rosidl_runtime_c/src/type_description_utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,13 @@ rosidl_runtime_c_type_description_utils_individual_type_description_is_valid(
goto end;
}

ret = rcutils_hash_map_fini(hash_map);
if (ret != RCUTILS_RET_OK) {
RCUTILS_SET_ERROR_MSG("Could not finalize hash map");
return ret;
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me if you should deallocate the hash_map in this case. I think you might need to. Otherwise it's a guaranteed memory leak, because you didn't deallocate it, but it also is no longer accessible after this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, in this case it isn't used anywhere else, so I think it's okay?

I did this because valgrind was complaining

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we're not talking about the same thing. Do you mean that if you deallocate hash_map here (before return ret;) that valgrind was complaining?

Copy link
Contributor Author

@methylDragon methylDragon Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, I meant that I added the deallocation because valgrind was complaining about the leak

The hash map isn't accessible from outside the function, and deallocating it doesn't cause what it points to to get deallocated, so I don't think there are any issues

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, this is a correct bugfix. That is, rosidl_runtime_c_type_description_utils_get_field_map allocates and adds values to a hash_map, which is the responsibility of the caller to free. Further, the hash map stores a key -> value mapping of char * -> rosidl_runtime_c__type_description__Field *. That is, we are only getting the pointers to the underlying data, not the underlying data itself. Thus, it is entirely appropriate for this function to initialize a hash_map to NULL, have that allocated and filled in by rosidl_runtime_c_type_description_utils_get_field_map, use that data, and then free it.

That said, this is buggy. If we end up at line 693, then we'll skip cleaning up the hash_map. I'll suggest moving this above the if (description->fields.size != map_length) { block, since we already got the data we need from the hash_map.

Finally, this actually seems like a very expensive way to get the number of unique items in the type description field. It would be far more efficient to have a rosidl_runtime_c_type_description_utils_get_size which iterates over the individual descriptions and just returns a count. @methylDragon you don't need to fix this now, but it would be good to open up an issue for follow-up which includes comments like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you know, never mind my comment about this being buggy; the end case was actually handling this. Still, I think if we move this block up above, we can avoid having this in two places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll put opening up a followup issue on my docket for now!

Copy link
Contributor Author

@methylDragon methylDragon Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding rosidl_runtime_c_type_description_utils_get_size, we would still need to check for duplicate individual descriptions, which I think would end up needing a hash set, or a growing list of individual description names that we check against as we're building the count (or we sort it and check for neighbouring duplicates?)

I ended up going with the hash set method (with the hash map construction), but yes we can defer this

Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is still open about getting rid of the duplicate rcutils_hash_map_fini. But it is correct right now, it is just code that is unnecessary. So we can defer that to a follow-up as well.

}
allocator.deallocate(hash_map, allocator.state);

return RCUTILS_RET_OK;

end:
Expand Down Expand Up @@ -853,7 +860,7 @@ rosidl_runtime_c_type_description_utils_type_description_is_valid(
goto end_sequence;
}

return RCUTILS_RET_OK;
ret = RCUTILS_RET_OK;

end_sequence:
rosidl_runtime_c__type_description__IndividualTypeDescription__Sequence__destroy(sorted_sequence);
Expand Down