Add main logic for Client Invocation #1687

xingsy97 · 2022-09-23T07:05:10Z

Summary of the changes

Add main logic for Client Invocation

The implementation for ClientInvocationManager is #1684

src/Microsoft.Azure.SignalR.Common/ClientInvocation/ClientInvocationManager.cs

src/Microsoft.Azure.SignalR.Common/ClientInvocation/ClientResultsManager.cs

src/Microsoft.Azure.SignalR.Common/ClientInvocation/IClientResultsManager.cs

src/Microsoft.Azure.SignalR.Common/ClientInvocation/IRoutedClientResultsManager.cs

src/Microsoft.Azure.SignalR.Common/ClientInvocation/IClientResultsManager.cs

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManagerBase.cs

xingsy97 · 2022-10-14T07:25:48Z

All comments related to CientInvocationManager, CallerClientResultsManager and RoutedClientResulsManager were resolved in #1684

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManagerBase.cs

src/Microsoft.Azure.SignalR/ServerConnections/ServiceConnection.cs

JialinXin · 2022-10-14T10:27:48Z

Remove file Visual Studio 2022/Visualizers/attribcache140.bin?

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManagerBase.cs

src/Microsoft.Azure.SignalR.Management/DependencyInjectionExtensions.cs

src/Microsoft.Azure.SignalR/ClientInvocation/CallerClientResultsManager.cs

src/Microsoft.Azure.SignalR.Common/ServiceConnections/ServiceConnectionBase.cs

src/Microsoft.Azure.SignalR.AspNet/ServerConnections/ServiceConnection.cs

vicancy · 2022-10-21T02:19:54Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManagerBase.cs

@@ -267,6 +288,86 @@ public override Task RemoveFromGroupAsync(string connectionId, string groupName,
            return WriteAckableMessageAsync(message, cancellationToken);
        }

+#if NET7_0_OR_GREATER
+        public override async Task<T> InvokeConnectionAsync<T>(string connectionId, string methodName, object[] args, CancellationToken cancellationToken = default)


since it is in base, does it mean Management API also supports ClientInvocation? @JialinXin ? Shall we throw for Management API?

vicancy · 2022-10-21T02:21:38Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManagerBase.cs

