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.

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

Add sub entities destruction on FastDDS entities #295

Merged
merged 3 commits into from
Dec 23, 2021

Conversation

Acuadros95
Copy link
Contributor

@Acuadros95 Acuadros95 commented Dec 22, 2021

Internal DataWriters and DataReaders were not being deleted on Agent FastDDS Publisher and Subscriber delete methods:

ReturnCode_t FastDDSParticipant::delete_publisher(
fastdds::dds::Publisher* publisher)
{
return ptr_->delete_publisher(publisher);
}

This implies that delete_publisher may fail, as explained on FastDDS documentation:

A Publisher can only be deleted if all Entities belonging to the Publisher (DataWriters) have already been deleted. Otherwise, the function will issue an error and the Publisher will not be deleted.

This bug caused a thread/memory leak and random response problems using services/clients with reconnection patterns as detected on this issue.

It didn't affect regular publishers/subscribers, as micro-ROS rmw_microxrcedds destroy methods handles the internal datawriter/reader destruction: detail.

@Acuadros95
Copy link
Contributor Author

Integration tests: eProsima/Micro-XRCE-DDS#127

pablogs9
pablogs9 previously approved these changes Dec 22, 2021
@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Windows Build Status

@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Windows Build Status

@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Windows Build Status

@Acuadros95 Acuadros95 merged commit 20df783 into develop Dec 23, 2021
@Acuadros95 Acuadros95 deleted the fix/delete_entities branch December 23, 2021 13:24
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.

3 participants