Add ClientResultsManager #1684

xingsy97 · 2022-09-20T10:42:27Z

Summary of the changes

Implement ClientResultsManager

See #1687 for how to use ClientInvocationManager

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

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

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

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

vicancy · 2022-09-21T06:22:06Z

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

+
+        public void TryCompleteRoutedResult(string connectionId, CompletionMessage message);
+
+        public (Type Type, string ConnectionId, object Tcs, Action<object, CompletionMessage> Completion)? RemoveInvocation(string invocationId);


From an interface design perspective, the method naming needs to be readable and self explainable.

How is serviceMappingMessage related to offline ping? Why AddServiceMappingMessage does not withOfflinePing?

what is invocation and what is routedInvocation? why one has Add one does not? the difference is one has callServerId and one does not?

Is that the caller knows which is routed which is not? could it be the ClientResultManager to handle the logic? Will that make the logic simpler?

When a pod will go offline, all client invocations which were routed by this pod should be cleaned. These invocations are stored in their route server and original caller server.

That's why serviceMappingMessage is related to offline ping.

After a server receives an offline ping from a specific pod, this server should do 2 things

For all invocations called by it, clean those were routed by the pod

For all invocations routed by it, clean those were routed by the pod

AddServiceMappingMessage doesn't involve in this procedure.

The difference between invocation and routed invocation is whether the server is the original caller server for the invocation.

I designed a class ClientResultsManager. It has 2 memebers Caller and Router. The Caller manages all client invocations which are originally created by the server. While the Router manages all invocations which are routed by the server.

vicancy · 2022-09-21T06:30:02Z

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

