fix capacity of serialized messages #58
Conversation
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
| if (cdr_stream->buffer_capacity < expected_length) { | ||
| uint8_t * new_buffer = static_cast<uint8_t *>(cdr_stream->allocator.allocate(expected_length, cdr_stream->allocator.state)); | ||
| if (NULL == new_buffer) { | ||
| fprintf(stderr, "failed to allocate memory for cdr data\n"); | ||
| return false; | ||
| } | ||
| cdr_stream->allocator.deallocate(cdr_stream->buffer, cdr_stream->allocator.state); | ||
| cdr_stream->buffer = static_cast<uint8_t *>(cdr_stream->allocator.allocate(cdr_stream->buffer_length, cdr_stream->allocator.state)); | ||
| cdr_stream->buffer = new_buffer; | ||
| cdr_stream->buffer_capacity = expected_length; |
There was a problem hiding this comment.
why not using rcutils_uint8_array_resize here?
https://github.com/ros2/rcutils/blob/master/src/uint8_array.c#L74
There was a problem hiding this comment.
I didn't plan to refactor existing code - just fix the failing test / broken logic.
I would suggest refactoring it in a follow up PR.
There was a problem hiding this comment.
I don't think it has anything to do with refactoring. The introduced code here is really the same as the one in rcutils: https://github.com/ros2/rcutils/blob/master/src/uint8_array.c#L91-L98 which also takes care of resetting the length and capacity in case the allocation fails.
There was a problem hiding this comment.
The introduced code here is really the same as the one in rcutils:
My patch doesn't introduce much at all. It essentially just adds the missing cdr_stream->buffer_capacity = expected_length; assignment.
Also using rcutils_uint8_array_resize implies that the original buffer is altered in case the reallocation fails which is neither necessary nor (imo) desired.
|
If we set the buffer capacity here, we should most likely revert this change here: https://github.com/ros2/rmw_connext/pull/417/files |
| if (@(__dds_cpp_msg_type_prefix)_Plugin_serialize_to_cdr_buffer( | ||
| reinterpret_cast<char *>(cdr_stream->buffer), | ||
| &buffer_length_uint, | ||
| &dds_message) != RTI_TRUE) | ||
| { | ||
| cdr_stream->buffer_length = 0; |
There was a problem hiding this comment.
if we print an error message above when reallocation fails, I think we can add an error message here as well.
There was a problem hiding this comment.
This is also unrelated to the change in this PR. I don't know why originally no error message was printed here - maybe that was intentional? Please address this in a separate PR if you think it should output an error message.
Please fill a separate issue for this if you think that code needs to be changed. It doesn't seem related to me. |
|
CI builds with only Connext:
|
delete-merged-branch
bot
deleted the
dirk-thomas/fix-capacity-of-serialized-messages
branch
|
If I'm not mistaken, this could be backported to Foxy. |
Since the code was added originally in ros2/rmw_connext#259 the
buffer_capacityof thercutils_uint8_array_twasn't updated after being reallocated. See http://build.ros2.org/view/Rci/job/Rci__nightly-connext_ubuntu_focal_amd64/29/testReport/rclcpp/TestSerializedMessage/serialization/ for a failing test because of that.The patch is a bit more length since it makes sure to not partially modify the passed in
rcutils_uint8_array_twhen exiting with an error.