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

[Epic]: Support returning values from client invocations #5280

Closed
3 tasks done
davidfowl opened this issue Sep 18, 2017 · 66 comments
Closed
3 tasks done

[Epic]: Support returning values from client invocations #5280

davidfowl opened this issue Sep 18, 2017 · 66 comments
Assignees
Labels
affected-medium This issue impacts approximately half of our customers api-approved API was approved in API review, it can be implemented area-signalr Includes: SignalR clients and servers Docs This issue tracks updating documentation enhancement This issue represents an ask for new feature or an enhancement to an existing one Epic Groups multiple user stories. Can be grouped under a theme. Needs: Design This issue requires design work before implementating. Priority:0 Work that we can't release without severity-minor This label is used by an internal tool
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented Sep 18, 2017

Support server to clients acks so that reliable messaging can be implemented more easily. This would only be when using Clients.Client(). I think we should go back to the SendAsync (fire and forget) InvokeAsync (wait for ack) naming pattern. That's the one sticking point.

EDIT by @anurse: For clarity, this issue is tracking all work related to allowing values to be returned from client invocations. It also covers allowing the server to wait for a void-returning (or Task-returning) client side method to complete.

Work left for .NET 7

  • [API] Add client result support to Java client #41777
    • What does an async .On method look like? Probably Single<T>
  • w/return results blocks client receive pipeline #41996
    This was already an issue with Task returning .On methods, but client results likely makes it more likely to block on the client side
    [ ] Hub methods can soft-lock the connection #41997
    • Today we detect if you allow parallel hub invocations and throw if you don't when trying to use the feature. This doesn't work if you use IHubContext in the Hub, or if you have multiple waiting results for the same connections Hubs.
    • This is also especially bad in OnConnectedAsync because that's a special method that runs before the receive loop starts, we need to throw/unblock/warn etc. for this
      [ ] Analyzer to warn about strongly-typed hubs and using InvokeAsync with .All, .Group, etc.
      [ ] InvokeAsync void result? Scenario, acks without needing a value
      [ ] [Scaleout] ServerA requests client result from connection on ServerB, ServerB goes down after receiving request, ServerA needs to know somehow so it can error the client result
  • [Scaleout] Consider alternative unique invocation ID generation instead of prepending the connectionID ([SignalR] Add client return results #40811 (comment))
    [ ] Look at performance
    The biggest performance issue I can think of right now is that RawResult allocates and copy the bytes which can be expensive
    [ ] Flow cancellation from server to client
    - Inject CancellationToken into .On methods and send CancelInvocation hub messages to clients
@davidfowl davidfowl changed the title Support Server -> Client ACKs for single clients Support Client -> Server ACKs for single clients Sep 20, 2017
@brettclutch
Copy link

Vote +1 since I was trying to implement exactly this and I was confused in the .net client the difference between connection.InvokeAsync() and connection.SendAsync(). Old SignalR docs don't shine any light and my google foo not returning anything but this reference.

@davidfowl
Copy link
Member Author

What exactly are you trying to do?

@brettclutch
Copy link

brettclutch commented Sep 28, 2017

Whoops on second visit maybe I misunderstood the thought here.

In any case: I wanted to use the c# async/await construct to fire off a request message to the server from the .net client and "await' the server response. I was hoping I could use InvokeAsync to invoke a hub method which would have a return type once the server replies. But found that is not its use case and rolled my own version:

  • Dictionary of "PendingRequests" awaiting responses keyed by a GUID and paired to a TaskCompletionSource.
  • As SignalR.Client On<>() method is triggered by message from server I find the related GUID and return that response using the paired TaskCompletionSource return.

Allows for IMHO nicer .net client code when trying to use SignalR in a Request/Response style. (Which I understand is just one style of SignalR usage and you want to stay agnostic/light)

What the client code looks like:

LoginRequest request = new LoginRequest();
request.User = user;
LoginResponse response = await Server.InvokeAsync(request) as LoginResponse;

What the extension method looks like:

public async Task InvokeAsync(ClientRequest request)
{
var tcs = new TaskCompletionSource();
request.Id = Guid.NewGuid().ToString();
PendingResponse.Add(request.Id, tcs);
await Connection.InvokeAsync(request);
return await tcs.Task;
}

DOWN WITH REST :)

@davidfowl
Copy link
Member Author

@brettclutch

I was hoping I could use InvokeAsync to invoke a hub method which would have a return type once the server replies. But found that is not its use case and rolled my own version:

