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

Async Workers #446

Draft
wants to merge 101 commits into
base: main
Choose a base branch
from
Draft

Async Workers #446

wants to merge 101 commits into from

Conversation

mxgrey
Copy link
Collaborator

@mxgrey mxgrey commented Dec 12, 2024

This PR is the final culmination of all the async execution features that I currently have plans to implement. This builds on top of #421 so there is no need to review this PR until that one is merged, but I'm opening this PR to help make it visible to anyone who wants to try out the full breadth of async features that I've been working on.

The key role of this PR is to introduce the worker concept that was initially proposed in these slides. The API of this implementation has some differences from the initial proposal, but the concept is exactly the same.

Since this is just a draft preview PR, in lieu of a detailed explanation, I'll direct folks to this slide deck which explains the motivation behind the Worker concept.

mxgrey and others added 30 commits October 7, 2024 15:56
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
…thread as the wait set

Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
mxgrey added 28 commits December 2, 2024 18:39
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
/// This trait defines the interface for having an executor run.
pub trait ExecutorRuntime: Send {
/// Get a channel that can add new items for the executor to run.
fn channel(&self) -> Arc<dyn ExecutorChannel>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saw this while reviewing #428, couldn't we use associated types to get away from trait objects here? I don't think we really need the extra flexibility. Do we really plan on doing something like storing executors in a heterogeneous container? (i.e. Vec<Box<dyn ExecutorRuntime>>)

pub trait ExecutorRuntime: Send {
    type ExecutorChannel: ExecutorChannel + Send + Sync;

    fn channel(&self) -> Arc<Self::ExecutorChannel>;

    fn spin(&mut self, conditions: SpinConditions) -> Vec<RclrsError>;

    fn spin_async(
        self,
        conditions: SpinConditions,
    ) -> BoxFuture<'static, (Self, Vec<RclrsError>)>
    where
        Self: Sized;
}

As long as Self is Sized, I think we can also remove the extra Box in the future return

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did consider exactly what you're describing, but the dyn ExecutorChannel gets used inside ExecutorCommands which is held by many different objects including Node, SubscriptionExecutable, ServiceExecutable, and in the future Action will probably need it.

If we used an associated type, then we would need to add a generic argument for the type of the ExecutorRuntime to ExecutorCommands, which would then require us to add a generic argument for the ExecutorRuntime to Node, SubscriptionExecutable, ServiceExecutable, and Client. That would undermine the separation of concerns that we currently enjoy between application code that creates nodes and primitives vs application code that chooses the runtime.

With what you're proposing, to be agnostic to the choice of runtime, user code would need to be like

struct MyNodeData<E: ExecutorRuntime> {
    node: Node<E>,
    subscription: Subscription<Msg>,
    client: Client<E, Srv>,
}

fn setup_my_application<E: ExecutorRuntime>(executor: Executor<E>) -> MyNode<E> {
    let node = executor.create_node("my_node");
    let subscription = node.create_subscription("my_topic", do_something);
    let client = node.create_client("my_service");
    MyNode { node, subscription, client }
}

Probably the worst part of this IMO is that we'll see inconsistencies between what kind of generic parameters are needed by Node vs Subscription vs Client vs Service. And if we change some implementation detail of Subscription or Service, it's possible that we'll need to add a generic parameter for the runtime to them.

On the other hand, using a trait object gives us a clean separation between these concerns, allows all the primitives to have consistent generic parameters, and reduces the coupling between the user-facing API and the implementation details.

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.

4 participants