-
Notifications
You must be signed in to change notification settings - Fork 800
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
[16038] Fix SegFault in delete_contained_entities() when compiled with statistics #3048
Conversation
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
…simplified Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: Notice that this feature is not tested in actual tests due to not compile with STATISTICS.
CI must be updated in order to check it, or add a test where in CMakeLists it is manually specified the option FASTDDS_STATISTICS=ON
.
NOTE: Can statistics entities be deleted as normal entities? There is a specific method to delete them eprosima::fastdds::statistics::dds::DomainParticipantImpl::delete_statistics_builtin_entities()
and eprosima::fastdds::statistics::dds::DomainParticipantImpl::disable_statistics_datawriter(const std::string &topic_name)
, maybe is not so trivial to remove them as any other entity.
ADDITION: Check in core/src/fastdds/src/cpp/fastdds/domain/DomainParticipantFactory.cpp
(135) eprosima::fastdds::dds::DomainParticipantFactory::delete_participant(eprosima::fastdds::dds::DomainParticipant *part)
that statistical entities are being deleted before checking if deletion could be done.
This is not totally correct 🙄 |
…e to statistics tests Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
test/unittest/statistics/dds/StatisticsDomainParticipantTests.cpp
Outdated
Show resolved
Hide resolved
@richiprosima please test this |
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@richiprosima please test this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
If compiled with statistics, when a participant calls delete_contained_entities() followed by a call to Factory::delete_participant(), a SegFault was produced.
Proposed solution changes:
Making ParticipantImpl delete_contained_entities() virtual so that the statistics participant can set the builtin_publishers to nullptr.
@Mergifyio backport 2.7.x 2.6.x 2.3.x
Contributor Checklist
versions.md
file (if applicable).Reviewer Checklist