Can you clarify? This is from client to server or server to client? If it's client to server it's exactly the case.

If you are talking about the server awaiting a client response, then that's what this issue is about and it would only be possible when addressing a single client.

@brettclutch
Copy link

brettclutch commented Sep 28, 2017

This is client to server where client is awaiting a reply from the server where server is addressing a single client and the response from the server is controlled by my server code.

@davidfowl
Copy link
Member Author

Today, the client supports waiting on a response from the server. What more is the server doing? Is it then calling back to some other client and wanting to wait on that response?

@brettclutch
Copy link

Nothing fancy, just looking for simple request/response where I did not see where/how client supports waiting on a response from the server.

To be clear, the response I await is not a simple AWK that server received message, but a return payload from the server as a result of the request.

@davidfowl
Copy link
Member Author

Nothing fancy, just looking for simple request/response where I did not see where/how client supports waiting on a response from the server.

InvokeAsync waits on the server to complete and supports returning results from server to client.

To be clear, the response I await is not a simple AWK that server received message, but a return payload from the server as a result of the request.

Sure.

After reading what you were looking for, I'm curious what code you wrote that made you think it doesn't do exactly what you want.

On the client API

InvokeAsync - wait for the response
SendAsync - fire and forget

@brettclutch
Copy link

Guess I need some clarity or will need to wait for docs to come around sorry.

The crux of it was: How would await InvokeAsync know what the return payload is?

class MessageA
class MessageB

Assume, Sending MessageA via InvokeAsync from client will result in Server sending MessageB back to that single client.

Server:
return Clients.Client(Context.ConnectionId).InvokeAsync("MessageB", MessageB);

Client SignalR "Receive" code:
On("MessageB", () => { HandleMessageBCode })

Client "Calling" code:
await InvokeAsync("MethodWhichReturnsMessageB", MessageA)

Thats where I lost it. Theres no way for the client "calling" code to await the receive of MessageB in the "receive" code.

What am I missing?

@davidfowl
Copy link
Member Author

I just wanted to you to be crystal clear with what you were doing. Code is usually the best way to do that so I appreciate it.

There's no way for the server to wait on the client callback and there's no way for a client callback to return results. This is about the client sending ACKS back to the server (which is what the original issue is about).

Theres no way for the client "calling" code to await the receive of MessageB in the "receive" code.

No that's not the correct way to put it (sorry to be pedantic here but it's important). There is a way for the calling client to wait on the server response. There's no built-in way for the server to wait on a client invocation. The Task returned from the server method represents the call going out to the client but doesn't wait until the client receives and processed that message.

@BrennanConroy
Copy link
Member

@brettclutch
Wouldn't the following do exactly what you want?

Server:
public MessageB MethodWhichReturnsMessageB(MessageA arg)
{
  // Do stuff
  return MessageB;
}

Client:
MessageB messageB = await InvokeAsync<MessageB>("MethodWhichReturnsMessageB", MessageA);

@brettclutch
Copy link

@BrennanConroy, Thanks for posting, that gave me the little bit of info I needed for the lights to click on and understand how it works and yes thats exactly what I was trying to do. I now can clearly see the difference between InvokeAsync and SendAsync on the .net client.

Awesome, I will give that a try tonight. Sorry for the noise.

@hadzim
Copy link

hadzim commented Oct 12, 2017

Thanks for this code implementing RPC (procedure is performed on remote server in this case).
May I ask, if there is some straightforward way to implement RPC on remote client?

What I am trying to achieve is:
1, Client A registers method on Server
2, Client B wants to call this method
2a, Client B calls server with name of the method and input arguments
2b, Server forwards these parameters to Client A
2c, Client A replies to server with result
2d, Server returns this result to Client B

I am able to implement this in asynchrous way only, where Client B requests server and receives only some unique RequestID. Then Client B has to waits until it receives asynchronous message with RequestID and final result of the RPC.

Is there a way to implement points 2a, 2b, 2c and 2d within one call, so ClientB directly receives result?

Something like this pseudocode:

//server code
public Response RPC(Request request)
{
    string whereToForward = methodThatFindWhoRegisteredMethod(request.methodName);
    Response response = Clients.Client(whereToForward ).InvokeAsync<Response>("Request", request);
    return response;
}

@davidfowl
Copy link
Member Author

davidfowl commented Oct 16, 2017

Is there a way to implement points 2a, 2b, 2c and 2d within one call, so ClientB directly receives result?

