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

Jump to bottom

[16647] Use shared_ptr for internal singletons #3185

Merged
merged 31 commits into from
Jan 17, 2023
Merged

[16647] Use shared_ptr for internal singletons #3185

merged 31 commits into from
Jan 17, 2023

Conversation

MiguelCompany
Copy link
Member

@MiguelCompany MiguelCompany commented Jan 4, 2023 •
edited
Loading

Description

The changes include using shared pointers for the internal singletons of RTPSDomainImpl and LogResources, and keeping a reference to both on DomainParticipantFactory.

I consider the changes ABI safe, because the only ABI incompatibility is on the size of DomainParticipantFactory, where two shared pointers are added at the end. Since the factory cannot be directly constructed by the user (and thus cannot be instantiated on the stack), there's no way of having ABI issues. I already checked that by building the examples before the changes, and updating the shared library after the changes.

This is related to #2537, and can be considered a follow-up of #2894.

@Mergifyio backport 2.8.x

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • N/A Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added.
  • Any new/modified methods have been properly documented using Doxygen.
  • Fast DDS test suite has been run locally.
  • Changes are ABI compatible.
  • Changes are API compatible.
  • Documentation builds and tests pass locally.
  • 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.

Reviewer Checklist

  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@MiguelCompany
Copy link
Member Author

@richiprosima Please test this.

@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

2 similar comments
@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

@MiguelCompany MiguelCompany marked this pull request as ready for review January 13, 2023 12:19
@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

@MiguelCompany
Refs #16647. Moved RTPSDomain::getNewId to source file.
b960827 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Moved RTPSDomain implementation to RTPSDomainImpl.
7561376 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Using shared_ptr for singleton data.
9770438 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. DomainParticipantFactory keeps a reference to RTPSDomain…
49d4528 
…Impl instance.

Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Use method to access Log resources.
ad1fee9 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Refactor Log KillThread.
f93e587 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Using shared_ptr for Log Resouces.
7f456d3 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Rename struct into LogResources.
3b069bb 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Rename instance getter.
d6397a1 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Make instance getter available on private header.
5b8b875 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. DomainParticipantFactory keeps a reference to LogResources.
94c8553 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Removed unused method for_each_participant.
786c2f6 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Add instance getter to RTPSDomainImpl mock.
a067bea 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Moved mock for clientServerEnvironmentCreationOverride.
5d0835d 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Allow injection of custom exit work registration.
a9e96a0 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. New helper singleton for registering atexit functions.
dfc268c 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Removed AuxiliaryBoostFunctor from DomainParticipantFact…
2d07899 
…ory.

Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Add src/cpp to include directories when using try_compile
603b666 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Keep raw reference to resources on Log::Run.
67a32c0 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Safety checks on LogResources::KillThread.
fd1ad93 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Kill log thread on test shutdown.
cfbdfc3 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Moved Log::preprocess to LogResources.
2e2c6dc 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Moved Log::run to LogResources.
f24a144 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Moved Log implementation to LogResources.
20f52ef 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. Flush log on destruction of LogResources before killing …
21b4ead 
…thread.

Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. LogResources moved into detail namespace.
e406b53 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Refs #16647. LogResources attributes made private.
8261245 
Signed-off-by: Miguel Company <[email protected]>
@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

jsan-rt
jsan-rt suggested changes Jan 16, 2023
View reviewed changes
src/cpp/utils/shared_memory/BoostAtExitRegistry.hpp Outdated Show resolved Hide resolved
src/cpp/rtps/RTPSDomainImpl.hpp Show resolved Hide resolved
jsan-rt
jsan-rt previously approved these changes Jan 16, 2023
View reviewed changes
Copy link
Contributor

@jsan-rt jsan-rt left a comment

Choose a reason for hiding this comment

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

LGTM

@jsan-rt
Copy link
Contributor

jsan-rt commented Jan 16, 2023

@richiprosima please test this

@MiguelCompany MiguelCompany added the ci-pending PR which CI is running label Jan 16, 2023
jsan-rt
jsan-rt approved these changes Jan 16, 2023
View reviewed changes
Copy link
Contributor

@jsan-rt jsan-rt left a comment

Choose a reason for hiding this comment

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

This still LGTM.

@MiguelCompany MiguelCompany merged commit f6670e3 into master Jan 17, 2023
@MiguelCompany MiguelCompany deleted the feature/shared-ptr-singletons branch January 17, 2023 08:43
@MiguelCompany
Copy link
Member Author

@Mergifyio backport 2.8.x

mergify bot pushed a commit that referenced this pull request Jan 17, 2023
@MiguelCompany @mergify
Use shared_ptr for internal singletons (#3185)
d3adf2e 
* Refs #16647. Moved RTPSDomain::getNewId to source file.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Moved RTPSDomain implementation to RTPSDomainImpl.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Using shared_ptr for singleton data.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. DomainParticipantFactory keeps a reference to RTPSDomainImpl instance.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Use method to access Log resources.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Refactor Log KillThread.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Using shared_ptr for Log Resouces.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Rename struct into LogResources.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Rename instance getter.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Make instance getter available on private header.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. DomainParticipantFactory keeps a reference to LogResources.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Removed unused method for_each_participant.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Add instance getter to RTPSDomainImpl mock.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Moved mock for clientServerEnvironmentCreationOverride.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Allow injection of custom exit work registration.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. New helper singleton for registering atexit functions.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Using new helper for registering boost at_exit_work.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. SharedSegmentBase keeps reference to BoostAtExitRegistry

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. RobustInterprocessCondition keeps reference to BoostAtExitRegistry

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Removed AuxiliaryBoostFunctor from DomainParticipantFactory.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Add src/cpp to include directories when using try_compile

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Keep raw reference to resources on Log::Run.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Safety checks on LogResources::KillThread.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Kill log thread on test shutdown.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Moved Log::preprocess to LogResources.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Moved Log::run to LogResources.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Moved Log implementation to LogResources.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. Flush log on destruction of LogResources before killing thread.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. LogResources moved into detail namespace.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. LogResources attributes made private.

Signed-off-by: Miguel Company <[email protected]>

* Refs #16647. BoostAtExitRegistry allows registering a function during function execution.

Signed-off-by: Miguel Company <[email protected]>

Signed-off-by: Miguel Company <[email protected]>
(cherry picked from commit f6670e3)

# Conflicts:
#	src/cpp/rtps/RTPSDomain.cpp
#	src/cpp/utils/shared_memory/SharedMemSegment.hpp
@mergify mergify bot mentioned this pull request Jan 17, 2023
Closed
@mergify
Copy link
Contributor

mergify bot commented Jan 17, 2023

backport 2.8.x

✅ Backports have been created

@maloel maloel mentioned this pull request Jan 22, 2023
Merged
@jsan-rt jsan-rt mentioned this pull request Feb 14, 2023
Merged
9 tasks
@JLBuenoLopez JLBuenoLopez mentioned this pull request Jun 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsan-rt jsan-rt jsan-rt approved these changes

Assignees
No one assigned
Labels
ci-pending PR which CI is running
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MiguelCompany @jsan-rt