gazebosim / gz-transport

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

Revert "Use `std::shared_ptr` for `gz::transport::NodeShared` (#484)" #490

Closed azeey closed 6 months ago

azeey commented 6 months ago

๐ŸฆŸ Bug fix

Summary

484 fixed the INTEGRATION_triggered_publisher test in gz-sim8, but introduced a couple of regressions (see https://build.osrfoundation.org/job/gz_sim-ci-gz-sim8-jammy-amd64/82/ and https://build.osrfoundation.org/job/gz_sim-ci-gz-sim8-jammy-amd64/83/):

NetworkHandshake.Updates

Error Message
/home/jenkins/workspace/gz_sim-ci-gz-sim8-jammy-amd64/gz-sim/test/integration/network_handshake.cc:220
Expected: (100u) <= (zPos.size()), actual: 100 vs 70

and ServerRepeat/SceneBroadcasterTest.StateStatic/0 (from ServerRepeat_SceneBroadcasterTest)


Error Message
/home/jenkins/workspace/gz_sim-ci-gz-sim8-jammy-amd64/gz-sim/test/integration/scene_broadcaster_system.cc:588
Expected: (stepMsg.stats().real_time().nsec()) < (_msg.stats().real_time().nsec()), actual: 943661800 vs 44593094

It is possible to fix the regressions, but I did notice deadlocks while testing my fixes locally. The deadlocks occur as NodeShared was being destroyed, so I think it's best to revert this and investigate why the deadlock occurs. I'll open an issue to track it.

Checklist

Note to maintainers: Remember to use Rebase and Merge

๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ๐Ÿ”ธ

codecov[bot] commented 6 months ago

Codecov Report

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

Project coverage is 87.71%. Comparing base (eac2e69) to head (0099c2d). Report is 10 commits behind head on gz-transport13.

Files Patch % Lines
src/NodeShared.cc 92.85% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## gz-transport13 #490 +/- ## ================================================== + Coverage 87.69% 87.71% +0.02% ================================================== Files 59 59 Lines 5704 5708 +4 ================================================== + Hits 5002 5007 +5 + Misses 702 701 -1 ```

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

azeey commented 6 months ago

There is an (expected) ABI failure, but the change hasn't been released yet, so we can go ahead and merge.