gazebosim / gz-transport

Transport library for component communication based on publication/subscription and service calls.
https://gazebosim.org
Apache License 2.0
29 stars 36 forks source link

Use `std::shared_ptr` for `gz::transport::NodeShared` #484

Closed azeey closed 3 months ago

azeey commented 3 months ago

🦟 Bug fix

Summary

Currently, NodeShared is never destroyed once it's created. This makes it hard for writing tests that do not interfere with each other. This patch uses a reference counted smart pointer (std::shared_ptr) to keep track of NodeShared instances, so that it gets properly destroyed when when all Node instances, which themselves contain NodeShared instances are destroyed.

This was brought up in https://github.com/gazebosim/gz-sim/pull/2334. The INTEGRATION_triggered_publisher test fails reliably in my local tests without this gz-transport PR.

~TODO: This is probably not threadsafe. It's possible to lock a mutex before checking the weak_ptr, but that would make the call to NodeSHared::Instance slower than the previous implementation since it was using std::shared_mutex.~

Checklist

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 94.44444% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 87.85%. Comparing base (eac2e69) to head (1f2d6f4). Report is 6 commits behind head on gz-transport13.

Files Patch % Lines
src/NodeShared.cc 91.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## gz-transport13 #484 +/- ## ================================================== + Coverage 87.69% 87.85% +0.16% ================================================== Files 59 59 Lines 5704 5708 +4 ================================================== + Hits 5002 5015 +13 + Misses 702 693 -9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

azeey commented 3 months ago

I can go ahead and merge this, but I think we should make a prerelease and test gz-sim before making a full release. @iche033 or @caguero can I ask one of you to do that please?