ros2 / rmw_fastrtps

Implementation of the ROS Middleware (rmw) Interface using eProsima's Fast RTPS.
Apache License 2.0
157 stars 117 forks source link

Found incoherent method calls in rmw_create_subscription. #647

Closed Oscarchoi closed 1 year ago

Oscarchoi commented 1 year ago

Bug report

In the following snippet of code, associate_reader(…)(line:104) and dissociate_writer(…)(line:114) do not correspond to each other. https://github.com/ros2/rmw_fastrtps/blob/ccea7a356900a16a2872670b67952c99a72ab65d/rmw_fastrtps_cpp/src/rmw_subscription.cpp#L101-L124 Considering that rmw_ds_common::GraphCache separates the gid storage of readers and writers, I think it has to dissociate the reader that was associated right above. In rmw_create_publisher(...), the corresponding counterpart of the codes, the associated writer is dissociated when publishing fails.

I think it should be modified as follows. https://github.com/Oscarchoi/rmw_fastrtps/commit/e997378b7fd8edbee352815aebe6bfb06e029498 If the existing code is not intended, is it okay to PR the above changes?
By the way, the test code assumes that rmw_create_subscription will always succeed, so this fallback code has no effect.

fujitatomoya commented 1 year ago

I think it should be modified as follows. https://github.com/Oscarchoi/rmw_fastrtps/commit/e997378b7fd8edbee352815aebe6bfb06e029498 If the existing code is not intended, is it okay to PR the above changes?

@MiguelCompany i think this is a bug, can you confirm?

Oscarchoi commented 1 year ago

I fixed formattings: https://github.com/Oscarchoi/rmw_fastrtps/commit/763103944b2e02ea70a66ecf21b509f6cb75e7c8

MiguelCompany commented 1 year ago

@fujitatomoya I think @Oscarchoi is right, and the proposed changes seem correct.

fujitatomoya commented 1 year ago

@Oscarchoi as @MiguelCompany mentioned, this needs to be fixed as bug. Could you create PR with using your fix? I am happy to review 👍

Oscarchoi commented 1 year ago

Thank you both for confirmation! 😃 I'll create PR soon and notify you.

clalancette commented 1 year ago

This was fixed by #649 , so closing this out.