make a new private mutex and add updating graph methods

[iuhilnehc-ynos]

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

[fujitatomoya]

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

[iuhilnehc-ynos]

@fujitatomoya @clalancette

Could you review this PR and these(ros2/rmw_fastrtps#725, ros2/rmw_cyclonedds#474, ros2/rmw_connextdds#134)?

[eboasson]

Looks ok to me, but I do wonder about two things.

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?

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.

This error handling detailed in itself could be an argument for splitting "insert" and "update".

[iuhilnehc-ynos]

@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.

[eboasson]

Looks good to me (insofar as I am able to judge this given my limited knowledge of how the RMW layers I don't do anything with are put together 😂)

[iuhilnehc-ynos]

CI:

• Linux
• Linux-aarch64
• Windows

[clalancette]

Overall, I like the idea. I've left a few changes to think about.

[iuhilnehc-ynos]

CI:

• Linux
• Linux-aarch64
• Windows

[clalancette]

Looks good to me with green CI.

[iuhilnehc-ynos]

CI:

• Linux
• Linux-aarch64
• Windows