Rust SDK phase 1

[Sushisource]

See proposal. Rendered: https://github.com/temporalio/proposals/blob/d06a90dd70d02854185d82a916b0e913313884c3/rust/RustSDK-phase-1.md

[cretz]

No real comments of note, all looks great to me. Just added a couple of unimportant comments.

[cretz]

Part of me wonders if it'd be worth a temporalio-workflows crate and maybe our samples can show a good way to have CI assert that they are no-std or whatever.

[Sushisource]

I'd be totally fine with a separate crate, you were the one who was all about reducing them! 😄

[cretz]

I for sure am, but workflows is a super special case where no-std'ing/WASM-ifying it can justify it being on its own. If you think you can get the same no-std/WASM-ness with it combined with all of the other worker stuff, no prob, just a bit concerned that would be hard (but so long as we have a CI check that confirms all of our test workflows compile w/ no-std, I think we're good).

[Sushisource]

It should be fairly doable simply by having a check in CI that tries to compile the common crate to the wasm backend. As long as that works we're good. If it's a mess of flags, I will move it out to its own thing.

[cretz]

I don't have anything against this, but I will say making each activity as an entire class in Ruby was quite refreshing for separation. I wonder if it's worth exploring an activity trait compared to macros. Meh, probably not. I'd have the same question for workflows implementing a workflow trait instead of having workflow, init, and run macros (would still need handler ones), but again I think it's probably not better than what's here. Just wanted to jot down the thought.

[casperin]

I'm new to Temporal and have not been using it in any other language than Rust. But having one struct (or even multiple) with each activity as a function attached to it seems like a mistake to me.

That &self can't be directly accessible is a sign that it isn't the right abstraction imo. I honestly don't see what you gain from this.

I would rather have an Activity trait, and do state the same way Axum does state. That's essentially what I built for our internal use, and it works quite well.

Ours is a bit more clunky (I had to build it quickly) so I won't reference it here, but if you model activities after how Axum does handlers then there's nothing in the way of ending up in a place where the user can do this

use temporal::{ActContext, Json, State};

async fn my_activity(
    ctx: ActContext,
    State(db): State<sqlx::Sqlite>,
    State(client): State<SomeClient>,
    Json(input): Json<MyInput>,
) -> Result<MyOutput, MyError> {
    unimplemented!()
}

and then

let state = MyState {
    some_client: SomeClient::new(),
};

let worker = Worker::new(state)
    .workflow(my_workflow)
    .activity(my_activity);
    // and many more

worker.listen(connection_config).await

You reference prior experience in other languages and I haven't read this document in full, so there could very well be reasons that I'm unaware of for why this wouldn't work or why you wouldn't want it this way.

[Sushisource]

Thanks for the feedback! The intent is certainly that you can have multiple activity structs.

Mainly I'm wondering exactly what using something like State buys you. You still need to use arc/mutex/interior mutability for state which is shared among activities. I suppose it does make it easier to store per-activitiy-instance state, though, since you could have &mut self now? My feeling is that can simply be stored on the stack. Plus, I'm not sure users would would find it intuitive that struct state is now per-instance rather than shared.

Plus, adding these state parameters to the worker is a concept that would only exist in the Rust SDK. I don't want to do that unless it's necessary, or brings some fairly major benefits.

[casperin]

And thanks for responding. Let me just quickly say that my feedback is just that: feedback. I trust you to make the decisions that's best for you, and I will respect that decision. I've just built a few of these types of things, and so I'm trying to share what I've arrived at. If you find it useful, great. If not, all good. :-)

Anyway.. The state pattern is good because I'm only cloning the part(s) of the state that I actually need. For instance, this is taken directly out of our code-base, and the Client here is cloned out of a more global state with multiple clients and configs loaded at startup time, etc.

Since ours is internal, I just made the rule that the first param must be "derived" from the state and/or ActContext (like Axum's FromRef), and everything else is inputs (and must impl Serialize + Deserialize).

#[activity]
async fn order_preview(
    client: Client,
    items: Vec<OrderItem>,
    country: CountryCode,
    order_date: Date,
) -> Result<OrderPreview, Error> {
    // ...
}

This is then called from a workflow like

wf_context.order_preview(items, country, order_date).await

It's not that I recommend doing it this way exactly, but to me this (or something in this direction) feels more elegant and more "rusty" than functions on a struct.

You still need to use arc/mutex/interior mutability for state which is shared among activities.

Yes. Kind of. I don't see a reason to enforce Arc or anything. Only that the state implements Clone. Then it's up to the user to wrap whatever isn't Clone in an Arc if needed. It is pretty rare that is needed though. At least in my experience, state is used to store clients and database pools, and they are generally already Clone.

I'm not sure users would would find it intuitive that struct state is now per-instance rather than shared.

Why not? It's just a function. It takes some state (it maybe called Self, maybe not) and that's it.

[cretz]

I don't think you want to assume state is shared across all activities of a worker (it is very normal to have a "group" of activity methods on a struct unrelated to another group).

Your example from before assumed related state, can you show an example with your setup of unrelated activity structs/methods? Trying to get an idea of this "state" concept instead of the traditional stateful approach of self.

wf_context.order_preview(items, country, order_date).await

We have learned proxying in this method from older SDKs that this is no good because 1) you don't know it's an activity vs any other call, and 2) there are caller options to pass.

I don't see a reason to enforce Arc or anything

I don't think we have to enforce Arc if the struct is Copy (can't imagine the use case for copy, and of course the user can elide self altogether), but otherwise, I think Arc is a reasonable default since these are called across threads and we own the lifetime of the object anyways (IIUC, though can have some discussion about whether you can use an activity instance in multiple workers as other users in other SDKs have seen a need to). Maybe we can also offer clone_self on the macro and accept Self instead for cloneable receivers? Seems like it just saves the step of let self = self.clone() yourself on the first line.

I'm not sure users would would find it intuitive that struct state is now per-instance rather than shared.

Why not? It's just a function. It takes some state (it maybe called Self, maybe not) and that's it.

IMO a function should only have (optional) self and its input and if a user wants to somehow instantiate local per-function state they may (e.g. cloning something from self), but it doesn't need to be done for them and arguably a confusing feature to add. I don't think we need to build cloning for the user, they can clone things themselves if they need to and when they need to (e.g. only clone something if input is a certain value). And we don't need special support for worker-level state IMO.

[casperin]

I don't think you want to assume state is shared across all activities of a worker (it is very normal to have a "group" of activity methods on a struct unrelated to another group).

Yeah you're right that putting everything on a struct gives you a layer that can be used for grouping state, and what I'm proposing doesn't out of the box. Nothing holds you back from making it possible, but it'd require additional concepts. I think Axum does it by allowing one server to serve multiple routers, and each router has its own state.

I have built a lot of web servers, and I have never needed this.

Your example from before assumed related state, can you show an example with your setup of unrelated activity structs/methods?

Like I said, I don't have a use from actually isolating parts of state from certain activities, but our code currently handles these two just fine:

#[activity]
async fn do_something_in_aws(aws: AwsClient, foo: u8) -> Result<u8, Error> { ... }

#[activity]
async fn do_something_in_db(db: Db, bar: String) -> Result<String, Error> { ... }

// Or if both are needed
#[activity]
async fn do_something((db, aws): (Db, AwsClient), foo: u8, bar: String) -> Result<u8, Error> { ... }

The latter works, because I made the rule that anything we want from the state must go into the first param.

The state to allow this, would look something like

#[derive(Clone, ActivityState)]
struct State {
    aws_client: AwsClient,
    database_pool: Db,
}

The ActivityState macro essentially implements this for each field in the state. (It's a bit more complicated than this, because I also want to give access to the raw ActContext as well.)

impl FromActivityState<State> for AwsClient {
    fn from_activity_state(state: &State) -> AwsClient {
        state.aws_client.clone()
    }
}

Say you have some state that can't be cloned, then you must wrap it in an Arc (and maybe Mutex if you need that too):

struct State {
    counter: Arc<Mutex<usize>>,
}

// And to use it
#[activity]
async fn counter_activity(counter: Arc<Mutex<usize>>, ...) { ... }

Trying to get an idea of this "state" concept instead of the traditional stateful approach of self.

I'd argue that it's not traditional in Rust frameworks. Only libraries. For frameworks it feels like working against the grain.

We have learned proxying in this method from older SDKs that this is no good because 1) you don't know it's an activity vs any other call, and 2) there are caller options to pass.

That's fair. I didn't mean to suggest this "as-is". We do it, enabled by a trait, and it works just fine. It also means we could (in theory) impl the same traits for mock activities in order to test the workflows fully. It is a bit of an acquired taste though. :-)

It's internal code, meaning I'm here to help alleviate confusion. I'm also not convinced it'd be the right way for a more generally available framework.

Maybe we can also offer clone_self on the macro and accept Self instead for cloneable receivers? Seems like it just saves the step of let self = self.clone() yourself on the first line.

Like this?

struct Activities {
    #[activity(clone_self)]
    async fn do_something_in_db(self: Self, bar: String) -> Result<String, Error> {
        self.database_pool.query(bar).await
    }
}

The trade-off here is the magic macro attribute and that it's cloning all of the state, not just the part this specific activity is interested in. This is probably fine since you are working with multiple states. I agree with you that it's not much of a win though.

IMO a function should only have (optional) self and its input and if a user wants to somehow instantiate local per-function state they may (e.g. cloning something from self), but it doesn't need to be done for them and arguably a confusing feature to add. I don't think we need to build cloning for the user, they can clone things themselves if they need to and when they need to (e.g. only clone something if input is a certain value). And we don't need special support for worker-level state IMO.

I believe you are already cloning. You clone the Arc<Self> when the worker calls the activity, no?

When I talk about cloning state, I'm talking about cloning Arcs or whatever the user deemed okay to clone. A database pool is made for cloning for instance. No need to wrap it in an Arc.

[cretz]

I have built a lot of web servers, and I have never needed this.

In my experience, it is very common in Temporal and it's also common in Temporal to start many workers with many activity groups defined by different teams/departments. It is also very normal for the authors of the activities to not know how they are registered on workers (because they don't care, they have authored the self-contained things they need and don't separate state from the method). It is also normal to start an ephemeral worker for just one thing real quick, but the author of the activity shouldn't care, they know their instance represents the instance/state lifetime.

The same way a normal Rust impl may need shared state from its struct, so may a Rust activity. Traditionally we have tried to use languages' existing mechanisms for stateful receiver/self/instance. It may be plenty normal for instance for the same struct of activities to be instantiated twice independently and be registered on independent workers/task-queues and want state isolated (e.g. a mutex or collection). This is especially true with plugins and interceptors that may be reused but need isolated state.

I think splitting state from the instance/methods causes registration and code sharing concerns and also has unclear state lifetimes/sharing from the activity author POV. I think the cost of state splitting is heavier than the cost of an Arc of self personally.

When I talk about cloning state, I'm talking about cloning Arcs or whatever the user deemed okay to clone.

To clarify here, yeah cheap cloning is fine, was talking about the situation where it may not be as cheap and a user may want to only do it in certain circumstances. There is also a Send requirement, but that exists regardless.