+            }
+            catch (Exception)
+            {
+                throw;


if it is catch and throw, why catch?

updated exception handling logic here

vicancy · 2022-10-21T02:22:38Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManagerBase.cs

+                }
+                else
+                {
+                    throw new ArgumentException(NullOrEmptyStringErrorMessage, nameof(connectionId));


ArgumentException does not sound correct here
Again, what is the user experience when it throws?

will replace it with ArgumentNullException
Throwing exception here follows AspNetCore.SignalR

To know the user experience when it throws, I started a back-track from InvokeConnectionAsync. Finally the back-track broke at this method. Found no reference to this method

vicancy · 2022-10-21T02:25:06Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManagerBase.cs

+                if (clientResultsManager != null)
+                {
+                    var protocol = clientConnectionContext.Protocol;
+                    var message = AppendMessageTracingId(new ClientCompletionMessage(result.InvocationId, connectionId, _callerId, protocol, SerializeCompletionMessage(result, protocol)));


What is ClientCompletionMessage for when it is from Caller? There is an additional result seralization, is it neccessory when Caller case?

Caller need to send back ClientCompletionMessage to service as well. Because service will rely on ClientCompeletionMessage to cleanup.
See here

~~Thus this seralization is also necessary when caller case.~~ Not necessary for caller case. I'll update

Probably we could simplify the case for Caller: make payload empty instead of calling SerializeCompletionMessage?

Yes, I did this.
And the other simplification will not be adopted.
Because we need to distinguish between caller and router to decide whether to use empty payload.
Just knowing the server is caller or router is not enough.

vicancy · 2022-10-21T02:29:17Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManagerBase.cs

+                }
+
+                // Block unknown `results` which belongs to neither Caller nor Router
+                if (clientResultsManager != null)


L338-349 can be simplified to:

if (!ClientInvocationManager.Caller.TryCompleteResult(connectionId, result) || !ClientInvocationManager.Router.TryCompleteResult(connectionId, result) { return; }

The simplification should use && rather than || ?

if (!ClientInvocationManager.Caller.TryCompleteResult(connectionId, result) && !ClientInvocationManager.Router.TryCompleteResult(connectionId, result) { return; }

do return ; when the invocation belongs to neither caller nor router. So &&

It's better not to simplify here. Explaination

src/Microsoft.Azure.SignalR/ServerConnections/ServiceConnection.cs

…se classes

…nto ci-main

JialinXin · 2022-10-21T07:28:28Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManager.cs

+            }
+
+            // when condition `_clientConnectionManager.ClientConnections.TryGetValue` is false
+            throw new InvalidOperationException($"ConnectionId {connectionId} is invalid.");


Why throw here? If target client is not routed to this invoke server, false is expected.

src/Microsoft.Azure.SignalR/ServerConnections/ServiceConnection.cs

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManager.cs

vicancy · 2022-10-21T07:37:03Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManager.cs

+            }
+
+            // when condition `_clientConnectionManager.ClientConnections.TryGetValue` is false
+            throw new InvalidOperationException($"ConnectionId {connectionId} is invalid.");


it is possible that connection is closed during this period

This line is a bug and was deleted.
If target connection is closed during method InvokeConnectionAsync, the triggered exception will be handled by try-catch

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManager.cs

vicancy · 2022-10-21T07:42:31Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManagerBase.cs

+                if (clientResultsManager != null)
+                {
+                    var protocol = clientConnectionContext.Protocol;
+                    var message = AppendMessageTracingId(new ClientCompletionMessage(result.InvocationId, connectionId, _callerId, protocol, SerializeCompletionMessage(result, protocol)));


Probably we could simplify the case for Caller: make payload empty instead of calling SerializeCompletionMessage?

vicancy · 2022-10-24T02:04:03Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManager.cs

                // If the invocation was not added by caller, it will be ignored.
                // The content of error message is useless and will not be exposed to client.
-                _clientInvocationManager.Caller.TryCompleteResult(connectionId, CompletionMessage.WithError(invocationId, "Invocation Failed"));
+                _clientInvocationManager.Caller.TryCompleteResult(connectionId, CompletionMessage.WithError(invocationId, ""));


Error message in CompletionMessage.WithError is not exposed to the customer? Then what is it for?

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManager.cs

vicancy · 2022-10-24T02:06:45Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManager.cs

+                {
+                    instanceId = clientConnectionContext.InstanceId;
+                }
+                var task = _clientInvocationManager.Caller.AddInvocation<T>(connectionId, invocationId, instanceId, cancellationToken);


why move out? if still add invocation when the connectionId is removed from _clientConnectionManager, who will clean the Invocation?

the target connection may not exist in current server

vicancy · 2022-10-24T02:10:10Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManager.cs

+                if (clientResultsManager != null)
+                {
+                    var protocol = clientConnectionContext.Protocol;
+                    var payload = clientResultsManager == _clientInvocationManager.Router


add some comments here

vicancy · 2022-10-24T02:12:46Z

src/Microsoft.Azure.SignalR/ServerConnections/ServiceConnection.cs

+#if NET7_0_OR_GREATER
+            if (RuntimeServicePingMessage.TryGetOffline(pingMessage, out var instanceId))
+            {
+                _clientInvocationManager.Caller.CleanupInvocationsByInstance(instanceId);


no need to clean Router here?

added comments

vicancy · 2022-10-24T08:33:39Z

src/Microsoft.Azure.SignalR/ClientInvocation/CallerClientResultsManager.cs

@@ -53,7 +53,7 @@ public Task<T> AddInvocation<T>(string connectionId, string invocationId, string
                        {
                            tcs.TrySetException(new Exception(completionMessage.Error));
                        }
-                    }) { RouterInstanceId = instanceId }
+                    }) { RouterInstanceId = null }


Suggested change

}) { RouterInstanceId = null }

})

vicancy · 2022-10-24T08:34:13Z

src/Microsoft.Azure.SignalR/ClientInvocation/CallerClientResultsManager.cs

@@ -174,6 +166,8 @@ public bool TryGetInvocationReturnType(string invocationId, out Type type)
            return false;
        }

+        public bool RemoveInvocation(string invocationId) => _pendingInvocations.TryRemove(invocationId, out _);


when it returns bool use TryRemoveInvocation, or change bool to void

vicancy · 2022-10-24T08:35:06Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManager.cs

+            if (nameProvider == null)
+            {
+                throw new ArgumentNullException(nameof(nameProvider));
+            }
+            _callerId = nameProvider.GetName();


Suggested change

if (nameProvider == null)

{

throw new ArgumentNullException(nameof(nameProvider));

}

_callerId = nameProvider.GetName();

_callerId = nameProvider?.GetName() ?? throw new ArgumentNullException(nameof(nameProvider));

vicancy · 2022-10-24T08:36:11Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManager.cs

+                var message = AppendMessageTracingId(new ClientInvocationMessage(invocationId, connectionId, _callerId, SerializeAllProtocols(methodName, args, invocationId)));
+                await WriteAsync(message);


can move to before L133

JialinXin · 2022-10-24T09:18:11Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManager.cs

