Skip to content

Drop unnecessary mutex lock in .event_mgr()#996

Open
roncapat wants to merge 2 commits into
ros2:rollingfrom
roncapat:patch-2
Open

Drop unnecessary mutex lock in .event_mgr()#996
roncapat wants to merge 2 commits into
ros2:rollingfrom
roncapat:patch-2

Conversation

@roncapat

Copy link
Copy Markdown
Contributor

Description

Closes #991

Is this user-facing behavior change?

Did you use Generative AI?

No

Additional Information

There are two occurrences of such pattern, where an internal shared_ptr<EventsManager> gets copied and returned to the caller. That smart pointer is initialized once in constructor and never re-assigned to point to a different instance. Hence, protecting its copy with the mutex is unnecessary.

@roncapat

roncapat commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

@Yadunund I did a quick search across the repo for trivial cases like this one, and I believe there is another one:

https://github.com/roncapat/rmw_zenoh/blob/cfd6a62215734310656913f312ad6ca96677cf4c/rmw_zenoh_cpp/src/detail/rmw_subscription_data.cpp#L1187-L1191

could you check also this one?

@Yadunund

Copy link
Copy Markdown
Member

@roncapat thanks for catching that too. We can safely get rid of it as GraphCache has a couple different mutexes protecting members

@roncapat

roncapat commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Is this one already ok or is there any pending action needed from myself? :)

@mergify

mergify Bot commented Jul 6, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@jmachowinski

Copy link
Copy Markdown
Contributor

Pulls: #996
Gist: https://gist.githubusercontent.com/jmachowinski/08a05ed51ffc2ce61ab4d9c4a27af1e5/raw/b8abc708ea87345706e03eb41dfe43ad14d613e0/ros2.repos
BUILD args: --cmake-args -DRMW_IMPLEMENTATION=rmw_zenoh_cpp
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/19782

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@jmachowinski

Copy link
Copy Markdown
Contributor

Pulls: #996
Gist: https://gist.githubusercontent.com/jmachowinski/885d6de201c4228e4a5d5808071873b6/raw/b8abc708ea87345706e03eb41dfe43ad14d613e0/ros2.repos
BUILD args: --cmake-args -DRMW_IMPLEMENTATION=rmw_zenoh_cpp
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/19813

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Possible non-necessary mutex acquisition?

3 participants