Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shared state pattern (spin-off of #427) #430

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mxgrey
Copy link
Collaborator

@mxgrey mxgrey commented Nov 20, 2024

This PR is spun out of #427 and focuses entirely on moving the API structs to a "shared state" pattern, reducing the prevalence of Arc in the API without any functional changes.

This one is very subjective so I don't mind discarding it, but I felt that the prevalence of Arc in the API was adding noise that is seldom relevant to the average user. In this PR, I've refactored the API a little so that Node, Subscription, Publisher, Client, and Service each have a respective _State structure which is public and represents the underlying instance. At the same time, Node, Subscription, etc are now type aliases of the form

  • pub type Node = Arc<NodeState>;
  • pub type Subscription<T> = Arc<SubscriptionState<T>>;

This does not lead to any actual change in the structure or memory management of any of these types; it's a purely superficial renaming. But it reduces how often Arc is tossed around in the API and thereby streamlines the 95% (my own estimate) of use cases where Arc is just an implementation detail. At the same time, it does not interfere with the remaining 5% of use cases where the Arc is relevant, e.g. if a user wants to hold onto a Weak<Node> for some reason.

Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey mxgrey mentioned this pull request Mar 10, 2025
@romainreignier
Copy link
Contributor

As a user it seems simpler to avoid the Arc on every rclrs object but for the Node, I often have to clone it everywhere to be able to use the logging macros so if it is not exposed as an Arc<Node>, will the users understand that it is actually an Arc and can be easily cloned?

@mxgrey
Copy link
Collaborator Author

mxgrey commented Mar 10, 2025

will the users understand that it is actually an Arc and can be easily cloned?

This is the main point of controversy that I expect for this PR. I think there are fair arguments in both directions.

We're not exactly hiding anything about what Node is. Anyone who looks at the docs or has an IDE with type hints will immediately see that it's just an alias for Arc<NodeState>, so they should immediately see that it's safe and easy to clone.

There's a fair question of how it looks inside a code review where type hints aren't readily available. Would someone reviewing the code at a glance be concerned when they see node.clone() and immediately be afraid that a new node is being created? They could check the docs and see that's not the case, but would they be annoyed at taking the additional step?

There are plenty examples of other objects in the standard library and third-party libraries where cloning an object creates a new handle to the same object, e.g. Sender, so I don't think this should be too surprising for regular Rust developers.

@romainreignier
Copy link
Contributor

Yes, that's true and in Rust the Clone is not automatic so we can guess that if it is available, its ok to use it.

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.

2 participants