ros2 / rmw_dds_common

Apache License 2.0
11 stars 20 forks source link

make a new private mutex and add updating graph methods #73

Closed iuhilnehc-ynos closed 11 months ago

iuhilnehc-ynos commented 11 months ago

Its intent is to avoid using mutex directly outside, and decrease the replicated code about updating graph in RMWs.

fujitatomoya commented 11 months ago

@iuhilnehc-ynos is this still draft? please ping us when this and related PRs are ready to review. thanks in advance.

iuhilnehc-ynos commented 11 months ago

@fujitatomoya @clalancette

Could you review this PR and these(https://github.com/ros2/rmw_fastrtps/pull/725, https://github.com/ros2/rmw_cyclonedds/pull/474, https://github.com/ros2/rmw_connextdds/pull/134)?

iuhilnehc-ynos commented 11 months ago

@eboasson Thanks for your reply, I really appreciate it.

Firstly, it appears to me that the publish_callback will typically always be the same, so perhaps it would make more sense to store set it when creating the context and not pass it as a parameter every time?

Good idea.

Secondly, I wonder about the naming: update and destroy. To me update sort-of implies it is already present, but I have no trouble accepting it as a shorthand for "update if present, insert if not". However, if the "update" part really means something, then the error handling is arguably incorrect. If it was present, the update succeeded, but the publishing failed: then arguably the entry should remain.

There exists "// Update graph" comments before the original logic, I suppose using the "update" name might be acceptable, regardless of whether the data is inserted or updated, if it already exists. I've used another "destroy" method name to make it a bit clearer where entities are being destroyed. The original logic was to remove the graph data if the publish message failed, so I decided to retain it.

iuhilnehc-ynos commented 11 months ago

CI:

iuhilnehc-ynos commented 11 months ago

CI:

iuhilnehc-ynos commented 11 months ago

CI: