temporalio / proposals

Temporal proposals
https://temporal.io
MIT License
69 stars 18 forks source link

.NET SDK - Phase 1 - Final Comment Period #71

Closed cretz closed 1 year ago

cretz commented 1 year ago

.NET SDK - Phase 1 - Proposal Review - Final Comment Period

This is the proposal for the first phase of the .NET SDK.

Please see the initial comment PR at #70 where most discussion took place.


image

nickwilliams-codynamic commented 1 year ago

This looks really good @cretz. I've added a couple of comments, a couple duplicate the conversations from #70 so feel free to close them off if these decisions have been made already.

The only thing I'm struggling with is the Ref pattern, it feels unnatural to me... that being said I don't really understand the proxying requirements which the SDK has. Do you know if there is anywhere I can go and read up about that? I had a look at the Typescript SDK and it seems that everything is just a reference to a function. Which doesn't seem too much difference to having references to a dotnet Func<> on the face of it.

cretz commented 1 year ago

@nickwilliams-codynamic - Thanks for the review!

The only thing I'm struggling with is the Ref pattern, it feels unnatural to me... that being said I don't really understand the proxying requirements which the SDK has. Do you know if there is anywhere I can go and read up about that?

I gave exercises at the bottom of two of your comments above that should help here.

I had a look at the Typescript SDK and it seems that everything is just a reference to a function. Which doesn't seem too much difference to having references to a dotnet Func<> on the face of it.

But you can't have a reference to a instance method as a Func<> without instance of the object in .NET unlike Java and many other languages. The only purpose of the Ref pattern is to make a fake instance so you can reference instance methods. You are more than welcome to instantiate a dummy version to do it. Could also use a lambda where I instantiate a fake instance and you pretend to make a single call so I know which method you want to invoke, but that is ugly IMO (ntm it only works with interface or virtual methods).

If you could show me a good way to reference a (non-virtual) instance method on a class in .NET without instantiating the class, I'd definitely welcome it. I need the method reference for proper typing. The fake instance via DynamicProxy using this Ref pattern is the best I could come up with.

awright415 commented 1 year ago

If you could show me a good way to reference a (non-virtual) instance method on a class in .NET without instantiating the class, I'd definitely welcome it. I need the method reference for proper typing. The fake instance via DynamicProxy using this Ref pattern is the best I could come up with.

Could an open delegate be useful here? Borrowing an example from https://blog.slaks.net/2011/06/open-delegates-vs-closed-delegates.html

Func<string, string> open = (Func<string, string>)
    Delegate.CreateDelegate(
        typeof(Func<string, string>),
        typeof(string).GetMethod("ToUpperInvariant")
    );
cretz commented 1 year ago

Could an open delegate be useful here? Borrowing an example from https://blog.slaks.net/2011/06/open-delegates-vs-closed-delegates.html

Func<string, string> open = (Func<string, string>)
    Delegate.CreateDelegate(
        typeof(Func<string, string>),
        typeof(string).GetMethod("ToUpperInvariant")
    );

@awright415 - The concern is not that I can't get an instance method from a class, it's how the user provides it to the client. For example, you have ExecuteWorkflowAsync. That needs to accept a workflow entry point method so it can derive the workflow name, input type, and return type and generically return it. Say you have these two workflows:

[Workflow]
public static class MyStaticWorkflow
{
    [WorkflowRun]
    public static async Task<string> RunAsync(string arg)
    {
        throw new NotImplementedException();
    }
}

[Workflow]
public class MyInstanceWorkflow
{
    [WorkflowRun]
    public async Task<string> RunAsync(string arg)
    {
        throw new NotImplementedException();
    }
}

Now if you wanted to "execute" the former (start + result), no problem, you can reference that method and everything:

var result = await myClient.ExecuteWorkflowAsync(
    MyStaticWorkflow.RunAsync, "some arg", myOptions);

But how do you do it with an instance method? Yes, you can instantiate:

var instance = new MyInstanceWorkflow();
var result = await myClient.ExecuteWorkflowAsync(
    instance.RunAsync, "some arg", myOptions);

But not only is instantiating something only for its method reference annoying, but constructors can do things for a workflow that make no sense here (and we are going to allow constructors with workflow args, but not require it). Also, how do you do this for referencing an interface or an abstract class? It is perfectly normal to use an interface or abstract class on the client side to reference a workflow. So we landed at suggesting adding this to all classes/interfaces with [WorkflowRun]:

public static readonly MyInstanceWorkflow Ref = Refs<MyInstanceWorkflow>.Instance;

That uses http://www.castleproject.org/projects/dynamicproxy/ to instantiate (so it works with interfaces and abstract classes). So now you can:

var result = await myClient.ExecuteWorkflowAsync(
    MyInstanceWorkflow.Ref.RunAsync, "some arg", myOptions);

We have also considered the proxy approach like so:

var result = await myClient.ExecuteWorkflowAsync<MyInstanceWorkflow>(
    inst => inst.RunAsync("some arg"), myOptions);

But that has many problems including that we can't control the contents lambda, that the method has to be marked virtual (can't dynamic-proxy a non-virtual method), that statics need a separate overload, etc. We also considered post-sharp-like compilation steps, the newer .NET source generation features, etc and all had too many downsides. For F# I'll probably do type providers or similar.

Here's the problem: "What is the easiest way for a user to reference an instance method in C#?". What @nickwilliams-codynamic suggested was to manually define all definitions, which (ignoring the lambda vs class approach) is basically what Refs<MyInstanceWorkflow>.Instance is a shortcut for doing. And remember, we need to apply this to signal and query methods too, so you can't just have some interface they implement, it needs to be free form. I am definitely open to any way to make this better, but we need to make the workflow read well to the user and be flexible at the same time. Any ideas I may have missed? Any other projects besides Azure Durable Functions/Entities I can look to for inspiration on how they handle "instance method references"?

cretz commented 1 year ago

Merging as this discussion as run its course. We will likely be providing source generation as an option for users.