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

Fix trying to get topic data that was already removed. #417

Merged
merged 3 commits into from
Aug 6, 2020
Merged

Fix trying to get topic data that was already removed. #417

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08632e8
Fix trying to get topic data that was already removed.
Jul 29, 2020
9c1e52e
Revert "Fix trying to get topic data that was already removed."
Aug 5, 2020
6a5272f
Updated based on suggestions
Aug 5, 2020
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
2 changes: 1 addition & 1 deletion rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ class SubListener : public EventListenerInterface, public eprosima::fastrtps::Su
}

void
data_taken(eprosima::fastrtps::Subscriber * sub)
updateDataCount(eprosima::fastrtps::Subscriber * sub)
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems to be intentional to call this data_taken after if the data is taken from RTPSReader.

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 think so.

But from the sample, it seems there is a case as follow,

FastDDS -> rmw_fastrtps_cpp : there is a data changed coming, set a flag

rclcpp
   spin
      rcl_wait(rmw_wait) -> check the flag
      get_data()
          if true, data_taken() -> reset the flag

But If the data changed removed not in get_data(), rclcpp::spin will get the data again and again because no function will reset the flag

So I think data_taken might be renamed as updateDataCount, and if get_data() failed, it also call updateDataCount.

{
// Make sure to call into Fast-RTPS before taking the lock to avoid an
// ABBA deadlock between internalMutex_ and mutexes inside of Fast-RTPS.
Expand Down
8 changes: 6 additions & 2 deletions rmw_fastrtps_shared_cpp/src/rmw_take.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,16 @@ _take(
data.data = ros_message;
data.impl = info->type_support_impl_;
if (info->subscriber_->takeNextData(&data, &sinfo)) {
info->listener_->data_taken(info->subscriber_);
info->listener_->updateDataCount(info->subscriber_);

if (eprosima::fastrtps::rtps::ALIVE == sinfo.sampleKind) {
if (message_info) {
_assign_message_info(identifier, message_info, &sinfo);
}
*taken = true;
}
} else {
info->listener_->updateDataCount(info->subscriber_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will be called in either way, maybe it can be outside of the if-else sentence. same below.

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 thought this before, this one could move after if so that use only one data_taken, but the latter one seems not because of a return ret; in the middle, and I don't want to update the original logic (such as data_taken() happened before _assign_message_info.

Copy link
Collaborator

Choose a reason for hiding this comment

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

could you elaborate why adding this will solve the problem and it should be added even when data is not taken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rmw_fastrtps_cpp use a variable data_ in SubListener to check if there is a topic data by onNewDataMessage which is called by FastDDS if there is a new change (mp_history->received_change)
but if FastDDS removed the change, it will not call a function to notify an interface of SubListener.

About why or in what kinds of cases will FastDDS remove the change, I need more time to investigate it.

Copy link
Contributor

Choose a reason for hiding this comment

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

About why or in what kinds of cases will FastDDS remove the change, I need more time to investigate it.

Maybe this is hard to investigate. It is better to consult eProsmia team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Barry-Xu-2018

Yes. This PR maybe is just a workaround, not a solution.
It is better to call eProsima team in.

}

return RMW_RET_OK;
Expand Down Expand Up @@ -267,7 +269,7 @@ _take_serialized_message(
data.data = &buffer;
data.impl = nullptr; // not used when is_cdr_buffer is true
if (info->subscriber_->takeNextData(&data, &sinfo)) {
info->listener_->data_taken(info->subscriber_);
info->listener_->updateDataCount(info->subscriber_);

if (eprosima::fastrtps::rtps::ALIVE == sinfo.sampleKind) {
auto buffer_size = static_cast<size_t>(buffer.getBufferSize());
Expand All @@ -285,6 +287,8 @@ _take_serialized_message(
}
*taken = true;
}
} else {
info->listener_->updateDataCount(info->subscriber_);
}

return RMW_RET_OK;
Expand Down