+
+        public override bool TryGetReturnType(string invocationId, [NotNullWhen(true)] out Type type)
+        {
+            if (_clientInvocationManager.Router.ContainsInvocation(invocationId))


Is this check necessary? Simply try get from caller then try get from router?

Removed this check. Also remove method Router.ContainsInvocation

JialinXin · 2022-10-24T09:27:55Z

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManager.cs

+                    var protocol = clientConnectionContext.Protocol;
+                    // For router server, it should send a ClientCompletionMessage with accurate payload content, which is necessary for the caller server.
+                    // For caller server, the only purpose of sending ClientCompletionMessage is to inform service to cleanup the invocation, which means only InvocationId and ConnectionId make sense. To avoid serialization for useless payload, we assign payload with empty bytes.
+                    var payload = clientResultsManager == _clientInvocationManager.Router


How about move payload assignment along with the check during L156~L162. So you don't have to do another manager type check here.

…nto ci-main

JialinXin · 2022-10-24T09:47:06Z

src/Microsoft.Azure.SignalR/ClientInvocation/CallerClientResultsManager.cs

@@ -70,14 +70,6 @@ public void AddServiceMapping(ServiceMappingMessage serviceMappingMessage)
                {


Initial null and service message only send once, assign directly is enough.

JialinXin · 2022-10-25T01:54:14Z

src/Microsoft.Azure.SignalR/ServerConnections/ServiceConnection.cs

@@ -434,7 +464,34 @@ private ClientConnectionContext RemoveClientConnection(string connectionId)
        {
            _connectionIds.TryRemove(connectionId, out _);
            _clientConnectionManager.TryRemoveClientConnection(connectionId, out var connection);
+#if NET7_0_OR_GREATER
+            _clientInvocationManager.Router.CleanupInvocationsByConnection(connectionId);


Shall we also do this for Caller? In the case server is both router and caller.

* Update Actions to use dotnet build (#1663) * Update windows.yml * Add configurable options (#1654) * Update CI to enable net7.0 preview builds (#1677) * Support net7.0 * update osx/ubuntu. * revert common package upgrade. * update build-source guide and revert net7.0 in src. * Add OrTimeout() * Fix build warnings and add net6 samples (#1676) * Fix build warning * Update docs * Fix build failure * Fix test failure * Fix vlunerability issue (#1678) * Fix vlunerability issue * fix filters * fix filters * Update use-signalr-service.md (#1683) * Change emulator to net6.0 (#1682) * Change emulator to net6.0 * Fix test * Add filter property...... (#1686) * Add filter * Add multiPayload case * Adding comments * Update management-sdk-guide.md (#1689) Improve a confusing statement. The original statement can also be interpreted as this SDK only supports SignalR clients based on ASP.NET Core C#, which confuses users. #1688 * Expose InstanceId in ClientConnectionContext (#1692) * Fix `HubConnectionContext.UserIdentifier` is null when negotiation with Management SDK (#1691) When clients negotiatie with Management SDK and connect to SignalR server, IUserIdProvider might not work as the user ID is set directly in the Management SDK. To make HubConnectionContext.UserIdentifier have the valid value in this case, we should set it before the server accesses it. HubLifetimeManager{THub}.OnConnectedAsync(HubConnectionContext) is the only chance we can set the value. However, we cannot access the Constants.ClaimType.UserId as ASRS system claims are trimmed there. HubConnectionContext.Features is the place where we can store the user Id. The following code is the injection point. https://github.com/dotnet/aspnetcore/blob/v6.0.9/src/SignalR/server/Core/src/HubConnectionHandler.cs#L132-L141 Fixes #1679 * Add ClientInvocationManager (#1684) * add ClientInvocationManager * update CallerClientResultsManager (#1697) add `TryCompleteResult` for `ErrorCompletionMessage` * Implement close on client authentication expiration (#1699) * Add spec for client-invocation. (#1701) * Add spec for client-invocation. * minor update direction for ErrorCompletionMessage * Add client Invocation implementation and UTs (#1687) * Add client Invocation implementation and UTs * [ClientInvocation] Fix cleanup in caller case. (#1702) * Fix cleanup in caller case. * fix UT. * Update src/Microsoft.Azure.SignalR.Common/ClientInvocation/IClientInvocationManager.cs Co-authored-by: Liangying.Wei <lianwei@microsoft.com> Co-authored-by: Liangying.Wei <lianwei@microsoft.com> * Add ClientResultSample (#1703) * Add ClientResultSample * Move file * minor update. * minor update README. * show server/service ex.message * Add Broadcast method and update README * Fix proxy not applied to serverless transient mode (#1708) Fix #1700 * silent IDE0090 check (#1709) * reduce retry interval when auth failed (#1451) * Clean up dependencies. (#1711) * Update dep to GA version. * clean up dependencies * clean management. * fix test dep. Co-authored-by: Liangying.Wei <lianwei@microsoft.com> Co-authored-by: Kevin Guo <105208143+kevinguo-ed@users.noreply.github.com> Co-authored-by: yzt <zityang@microsoft.com> Co-authored-by: Eric Xing <87063252+xingsy97@users.noreply.github.com> Co-authored-by: Terence Fan <stdrickforce@gmail.com>

xingsy97 added 2 commits September 23, 2022 14:34

main logic for client invocation

79a0567

add ClientInvocation main logic

f3281af

xingsy97 mentioned this pull request Sep 23, 2022

Add ClientResultsManager #1684

Merged

xingsy97 added 6 commits September 23, 2022 15:18

bug fix

790b6c0

bug fix

f594136

bug fix

9dccae7

update

a7c8c8a

update

f1b96c1

update

b129204

vicancy reviewed Sep 26, 2022

View reviewed changes

xingsy97 added 10 commits September 27, 2022 10:41

sync with PR 1684 and some updates

f4e143c

minor fix

ee60530

update about nameProvider

d2a3178

Merge branch 'dev' into ci-main

8299c2d

tmp

d18743a

update

ecf28b4

Merge branch 'dev' into ci-main

789427d

solve conflict

a64dec1

Merge branch 'dev' into ci-main

9a57478

update

c8198d5

Merge branch 'dev' into ci-main

1ad6573

JialinXin reviewed Oct 14, 2022

View reviewed changes

update

9265187

xingsy97 added 2 commits October 14, 2022 18:31

minior fix

b93a8c1

new-line character fix

ec252ad

JialinXin reviewed Oct 14, 2022

View reviewed changes

src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManagerBase.cs Outdated Show resolved Hide resolved

JialinXin reviewed Oct 14, 2022

View reviewed changes

src/Microsoft.Azure.SignalR.Management/DependencyInjectionExtensions.cs Outdated Show resolved Hide resolved

minor fix

886b985

vicancy reviewed Oct 21, 2022

View reviewed changes

xingsy97 added 5 commits October 21, 2022 14:48

migrate to ServiceConnection and ServiceLifetimeManager from their ba…

fcb849e

…se classes

Merge branch 'ci-main' of https://github.com/xingsy97/azure-signalr i…

995cadb

…nto ci-main

minor bug fix

d0a36d0

format clean

fb5a38e

bug fix for PingMessage

2d98f5a

JialinXin reviewed Oct 21, 2022

View reviewed changes

vicancy reviewed Oct 21, 2022

View reviewed changes

xingsy97 added 2 commits October 21, 2022 17:55

bug fix & add log

2c58f3b

code clean

e6dcc32

xingsy97 requested review from JialinXin and vicancy October 23, 2022 15:09

vicancy reviewed Oct 24, 2022

View reviewed changes

xingsy97 added 2 commits October 24, 2022 15:19

add RemoveInvocation for caller, improve exception handling

2168535

remove instanceId for caller AddInvocation

8a5356f

vicancy reviewed Oct 24, 2022

View reviewed changes

xingsy97 added 3 commits October 24, 2022 17:02

make RemoveInvocation return void

85f4b9f

update

67ff88c

Merge branch 'dev' into ci-main

51b1271

JialinXin reviewed Oct 24, 2022

View reviewed changes

xingsy97 added 2 commits October 24, 2022 17:36

update

0e6631f

Merge branch 'ci-main' of https://github.com/xingsy97/azure-signalr i…

a80e79f

…nto ci-main

JialinXin reviewed Oct 24, 2022

View reviewed changes

minor fix

a1a38e2

vicancy approved these changes Oct 24, 2022

View reviewed changes

xingsy97 merged commit c959735 into Azure:dev Oct 24, 2022

xingsy97 deleted the ci-main branch October 24, 2022 12:09

JialinXin reviewed Oct 25, 2022

View reviewed changes

		var message = AppendMessageTracingId(new ClientInvocationMessage(invocationId, connectionId, _callerId, SerializeAllProtocols(methodName, args, invocationId)));
		await WriteAsync(message);

		@@ -70,14 +70,6 @@ public void AddServiceMapping(ServiceMappingMessage serviceMappingMessage)
		{

Add main logic for Client Invocation #1687

Add main logic for Client Invocation #1687

Conversation

xingsy97 commented Sep 23, 2022

Summary of the changes

xingsy97 commented Oct 14, 2022

JialinXin commented Oct 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xingsy97 Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

vicancy Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xingsy97 Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

vicancy Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JialinXin Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xingsy97 Oct 21, 2022 •

edited

Loading

vicancy Oct 21, 2022 •

edited

Loading

xingsy97 Oct 21, 2022 •

edited

Loading

vicancy Oct 21, 2022 •

edited

Loading

JialinXin Oct 24, 2022 •

edited

Loading