No. There's no first class way to do this 2c, Client A replies to server with result. Even if they were though, it would still all be asynchronous. I think you mean that you just wanted a way to do it without storing request ids and managing that state yourself. If this feature existed, SignalR would have to do the exact same thing.

@hadzim
Copy link

hadzim commented Oct 17, 2017

Thank you very much for your answer. I will pair request IDs myself.

@davidfowl
Copy link
Member Author

We're not doing this for 2.1. We will revisit later.

@analogrelay analogrelay changed the title Support Client -> Server ACKs for single clients Support returning values from client invocations Oct 4, 2018
@aspnet-hello aspnet-hello added this to the Backlog milestone Dec 17, 2018
@aspnet-hello aspnet-hello added area-signalr Includes: SignalR clients and servers status: Needs Design labels Dec 17, 2018
@aspnet-hello aspnet-hello transferred this issue from aspnet/SignalR Dec 17, 2018
@yasin-uohyd
Copy link

hi @davidfowl , Are we going to release this anytime soon? I'm working on a micro-service orchestration design based on Azure SignalR Service, this feature would make my architecture simple.

@davidfowl
Copy link
Member Author

No, there are no plans to do this in 3.0.

@yasin-uohyd
Copy link

No, there are no plans to do this in 3.0.

Okay. Is there anyway I can achieve this with correct code base.

@hodgezappa
Copy link

I have the same need

@analogrelay analogrelay added Needs: Design This issue requires design work before implementating. and removed status: Needs Design labels Mar 21, 2019
@ByronAP
Copy link

ByronAP commented May 24, 2019

another +1 on this

@rafikiassumani-msft rafikiassumani-msft added this to the 7.0-preview4 milestone Mar 15, 2022
@halter73
Copy link
Member

Review Notes

  • The suggestion of using new Caller and Client methods ISingleClientProxy would be source breaking with existing IHubClients and IHubCallerClients implementations, decorators etc... if someone replace the IHubContext<> in DI, so we'll stick with Single
  • We should reach out to the ASRS team soon to verify they're happy with this design.

Approved API

Client side:

class HubConnection
{
    public virtual IDisposable On(string methodName, Type[] parameterTypes, Func<object?[], object, Task> handler, object state)
+   public virtual IDisposable On(string methodName, Type[] parameterTypes, Func<object?[], object, Task<object?>> handler, object state);
}

public static partial class HubConnectionExtensions
{
    public static IDisposable On(this HubConnection hubConnection, string methodName, Type[] parameterTypes, Func<object?[], Task> handler);
    public static IDisposable On<T1>(this HubConnection hubConnection, string methodName, Func<T1, Task> handler);
+   public static IDisposable On<TResult>(this HubConnection hubConnection, string methodName, Type[] parameterTypes, Func<object?[], Task<T>> handler);
+   public static IDisposable On<TResult>(this HubConnection hubConnection, string methodName, Func<Task<T>> handler);
+   public static IDisposable On<TResult>(this HubConnection hubConnection, string methodName, Func<T> handler);
+   public static IDisposable On<T1, TResult>(this HubConnection hubConnection, string methodName, Func<T1, T> handler);
+   public static IDisposable On<T1, TResult>(this HubConnection hubConnection, string methodName, Func<T1, Task<T>> handler);
+   public static IDisposable On<T1, T2, TResult>(this HubConnection hubConnection, string methodName, Func<T1, T2, Task<T>> handler);
    // etc...
}

TS client side:

+ public on(methodName: string, newMethod: (...args: any[]) => any): void
public on(methodName: string, newMethod: (...args: any[]) => void): void {
    // ...
}

Server side:

namespace Microsoft.AspNetCore.SignalR;

+ public interface ISingleClientProxy : IClientProxy
+ {
+     Task<T> InvokeCoreAsync<T>(string method, object?[] args, CancellationToken cancellationToken = default) => throw new NotImplementedException();
+ }

public interface IHubClients<T>
{
+   T Single(string connectionId) => throw new NotImplementedException();
    T All { get; }
    T AllExcept(IReadOnlyList<string> excludedConnectionIds);
    T Client(string connectionId);
    T Clients(IReadOnlyList<string> connectionIds);
    T Group(string groupName);
    T Groups(IReadOnlyList<string> groupNames);
    T GroupExcept(string groupName, IReadOnlyList<string> excludedConnectionIds);
    T User(string userId);
    T Users(IReadOnlyList<string> userIds);
}

