Skip to content

[23042] Bugfix: Properly delete secure endpoints if registration fails#5814

Merged
MiguelCompany merged 3 commits intomasterfrom
fix/23042
May 21, 2025
Merged

[23042] Bugfix: Properly delete secure endpoints if registration fails#5814
MiguelCompany merged 3 commits intomasterfrom
fix/23042

Conversation

@Mario-DL
Copy link
Copy Markdown
Contributor

@Mario-DL Mario-DL commented May 14, 2025 •
edited by cferreiragonz
Loading

Description

This PR fixes a bug in which, when secure writers or readers failed to register, cleanup is not properly performed.

No need for backport since local_actions_on_.._removed() was introduced in 3.2.x.

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • Any new/modified methods have been properly documented using Doxygen.
  • Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
  • NO Changes are backport compatible: they do NOT break ABI nor change library core behavior.
  • Changes are API compatible.
  • N/A New feature has been added to the versions.md file (if applicable).
  • N/A New feature has been documented/Current behavior is correctly described in the documentation.
  • NO Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • N/A If this is a critical bug fix, backports to the critical-only supported branches have been requested.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

Mario-DL added 2 commits May 14, 2025 12:49
@Mario-DL
Add BB test
31e5232 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Add Fix
caeb9dd 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL Mario-DL added this to the v3.2.3 milestone May 14, 2025
@Mario-DL Mario-DL requested review from richiprosima and removed request for richiprosima May 14, 2025 11:02
@github-actions github-actions Bot added the ci-pending PR which CI is running label May 14, 2025
cferreiragonz
cferreiragonz requested changes May 20, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@cferreiragonz cferreiragonz left a comment

Choose a reason for hiding this comment

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

If calling local_actions_on_writer_removed() from StatefulWriter destructor does not have any side-effects I would try to follow that approach.

Comment thread test/blackbox/common/RTPSBlackboxTestsSecurity.cpp Outdated
Comment thread src/cpp/rtps/participant/RTPSParticipantImpl.cpp
Comment thread src/cpp/rtps/participant/RTPSParticipantImpl.cpp
if (!m_security_manager.register_local_writer(SWriter->getGuid(),
param.endpoint.properties, SWriter->getAttributes().security_attributes()))
{
SWriter->local_actions_on_writer_removed();
Copy link
Copy Markdown
Contributor

@cferreiragonz cferreiragonz May 20, 2025

Choose a reason for hiding this comment

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

Should we add a call to local_actions_on_writer_removed() on the StatefulWriter Destructor to ensure that events are always destroyed, instead of manually calling it here?

Copy link
Copy Markdown
Contributor Author

@Mario-DL Mario-DL May 20, 2025

Choose a reason for hiding this comment

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

The local_actions_on_writer_removed() comes from the former deinit() that was explicitly moved out from the StatefulPersistentWriter destructor, in this commit

However I dont see any reason why we should not call it from the StatafulWriter

Copy link
Copy Markdown
Member

@MiguelCompany MiguelCompany May 21, 2025

Choose a reason for hiding this comment

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

Calling virtual functions inside destructors is dangerous. We could have a proxy method that receives a pointer to a writer and performs the call to local_actions_on_writer_removed() and then deletes the writer

@Mario-DL
Refs #23042: Apply Carlos' test suggestion
3f8e48d 
Signed-off-by: Mario-DL <mariodominguez@eprosima.com>
@Mario-DL
Copy link
Copy Markdown
Contributor Author

Mario-DL commented May 21, 2025

This latter commit only changes the test description, maybe with the former ci is enough

@cferreiragonz
Copy link
Copy Markdown
Contributor

cferreiragonz commented May 21, 2025

CI passed in previous commit caeb9dd

@cferreiragonz cferreiragonz added ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. and removed ci-pending PR which CI is running labels May 21, 2025
@MiguelCompany MiguelCompany merged commit c3b71e3 into master May 21, 2025
3 checks passed
@MiguelCompany MiguelCompany deleted the fix/23042 branch May 21, 2025 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MiguelCompany MiguelCompany MiguelCompany left review comments

@cferreiragonz cferreiragonz cferreiragonz approved these changes

Assignees

No one assigned

Labels

ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.

Projects

None yet

Milestone

v3.2.3 - Frozen ❄️

Development

Successfully merging this pull request may close these issues.

3 participants

@Mario-DL @cferreiragonz @MiguelCompany