Azure / azure-functions-dotnet-worker

Azure Functions out-of-process .NET language worker
MIT License
432 stars 184 forks source link

Signalr Extensions using Class-based model is not currently supported #771

Open michaeldaw opened 2 years ago

michaeldaw commented 2 years ago

Worker.Extensions.SignalRService does not currently contains classes required to develop Functions using the Class based model. eg:

Are there plans to support the Class Based Model when using Worker.Extensions.SignalRService?

fabiocav commented 2 years ago

@Y-Sindo have you had a chance to look at this? Some of them should be very simple to bring in.

Y-Sindo commented 2 years ago

I will add a PR to provide some convenient classes such as SignalRConnectionInfo, SignalRInvocationContext soon.

About the serverless hub in isolated-process model, we do have a plan to support it, but we still have some problems to overcome.

Y-Sindo commented 2 years ago

@fabiocav I have added a PR to address the issue, please take a look at it.

fabiocav commented 2 years ago

Fantastic! Thanks @Y-Sindo. We'll review that ASAP.

FYI @anthonychu

michaeldaw commented 2 years ago

Sorry to interrupt the discussion. Just wanted to say thanks for the response.

Arash-Sabet commented 2 years ago

@fabiocav @anthonychu Any idea when the PR associated with this issue will be approved and released? Thanks.

Y-Sindo commented 2 years ago

@fabiocav @kshyju Currently the SignalR class-based model can bind the RPC call parameters to trigger function parameters automatically, I want to know if the bindings in host process can get the parameter info of the trigger functions in worker process, so that we can do similar things in isolated model.

I also have other related questions in #806 .

liliankasem commented 2 years ago

@kshyju looks like the PR was merged last week, any idea on a potential release date?

michaeldaw commented 2 years ago

Thanks to everyone who's worked on this. Will it be part of a release soon? I've started a project that could really use this 😊

Y-Sindo commented 2 years ago

In the latest version 1.7.0, we have provided some built-in POJOs such as SignalRConnectionInfo and SignalRInvocationContext. I will provide a sample later.

For class-based SignalR trigger, it's still in the back log.

duncanthescot commented 1 year ago

What is the current status of this? The last message I see is from a year ago.

Y-Sindo commented 1 year ago

The class-based model can be broken down into two main tasks. The first task involves resolving SignalR method names and arguments directly from the function signatures, without the need for them to be specified in the SignalRTriggerAttribute parameters. However, it's unclear whether we can obtain the function signatures in the host process. If we can, then this task can be easily accomplished. If not, we'll need to rewrite the entire SignalR trigger logic in the worker process. Unfortunately, during my investigation, I encountered a blocker #1479 which has halted progress on this front. We'll have to wait until this issue is resolved before we can move forward.

The second task is related to SignalR negotiation and message sending. This involves using an SDK-style client instead of input binding and output binding. This task is similar to the preview feature known as the SDK types binding in dotnet isolated worker, which is currently in an early stage and not yet supported by SignalR. In the meantime, a possible workaround could be to use the Azure SignalR management SDK . To make the integration with the management SDK easier, I'm currently working on writing a sample and exploring other possible solutions.

Y-Sindo commented 1 year ago

Please see this project for an example to integrate with SignalR management SDK https://github.com/aspnet/AzureSignalR-samples/tree/2e928b19f545b4d66c943569c21b3ef296b0397b/samples/DotnetIsolated-ClassBased

Y-Sindo commented 1 year ago

@fabiocav Edit: One of the benefits of class-based model is that we don't need to specify the SignalR invocation parameter names but still get them bound. The following code snippets are a comparison of non-class-based model and class-based model.

// non-class-based model
[FunctionName("Broadcast")]
public Task Broadcast(
    [SignalRTrigger("Broadcast", "messages", "SendMessage", parameterNames: new string[] {"signalRRemoteInvocationParameter1", "signalRRemoteInvocationParameter2"})]InvocationContext invocationContext, 
    string signalRRemoteInvocationParameter1, 
    string signalRRemoteInvocationParameter2) { }

// class-based model
[Function("Broadcast")]
public Task Broadcast(
    [SignalRTrigger] SignalRInvocationContext invocationContext, 
    string signalRRemoteInvocationParameter1, 
    string signalRRemoteInvocationParameter2) { }

We achieve this in the in-process library by resolving the parameters to bind from the MethodInfo of the function. However, in the case of isolated workers, the host process is unable to obtain the actual MethodInfo of functions within isolated workers; in my test, the method.GetParameters() returns 5 parameters which are JObject invocationContext, IBinder _binder, ExceutionContext _context, ILogger _logger, _cancellationToken. Therefore, we cannot determine the ITriggerBinding.BindingDataContract for a SignalR trigger beforehand without declaring the parameters in the trigger.

Given this, can we get the parameters bound without declaring them in the trigger attribute?

michaeldaw commented 1 year ago

@Y-Sindo If we're still talking about non-class-based mode, I've had to declare the function trigger as in the example below in order to ensure the parameters are correctly bound:

    [Function("MessageFromClient")]
    public async Task ClientCommand(
        [SignalRTrigger("hub", "messages", "MessageFromClient", "parameter1", "parameter2")]
        SignalRInvocationContext invocationContext, string parameter1, string parameter2)
    {
    }

To be clear, this is for a trigger in which a message is coming from the client to the server. The parameters of the SignalRTrigger attribute are "hubName", "category", "eventName", followed by a params list of parameter names.

Edit: One thing I have not been able to do successfully is return a value to the client from a trigger like the above. In earlier projects (before I switched to Isolated-mode), I was able to specify a return value for the function. The return value would be serialized and sent back to the client. Since using isolated mode, I've found that returns values are null/undefined when they get to the client.

michaeldaw commented 1 year ago

In any case, since creating this issue I've learned to live with the non-class-based mode 😅.

Y-Sindo commented 1 year ago

@michaeldaw

@Y-Sindo If we're still talking about non-class-based mode, I've had to declare the function trigger as in the example below in order to ensure the parameters are correctly bound:

    [Function("MessageFromClient")]
    public async Task ClientCommand(
        [SignalRTrigger("hub", "messages", "MessageFromClient", "parameter1", "parameter2")]
        SignalRInvocationContext invocationContext, string parameter1, string parameter2)
    {
    }

To be clear, this is for a trigger in which a message is coming from the client to the server. The parameters of the SignalRTrigger attribute are "hubName", "category", "eventName", followed by a params list of parameter names.

Yes, you're right. To remove the requirement of declaring the method name and parameter names is one of the tasks in serverless hub. Currently I've encountered an issue when trying to implement it. See my earlier comment for detail: https://github.com/Azure/azure-functions-dotnet-worker/issues/771#issuecomment-1524641046

Edit: One thing I have not been able to do successfully is return a value to the client from a trigger like the above. In earlier projects (before I switched to Isolated-mode), I was able to specify a return value for the function. The return value would be serialized and sent back to the client. Since using isolated mode, I've found that returns values are null/undefined when they get to the client.

Confirmed the problem. I have created a new issue #1496 to check the problem. It still needs more investigation.

Thank you for sharing your experience. We appreciate your adaptability in working with the non-class-based mode. If you have any further issues or suggestions, please feel free to let us know. We value your feedback as we continue to improve our software.