public interface IHubClients : IHubClients<IClientProxy>
{
+   new ISingleClientProxy Single(string connectionId) => throw new NotImplementedException();
}

public interface IHubCallerClients : IHubCallerClients<IClientProxy>
{
+   new ISingleClientProxy Single(string connectionId) => throw new NotImplementedException();
}

public static class ClientProxyExtensions
{
    public static Task SendAsync(this IClientProxy clientProxy, string method, CancellationToken cancellationToken = default);
    public static Task SendAsync(this IClientProxy clientProxy, string method, object? arg1, 
CancellationToken cancellationToken = default);
    // etc...

+   public static Task<T> InvokeAsync<T>(this ISingleClientProxy clientProxy, string method, CancellationToken cancellationToken = default);
+   public static Task<T> InvokeAsync<T>(this ISingleClientProxy clientProxy, string method, object? arg1, CancellationToken cancellationToken = default);
    // etc...
}

public abstract class HubLifetimeManager<THub> where THub : Hub
{
    public abstract Task SendAllAsync(string methodName, object?[] args, CancellationToken cancellationToken = default);
    public abstract Task SendConnectionAsync(string connectionId, string methodName, object?[] args, CancellationToken cancellationToken = default);

+   public virtual Task<T> InvokeConnectionAsync<T>(string connectionId, string methodName, object?[] args, CancellationToken cancellationToken = default) => throw new NotImplementedException();

+   public virtual Task SetConnectionResultAsync(string connectionId, CompletionMessage result) => throw new NotImplementedException();

+   public virtual bool TryGetReturnType(string invocationId, [NotNullWhen(true)] out Type? type)
+   {
+       type = null;
+       return false;
+   }
}

namespace Microsoft.AspNetCore.SignalR.Protocol;
+ public class RawResult
+ {
+     public RawResult(ReadOnlySequence<byte> rawBytes);
+     public ReadOnlySequence<byte> RawSerializedData { get; private set; }
+ }

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Mar 21, 2022
@rafikiassumani-msft rafikiassumani-msft changed the title Support returning values from client invocations [Epic]: Support returning values from client invocations Apr 21, 2022
@rafikiassumani-msft rafikiassumani-msft added Epic Groups multiple user stories. Can be grouped under a theme. Docs This issue tracks updating documentation labels Apr 21, 2022
@BrennanConroy
Copy link
Member

BrennanConroy commented Apr 25, 2022

The initial iteration of client return results is out and will be in preview4 bits.

Users

To use this new feature there are changes you need to make on the server side and the client side.
On the server you need to use the new APIs on IHubContext or IHubCallerClients.

IHubContext<MyHub> context;
int arg = 10;
string result = await context.Single(connectionID).InvokeAsync<string>("ClientMethodName", arg);

or inside a Hub method, the caveat here is that you need to set the MaximumParallelInvocationsPerClient option on the server to be greater than 1, and there are still some issues with this that we're working through in later previews:

public async Task HubMethod(string connectionId)
{
    int arg = 11;
    string result = await Clients.Single(connectionId).InvokeAsync<string>("ClientMethodName", arg);
}

Additionally, strongly-typed hubs can be used so now your interfaces can return a value and be used:

public interface IClient
{
    Task<string> Send(int arg);
}

public class HubT : Hub<IClient>
{
    public async Task HubMethod(string connectionId)
    {
        int arg = 12;
        string result = await Clients.Single(connectionId).Send(arg);
    }
}

And on the client you need to return a value from your .On(...) method:
.NET Client:

hubConnection.On("ClientMethodName", (int someArg) =>
{
    return someArg.ToString();
});

TypeScript Client:

hubConnection.on("ClientMethodName", arg => {
    return `${arg}`;
});

HubLifetimeManager implementors

Three new methods have been added to HubLifetimeManager, they have default implementations (that throw NotImplementedExcetion) so that it's not a breaking change to consume the new bits. Additionally, if the implementation of HubLifetimeManager communicates with other servers then you will need to be aware of the RawResult type which is used to pass the raw serialized bytes of a clients payload to another server without needed to fully deserialize the payload on the intermediary server.

public abstract class HubLifetimeManager<THub> where THub : Hub
{
+    public virtual Task<T> InvokeConnectionAsync<T>(string connectionId, string methodName, object?[] args, CancellationToken cancellationToken = default);
+    public virtual Task SetConnectionResultAsync(string connectionId, CompletionMessage result);
+    public virtual bool TryGetReturnType(string invocationId, [NotNullWhen(true)] out Type? type);
}