+
+        public Task<T> AddInvocation<T>(string connectionId, string invocationId, CancellationToken cancellationToken)
+        {
+            var tcs = new TaskCompletionSourceWithCancellation<T>(this, connectionId, invocationId, cancellationToken);


is canellationToken.Register(()=>tcs.SetCancelled()) enough?

See this comment

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

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

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

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

JialinXin · 2022-09-21T06:56:48Z

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

+            }
+            if (_routedInvocations.TryGetValue(invocationId, out var item2))
+            {
+                type = item2.Type;


For routed ones, it seems always return object refer to the add method. Simply make type null and return true?

type will return typeof(RawResult) now.
See details in #1684 (comment) and #1684 (comment)

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

…/azure-signalr into ci-ClientResultsManager

xingsy97 · 2022-09-23T07:08:55Z

See #1687 for how to use ClientInvocationManager

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

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

src/Microsoft.Azure.SignalR.Common/ClientInvocation/TaskCompletionSourceWithCancellation.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.Common/Microsoft.Azure.SignalR.Common.csproj

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

JialinXin · 2022-09-26T05:14:12Z

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

+
+        bool TryGetInvocationReturnType(string invocationId, out Type type);
+
+        void CleanupInvocations(string instanceId);


Routed server has client connection status. This method is not needed, or we may leverage TryCompleteResult() to simply cleanup by connectionId regarding route server finds client disconnect?

vicancy · 2022-10-10T03:42:27Z

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

+        /// <returns></returns>
+        Task<T> AddInvocation<T>(string connectionId, string invocationId, string instanceId, CancellationToken cancellationToken);
+
+        void AddServiceMappingMessage(ServiceMappingMessage serviceMappingMessage);


naming: UpdateServiceMapping?

is this one and the CleanupInvocations one a pair? Give this pair matching names?

Though this method uses Dictionary.AddOrUpdate , it's only designed to add a item to a list rather than entirely update a (key, value) according to our business logic.

Dictionary.AddOrUpdate is used to avoid manual checking whether a key exists in the dictionary or not.

_serviceMappingMessages.AddOrUpdate( serviceMappingMessage.InstanceId, new List<string>() { serviceMappingMessage.InvocationId }, (key, valueList) => { valueList.Add(serviceMappingMessage.InvocationId); return valueList; });

So I think the old naming is better

They are not pair or symmetric. CleanupInvocations cleans _serviceMappingMessages only when a service is offline. _serviceMappingMessages can be cleaned when an invocation is completed normally as well.

vicancy · 2022-10-10T03:55:30Z

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

+
+        bool TryGetInvocationReturnType(string invocationId, out Type type);
+
+        bool ContainsInvocation(string invocationId);


This method is no need? you can directly use TryGetInvocationReturnType to check if it contains Invocation?

Yes.
But considering when we just want to know whether a invocationId exists, using TryGetInvocationReturnType is some kind of confusing.
Because ...ReturnType has nothing to do with our logic.
This method is indeed unnecessary but I'm not sure if it should be removed.

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

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

vicancy · 2022-10-10T04:23:28Z

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

+            _serviceMappingMessages.AddOrUpdate(
+                serviceMappingMessage.InstanceId,
+                new List<string>() { serviceMappingMessage.InvocationId },
+                (key, valueList) => { valueList.Add(serviceMappingMessage.InvocationId); return valueList; });


this valueList.Add is not thread safe

I replace List<string> with ConcurrentBag<string>
Now private readonly ConcurrentDictionary<string, ConcurrentBag<string>> _serviceMappingMessages = new();

I created a new Interface IBaseClientResultsManager and a new class BaseClientResultsManager.
In BaseClientResultsManager, a re-designed _serviceMappingMessages without using List

protected readonly ConcurrentDictionary<string, ConcurrentDictionary<string, bool>> _serviceMapping = new();

and public void AddServiceMappingMessage(ServiceMappingMessage serviceMappingMessage)

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

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

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

…/azure-signalr into ci-ClientResultsManager

vicancy · 2022-10-12T06:17:35Z

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

                {
-                    tcs.SetResult((T)completionMessage.Result);
+                    invocation.RouterInstanceId = serviceMappingMessage.InstanceId;
+                    _pendingInvocations[serviceMappingMessage.InvocationId] = invocation;


what if this invocationId was just removed from _pendingInvocations? Change PendingInvocation to be a reference type?

vicancy · 2022-10-12T06:17:52Z

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

                else
                {
-                    tcs.SetException(new Exception(completionMessage.Error));
+                    throw new InvalidOperationException($"Failed to record a service mapping whose RouterInstanceId '{serviceMappingMessage.InvocationId}' was already existing.");


silent failure is enough?

vicancy · 2022-10-12T06:22:15Z

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

+        public void CleanupInvocations(string instanceId)
        {
-            if (_serviceMapping.TryRemove(instanceId, out var invocationIdDict))
+            foreach (var (invocationId, invocation) in _pendingInvocations.Select(x => (x.Key, x.Value)))


why Select? foreach( var (id, invocation) in _pendingInvocations

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

vicancy · 2022-10-12T06:23:37Z

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

+                }
+                else
+                {
+                    throw new InvalidOperationException($"Failed to record a service mapping whose RouterInstanceId '{serviceMappingMessage.InvocationId}' was already existing.");


is silent ignore enough?

vicancy · 2022-10-12T06:23:52Z

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

+            if (_routedInvocations.TryGetValue(invocationId, out var invocation))
+            {
+                invocation.RouterInstanceId = null;
+                _routedInvocations[invocationId] = invocation;


also here, is reference type better?

vicancy · 2022-10-12T06:24:01Z

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

+
+        public void CleanupInvocations(string instanceId)
+        {
+            foreach (var (invocationId, invocation) in _routedInvocations.Select(x => (x.Key, x.Value)))


also here, why Select?

vicancy · 2022-10-13T02:05:31Z

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

+{
+    internal sealed class DummyClientInvocationManager : IClientInvocationManager
+    {
+        public ICallerClientResultsManager Caller { get; }


defensive code as: Caller => throw new NotSupportedException()

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

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


-    internal record struct PendingInvocation(Type Type, string ConnectionId, string RouterInstanceId, object Tcs, Action<object, CompletionMessage> Complete)
-    {
+    class PendingInvocation {


Any other classes consuming this PendingInvocation? If yes, move to a separate file, if not, move into CallerClientResultManager class and make it private

you can simplify the code as below since only RouterInstanceId is settable
record PendingInvocation(Type Type, string ConnectionId, object Tcs, Action<object, CompletionMessage> Complete) { public string RouterInstanceId {get; set;} }

vicancy · 2022-10-13T02:11:21Z

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

    }

-    internal record struct RoutedInvocation(string ConnectionId, string CallerServerId, string RouterInstanceId)
+    class RoutedInvocation


similar comment here as PendingInvocation

vicancy · 2022-10-13T03:52:28Z

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


-    class PendingInvocation {
-        public PendingInvocation(Type type, string connectionId, string routerInstanceId, object tcs, Action<object, CompletionMessage> complete)
+        private record PendingInvocation(Type Type, string ConnectionId, string RouterInstanceId, object Tcs, Action<object, CompletionMessage> Complete)


remove string RouterInstanceId from ctor

vicancy · 2022-10-13T03:54:56Z

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

+
+namespace Microsoft.Azure.SignalR
+{
+


remove extra line

vicancy · 2022-10-13T03:56:47Z

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

+    internal sealed class CallerClientResultsManager : ICallerClientResultsManager, IInvocationBinder
+    {
+        private readonly ConcurrentDictionary<string, PendingInvocation> _pendingInvocations = new();
+        private readonly string _clientResultManagerId = Guid.NewGuid().ToString();


use ToString("N")?

vicancy · 2022-10-13T03:57:49Z

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

+            {
+                if (invocation.RouterInstanceId == null)
+                {
+                    _pendingInvocations[serviceMappingMessage.InvocationId].RouterInstanceId = serviceMappingMessage.InstanceId;


use invocation.RouterInstanceId instead of another fetch

vicancy · 2022-10-13T03:59:05Z

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

+        {
+            if (_pendingInvocations.TryGetValue(invocationId, out var invocation))
+            {
+                _pendingInvocations[invocationId].RouterInstanceId = null;


When will be the scenario reset the instanceId of the invocation?

vicancy · 2022-10-13T04:01:31Z

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

+                if (_pendingInvocations.TryRemove(message.InvocationId, out _))
+                {
+                    item.Complete(item.Tcs, message);
+                    RemoveServiceMapping(message.InvocationId);


the invocation is already removed

vicancy · 2022-10-13T04:03:12Z

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

+                }
+                else
+                {
+                     throw new InvalidOperationException($"The payload of ClientCompletionMessage whose type is {hubMessage.GetType()} cannot be parsed into CompletionMessage correctly.");


GetType().Name

vicancy · 2022-10-13T04:04:08Z

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

+            {
+                return type;
+            }
+            throw new InvalidOperationException($"Invocation ID '{invocationId}' is not associated with a pending client result.");


Is it possible that invocation is already removed? What happens to the customer when it throws?

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

vicancy · 2022-10-13T06:41:27Z

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

+            var protocol = _hubProtocolResolver.GetProtocol(message.Protocol, null);
+            if (protocol == null)
+            {
+                throw new InvalidOperationException($"Not supported protocol {message.Protocol} by server");


what is the customer experience when it happens?

vicancy · 2022-10-13T06:42:55Z

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

+
+        public ClientInvocationManager(IHubProtocolResolver hubProtocolResolver)
+        {
+            Caller = new CallerClientResultsManager(hubProtocolResolver);


hubProtocolResolver null check

vicancy · 2022-10-13T06:43:16Z

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

+
+        public CallerClientResultsManager(IHubProtocolResolver hubProtocolResolver)
+        {
+            _hubProtocolResolver = hubProtocolResolver;


hubProtocolResolver null check:

_hubProtocolResolver = hubProtocolResolver ?? throw new ArgumentNullException(nameof(hubProtocolResolver))

vicancy · 2022-10-13T06:44:55Z

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

+            }
+        }
+
+        public void RemoveServiceMapping(string invocationId)


Can remove?

vicancy · 2022-10-13T06:45:40Z

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

+        {
+            Debug.Assert(false);
+        }
+        public static new void TrySetCanceled(CancellationToken cancellationToken)


we don't have a void TrySetCanceled?

This reverts commit 4fc63e5, reversing changes made to 15876d6.

xingsy97 · 2022-10-13T08:54:35Z

I caused a wrong merge accidentally. Many commits from PR 1687 were merged in.
Now I reverted the wrong merge. So just ignore

vicancy · 2022-10-13T15:45:18Z

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

+                }
+                else
+                {
+                     var errorMessage = $"The payload of ClientCompletionMessage whose type is {hubMessage.GetType().Name} cannot be parsed into CompletionMessage correctly.";


this looks like a system error and should not happen, customer can do nothing about it. For this case, shall we throw?

vicancy · 2022-10-13T15:47:02Z

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

+
+        public static new void SetException(IEnumerable<Exception> exceptions) => Debug.Assert(false);
+        public static new void SetCanceled() => Debug.Assert(false);
+        public static new void TrySetCanceled(CancellationToken cancellationToken) => Debug.Assert(false);


xingsy97 · 2022-10-14T03:45:15Z

Now RoutedClientResultsManager will not record InstanceId. And its Cleanup is according to ConnectionId rather than InstanceId

xingsy97 added 3 commits September 20, 2022 15:25

add ClientResultsManager

a3451e6

add ClientResultsManager

a33c904

Merge branch 'dev' into ci-ClientResultsManager

b08557f

xingsy97 mentioned this pull request Sep 20, 2022

Support for Client Invocation #1681

Closed

vicancy reviewed Sep 21, 2022

View reviewed changes

JialinXin reviewed Sep 21, 2022

View reviewed changes

xingsy97 added 7 commits September 23, 2022 14:28

re-design ClientResultsManager

96058f5

Merge branch 'ci-ClientResultsManager' of https://github.com/xingsy97…

00348d6

…/azure-signalr into ci-ClientResultsManager

Merge branch 'dev' into ci-ClientResultsManager

a03f434

main logic for client invocation

79a0567

typo fix

28dc3c6

Merge branch 'ci-ClientResultsManager' of https://github.com/xingsy97…

6b504cf

…/azure-signalr into ci-ClientResultsManager

add ClientInvocation main logic

f3281af

xingsy97 mentioned this pull request Sep 23, 2022

Add main logic for Client Invocation #1687

Merged

xingsy97 added 8 commits September 23, 2022 15:18

bug fix

790b6c0

bug fix

f594136

bug fix

9dccae7

update

a7c8c8a

update

517dfaa

update interface, add simple unit tests

cfd493b

update

f1b96c1

update

7a13169

xingsy97 commented Sep 25, 2022

View reviewed changes

src/Microsoft.Azure.SignalR.Common/ClientInvocation/TaskCompletionSourceWithCancellation.cs Show resolved Hide resolved

xingsy97 added 2 commits September 26, 2022 08:17

update

b129204

sync interface

67c9783

JialinXin reviewed Sep 26, 2022

View reviewed changes

src/Microsoft.Azure.SignalR.Common/Microsoft.Azure.SignalR.Common.csproj Outdated Show resolved Hide resolved

JialinXin reviewed Sep 26, 2022

View reviewed changes

src/Microsoft.Azure.SignalR.Common/ClientInvocation/TaskCompletionSourceWithCancellation.cs Outdated Show resolved Hide resolved

JialinXin reviewed Sep 26, 2022

View reviewed changes

vicancy reviewed Oct 10, 2022

View reviewed changes

xingsy97 added 5 commits October 10, 2022 16:22

update

b7d027e

Merge branch 'ci-ClientResultsManager' of https://github.com/xingsy97…

d48ad23

…/azure-signalr into ci-ClientResultsManager

add BaseClientResultsManager and RemoveServiceMappingMessage

55b5fb4

remove independent mapping information dictionary

8811ca0

undo comment of ClientCompletionMessage

7867dd8

vicancy reviewed Oct 12, 2022

View reviewed changes

xingsy97 added 3 commits October 12, 2022 18:03

update

c4f7ad7

update exception

46eaf5a

update

c5aca5a

vicancy reviewed Oct 13, 2022

View reviewed changes

update

80031be

vicancy reviewed Oct 13, 2022

View reviewed changes

update

4d196f0

vicancy reviewed Oct 13, 2022

View reviewed changes

xingsy97 added 5 commits October 13, 2022 15:20

update

7f5d42e

hubProtocolResolver null check

e296cb8

format fix

15876d6

Merge branch 'ci-main' into ci-ClientResultsManager

4fc63e5

Revert bad merge "Merge branch 'ci-main' into ci-ClientResultsManager"

f4e9f8f

This reverts commit 4fc63e5, reversing changes made to 15876d6.

vicancy reviewed Oct 13, 2022

View reviewed changes

route server won't record RouterInstanceId

b97589f

fix bug for TrySetCanceled

8ec3042

JialinXin approved these changes Oct 14, 2022

View reviewed changes

xingsy97 merged commit 724624c into Azure:dev Oct 14, 2022

xingsy97 deleted the ci-ClientResultsManager branch October 14, 2022 06:17


		public void TryCompleteRoutedResult(string connectionId, CompletionMessage message);

		public (Type Type, string ConnectionId, object Tcs, Action<object, CompletionMessage> Completion)? RemoveInvocation(string invocationId);


		bool TryGetInvocationReturnType(string invocationId, out Type type);

		void CleanupInvocations(string instanceId);


		bool TryGetInvocationReturnType(string invocationId, out Type type);

		bool ContainsInvocation(string invocationId);


		namespace Microsoft.Azure.SignalR
		{

Add ClientResultsManager #1684

Add ClientResultsManager #1684

Uh oh!

Conversation

xingsy97 commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xingsy97 Sep 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xingsy97 Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xingsy97 commented Sep 23, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xingsy97 Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xingsy97 Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

xingsy97 commented Sep 20, 2022 •

edited

Loading

xingsy97 Sep 25, 2022 •

edited

Loading

xingsy97 Sep 29, 2022 •

edited

Loading

xingsy97 Oct 10, 2022 •

edited

Loading

xingsy97 Oct 10, 2022 •

edited

Loading