IHubProtocol implementors

The main change needed for IHubProtocol is to handle the RawResult type. IInvocationBinder.GetReturnType(string invocationID) can return RawResult which means don't deserialize the result and instead store the raw bytes of the result in the RawResult type. And when serializing the payload if a RawResult type is being written write the raw bytes as if they were already serialized.

Example:
Receiving Json of {"type":3,"result":34,"invocationId":"1234"} and seeing that result is RawResult will mean don't try to deserialize 34 as an int or anything and instead store the bytes [(byte)'3', (byte)'4'] in a RawResult object. And when writing the Json of this RawResult object you wouldn't serialize the byte[] as that would base64 encode the value, you instead directly write (byte)'3' and (byte)'4' to the output.

Work left for .NET 7

  • Add Java client implementation for client results
    • What does an async .On method look like? Probably Single<T>
  • HubConnection.On w/return results blocks client receive pipeline
    • This was already an issue with Task returning .On methods, but client results likely makes it more likely to block on the client side
      • .NET client blocks other .On handlers but does not block the receive loop
      • JS client doesn't block anything, .On handlers for future invokes/sends can still run while blocked on user input, pings/etc. still received and processed
      • Java client blocks receive loop
  • InvokeAsync in Hub methods can soft-lock the connection
    • Today we detect if you allow parallel hub invocations and throw if you don't when trying to use the feature. This doesn't work if you use IHubContext in the Hub, or if you have multiple waiting results for the same connections Hubs.
    • This is also especially bad in OnConnectedAsync because that's a special method that runs before the receive loop starts, we need to throw/unblock/warn etc. for this
  • Analyzer to warn about strongly-typed hubs and using InvokeAsync with .All, .Group, etc.
  • InvokeAsync void result? Scenario, acks without needing a value
  • [Scaleout] ServerA requests client result from connection on ServerB, ServerB goes down after receiving request, ServerA needs to know somehow so it can error the client result
  • [Scaleout] Consider alternative unique invocation ID generation instead of prepending the connectionID ([SignalR] Add client return results #40811 (comment))
  • Look at performance
    • The biggest performance issue I can think of right now is that RawResult allocates and copy the bytes which can be expensive
  • Flow cancellation from server to client
    • Inject CancellationToken into .On methods and send CancelInvocation hub messages to clients

@Aloento
Copy link

Aloento commented May 10, 2022

I'm having an issue where I can't seem to return value from the C# client.
Calling the client method in OnConnectedAsync and waiting for the return, the client can already see the return request, but the server keeps waiting.
7.0.0-preview.4.22251.1 with JSON or MessagePack

@BrennanConroy
Copy link
Member

Ah yeah, thought about that scenario but didn't look into it. It's not going to work and likely never will work because of how OnConnectedAsync is called.

@Aloento
Copy link

Aloento commented May 11, 2022

uh…ok, so maybe someone can mention this in the document.

@BrennanConroy
Copy link
Member

Yep, we'll either document it somewhere or see if we can make it error. Added it to the list above.

@halter73
Copy link
Member

I agree that this should be documented. I think it's even more important to just throw an InvalidOperationException immediately if someone tries to do this.

@DavidErben
Copy link

* [ ]  `HubConnection.On` w/return results blocks client receive pipeline
  
  * This was already an issue with Task returning `.On` methods, but client results likely makes it more likely to block on the client side
    
    * .NET client blocks other `.On` handlers but does not block the receive loop

Hey guys, I just tried this out with .NET 7 RC1 and the methods are still blocking (like before if one returned a Task). Are there any plans to change this behavior for the official .NET 7 release?

We are using this feature to trigger commands on connected IoT devices. Usually a bunch of commands are triggered simultaneously, so it would be cool if they would run in parallel. But if it is not possible it is not a huge deal either.

@halter73
Copy link
Member

Hey guys, I just tried this out with .NET 7 RC1 and the methods are still blocking (like before if one returned a Task). Are there any plans to change this behavior for the official .NET 7 release?

Done. #44014

@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-medium This issue impacts approximately half of our customers api-approved API was approved in API review, it can be implemented area-signalr Includes: SignalR clients and servers Docs This issue tracks updating documentation enhancement This issue represents an ask for new feature or an enhancement to an existing one Epic Groups multiple user stories. Can be grouped under a theme. Needs: Design This issue requires design work before implementating. Priority:0 Work that we can't release without severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests