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

Add main logic for Client Invocation #1687

Merged
merged 42 commits into from
Oct 24, 2022
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
79a0567
main logic for client invocation
xingsy97 Sep 23, 2022
f3281af
add ClientInvocation main logic
xingsy97 Sep 23, 2022
790b6c0
bug fix
xingsy97 Sep 23, 2022
f594136
bug fix
xingsy97 Sep 23, 2022
9dccae7
bug fix
xingsy97 Sep 23, 2022
a7c8c8a
update
xingsy97 Sep 23, 2022
f1b96c1
update
xingsy97 Sep 23, 2022
b129204
update
xingsy97 Sep 26, 2022
f4e143c
sync with PR 1684 and some updates
xingsy97 Sep 27, 2022
ee60530
minor fix
xingsy97 Sep 27, 2022
d2a3178
update about nameProvider
xingsy97 Sep 27, 2022
8299c2d
Merge branch 'dev' into ci-main
xingsy97 Sep 28, 2022
d18743a
tmp
xingsy97 Sep 28, 2022
ecf28b4
update
xingsy97 Oct 10, 2022
789427d
Merge branch 'dev' into ci-main
xingsy97 Oct 10, 2022
a64dec1
solve conflict
xingsy97 Oct 14, 2022
9a57478
Merge branch 'dev' into ci-main
xingsy97 Oct 14, 2022
c8198d5
update
xingsy97 Oct 14, 2022
1ad6573
Merge branch 'dev' into ci-main
xingsy97 Oct 14, 2022
9265187
update
xingsy97 Oct 14, 2022
b93a8c1
minior fix
xingsy97 Oct 14, 2022
ec252ad
new-line character fix
xingsy97 Oct 14, 2022
886b985
minor fix
xingsy97 Oct 14, 2022
0c40d03
update
xingsy97 Oct 17, 2022
c337cd3
add new UTs and move existing UTs
xingsy97 Oct 20, 2022
3ba27a2
add & update UTs
xingsy97 Oct 20, 2022
4b1dc1e
Merge branch 'dev' into ci-main
xingsy97 Oct 20, 2022
fcb849e
migrate to ServiceConnection and ServiceLifetimeManager from their ba…
xingsy97 Oct 21, 2022
995cadb
Merge branch 'ci-main' of https://github.com/xingsy97/azure-signalr i…
xingsy97 Oct 21, 2022
d0a36d0
minor bug fix
xingsy97 Oct 21, 2022
fb5a38e
format clean
xingsy97 Oct 21, 2022
2d98f5a
bug fix for PingMessage
xingsy97 Oct 21, 2022
2c58f3b
bug fix & add log
xingsy97 Oct 21, 2022
e6dcc32
code clean
xingsy97 Oct 21, 2022
2168535
add RemoveInvocation for caller, improve exception handling
xingsy97 Oct 24, 2022
8a5356f
remove instanceId for caller AddInvocation
xingsy97 Oct 24, 2022
85f4b9f
make RemoveInvocation return void
xingsy97 Oct 24, 2022
67ff88c
update
xingsy97 Oct 24, 2022
51b1271
Merge branch 'dev' into ci-main
xingsy97 Oct 24, 2022
0e6631f
update
xingsy97 Oct 24, 2022
a80e79f
Merge branch 'ci-main' of https://github.com/xingsy97/azure-signalr i…
xingsy97 Oct 24, 2022
a1a38e2
minor fix
xingsy97 Oct 24, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build/dependencies.props
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<MicrosoftAspNetCoreHttpConnectionsClientPackage3_0Version>3.0.0</MicrosoftAspNetCoreHttpConnectionsClientPackage3_0Version>
<MicrosoftAspNetCoreHttpConnectionsClientPackage3_1Version>3.1.9</MicrosoftAspNetCoreHttpConnectionsClientPackage3_1Version>
<MicrosoftAspNetCoreHttpConnectionsClientPackage5_0Version>5.0.1</MicrosoftAspNetCoreHttpConnectionsClientPackage5_0Version>
<MicrosoftAspNetCoreHttpConnectionsClientPackage7_0Version>7.0.0-preview.7.22376.6</MicrosoftAspNetCoreHttpConnectionsClientPackage7_0Version>
<MicrosoftAspNetCoreHttpConnectionsClientPackage6_0Version>6.0.10</MicrosoftAspNetCoreHttpConnectionsClientPackage6_0Version>
<MicrosoftAspNetCoreHttpConnectionsCommonPackageVersion>1.0.4</MicrosoftAspNetCoreHttpConnectionsCommonPackageVersion>
<MicrosoftAspNetCoreHttpConnectionsCommonPackage3_0Version>3.0.0</MicrosoftAspNetCoreHttpConnectionsCommonPackage3_0Version>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@ internal interface ICallerClientResultsManager : IClientResultsManager
/// <typeparam name="T"></typeparam>
/// <param name="connectionId"></param>
/// <param name="invocationId"></param>
/// <param name="instanceId"> The InstanceId of target client the caller server knows when this method is called. If the target client is managed by the caller server, the caller server knows the InstanceId of target client and this parameter is not null. Otherwise, this parameter is null. </param>
/// <param name="cancellationToken"></param>
/// <returns></returns>
Task<T> AddInvocation<T>(string connectionId, string invocationId, string instanceId, CancellationToken cancellationToken);
Task<T> AddInvocation<T>(string connectionId, string invocationId, CancellationToken cancellationToken);

void AddServiceMapping(ServiceMappingMessage serviceMappingMessage);

Expand All @@ -29,5 +28,7 @@ internal interface ICallerClientResultsManager : IClientResultsManager
bool TryCompleteResult(string connectionId, ClientCompletionMessage message);

bool TryCompleteResult(string connectionId, ErrorCompletionMessage message);

bool RemoveInvocation(string invocationId);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ protected Task OnServiceErrorAsync(ServiceErrorMessage serviceErrorMessage)
return Task.CompletedTask;
}

protected Task OnPingMessageAsync(PingMessage pingMessage)
protected virtual Task OnPingMessageAsync(PingMessage pingMessage)
{
if (RuntimeServicePingMessage.TryGetOffline(pingMessage, out var instanceId))
{
Expand Down Expand Up @@ -550,7 +550,7 @@ private async Task ProcessIncomingAsync(ConnectionContext connection)
}
}

private Task DispatchMessageAsync(ServiceMessage message)
protected virtual Task DispatchMessageAsync(ServiceMessage message)
{
return message switch
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public string GenerateInvocationId(string connectionId)
return $"{connectionId}-{_clientResultManagerId}-{Interlocked.Increment(ref _lastInvocationId)}";
}

public Task<T> AddInvocation<T>(string connectionId, string invocationId, string instanceId, CancellationToken cancellationToken)
public Task<T> AddInvocation<T>(string connectionId, string invocationId, CancellationToken cancellationToken)
{
var tcs = new TaskCompletionSourceWithCancellation<T>(
cancellationToken,
Expand All @@ -53,7 +53,7 @@ public Task<T> AddInvocation<T>(string connectionId, string invocationId, string
{
tcs.TrySetException(new Exception(completionMessage.Error));
}
}) { RouterInstanceId = instanceId }
}) { RouterInstanceId = null }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}) { RouterInstanceId = null }
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

);
Debug.Assert(result);

Expand All @@ -70,14 +70,6 @@ public void AddServiceMapping(ServiceMappingMessage serviceMappingMessage)
{
Copy link
Contributor

@JialinXin JialinXin Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

invocation.RouterInstanceId = serviceMappingMessage.InstanceId;
}
else
{
// do nothing
}
}
else
{
// do nothing
}
}

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


// Unused, here to honor the IInvocationBinder interface but should never be called
public IReadOnlyList<Type> GetParameterTypes(string methodName) => throw new NotImplementedException();

Expand Down
5 changes: 5 additions & 0 deletions src/Microsoft.Azure.SignalR/DependencyInjectionExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,11 @@ private static ISignalRServerBuilder AddAzureSignalRCore(this ISignalRServerBuil
.AddSingleton<IClientConnectionFactory, ClientConnectionFactory>()
.AddSingleton<IHostedService, HeartBeat>()
.AddSingleton<IAccessKeySynchronizer, AccessKeySynchronizer>()
#if NET7_0_OR_GREATER
.AddSingleton<IClientInvocationManager, ClientInvocationManager>()
#else
.AddSingleton<IClientInvocationManager, DummyClientInvocationManager>()
#endif
.AddSingleton(typeof(NegotiateHandler<>));

// If a custom router is added, do not add the default router
Expand Down
6 changes: 5 additions & 1 deletion src/Microsoft.Azure.SignalR/HubHost/ServiceHubDispatcher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ internal class ServiceHubDispatcher<THub> where THub : Hub
private readonly IEndpointRouter _router;
private readonly string _hubName;
private readonly IServiceEventHandler _serviceEventHandler;
private readonly IClientInvocationManager _clientInvocationManager;

protected readonly IServerNameProvider _nameProvider;

Expand All @@ -45,6 +46,7 @@ public ServiceHubDispatcher(
IServerNameProvider nameProvider,
ServerLifetimeManager serverLifetimeManager,
IClientConnectionFactory clientConnectionFactory,
IClientInvocationManager clientInvocationManager,
IServiceEventHandler serviceEventHandler)
{
_serviceProtocol = serviceProtocol;
Expand All @@ -62,6 +64,7 @@ public ServiceHubDispatcher(
_nameProvider = nameProvider;
_hubName = typeof(THub).Name;
_serviceEventHandler = serviceEventHandler;
_clientInvocationManager = clientInvocationManager;

serverLifetimeManager?.Register(ShutdownAsync);
}
Expand Down Expand Up @@ -150,7 +153,8 @@ internal virtual ServiceConnectionFactory GetServiceConnectionFactory(
connectionDelegate,
_clientConnectionFactory,
_nameProvider,
_serviceEventHandler)
_serviceEventHandler,
_clientInvocationManager)
{
ConfigureContext = contextConfig,
ShutdownMode = _options.GracefulShutdown.Mode
Expand Down
102 changes: 97 additions & 5 deletions src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.SignalR;
using Microsoft.AspNetCore.SignalR.Protocol;
using Microsoft.Azure.SignalR.Common;
using Microsoft.Azure.SignalR.Protocol;
using Microsoft.Extensions.Logging;
Expand All @@ -18,8 +20,9 @@ internal class ServiceLifetimeManager<THub> : ServiceLifetimeManagerBase<THub> w
{
private const string MarkerNotConfiguredError =
"'AddAzureSignalR(...)' was called without a matching call to 'IApplicationBuilder.UseAzureSignalR(...)'.";

private readonly IClientInvocationManager _clientInvocationManager;
private readonly IClientConnectionManager _clientConnectionManager;
private readonly string _callerId;

public ServiceLifetimeManager(
IServiceConnectionManager<THub> serviceConnectionManager,
Expand All @@ -29,12 +32,15 @@ public ServiceLifetimeManager(
AzureSignalRMarkerService marker,
IOptions<HubOptions> globalHubOptions,
IOptions<HubOptions<THub>> hubOptions,
IBlazorDetector blazorDetector)
IBlazorDetector blazorDetector,
IServerNameProvider nameProvider,
IClientInvocationManager clientInvocationManager)
: base(
serviceConnectionManager,
protocolResolver,
globalHubOptions,
hubOptions, logger)
hubOptions,
logger)
{
// after core 3.0 UseAzureSignalR() is not required.
#if NETSTANDARD2_0
Expand All @@ -43,12 +49,19 @@ public ServiceLifetimeManager(
throw new InvalidOperationException(MarkerNotConfiguredError);
}
#endif
_clientConnectionManager = clientConnectionManager;

if (hubOptions.Value.SupportedProtocols != null && hubOptions.Value.SupportedProtocols.Any(x => x.Equals(Constants.Protocol.BlazorPack, StringComparison.OrdinalIgnoreCase)))
{
blazorDetector?.TrySetBlazor(typeof(THub).Name, true);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


_clientInvocationManager = clientInvocationManager ?? throw new ArgumentNullException(nameof(clientInvocationManager));
_clientConnectionManager = clientConnectionManager ?? throw new ArgumentNullException(nameof(clientConnectionManager));
}

public override Task OnConnectedAsync(HubConnectionContext connection)
Expand Down Expand Up @@ -103,6 +116,85 @@ public override async Task SendConnectionAsync(string connectionId, string metho
}
}

#if NET7_0_OR_GREATER
vicancy marked this conversation as resolved.
Show resolved Hide resolved
public override async Task<T> InvokeConnectionAsync<T>(string connectionId, string methodName, object[] args, CancellationToken cancellationToken = default)
{
if (IsInvalidArgument(connectionId))
{
throw new ArgumentNullException(nameof(connectionId));
}

if (IsInvalidArgument(methodName))
{
throw new ArgumentNullException(nameof(methodName));
}

var invocationId = _clientInvocationManager.Caller.GenerateInvocationId(connectionId);
var task = _clientInvocationManager.Caller.AddInvocation<T>(connectionId, invocationId, cancellationToken);

// Exception handling follows https://source.dot.net/#Microsoft.AspNetCore.SignalR.Core/DefaultHubLifetimeManager.cs,349
try
{
var message = AppendMessageTracingId(new ClientInvocationMessage(invocationId, connectionId, _callerId, SerializeAllProtocols(methodName, args, invocationId)));
await WriteAsync(message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can move to before L133

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved

return await task;
}
catch
{
_clientInvocationManager.Caller.RemoveInvocation(invocationId);
throw;
vicancy marked this conversation as resolved.
Show resolved Hide resolved
}
}

public override async Task SetConnectionResultAsync(string connectionId, CompletionMessage result)
{
if (IsInvalidArgument(connectionId))
{
throw new ArgumentException(NullOrEmptyStringErrorMessage, nameof(connectionId));
}
if (_clientConnectionManager.ClientConnections.TryGetValue(connectionId, out var clientConnectionContext))
{
// Determine which manager (Caller / Router) the `result` belongs to.
// `TryCompletionResult` returns false when the corresponding invocation is not existing.
IClientResultsManager clientResultsManager = null;
if (_clientInvocationManager.Caller.TryCompleteResult(connectionId, result))
{
clientResultsManager = _clientInvocationManager.Caller;
}
if (_clientInvocationManager.Router.TryCompleteResult(connectionId, result))
{
clientResultsManager = _clientInvocationManager.Router;
}

// Block unknown `results` which belongs to neither Caller nor Router
if (clientResultsManager != null)
{
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add some comments here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved

? SerializeCompletionMessage(result, protocol)
: Array.Empty<byte>();

var message = AppendMessageTracingId(new ClientCompletionMessage(result.InvocationId, connectionId, _callerId, protocol, payload));
await WriteAsync(message);
}
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this check. Also remove method Router.ContainsInvocation

{
return _clientInvocationManager.Router.TryGetInvocationReturnType(invocationId, out type);
}
else
{
return _clientInvocationManager.Caller.TryGetInvocationReturnType(invocationId, out type);
}
}
#endif

private MultiConnectionDataMessage CreateMessage(string connectionId, string methodName, object[] args, ClientConnectionContext serviceConnectionContext)
{
IDictionary<string, ReadOnlyMemory<byte>> payloads;
Expand Down
16 changes: 14 additions & 2 deletions src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManagerBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using System.Diagnostics.CodeAnalysis;
xingsy97 marked this conversation as resolved.
Show resolved Hide resolved
using Microsoft.AspNetCore.SignalR;
using Microsoft.AspNetCore.SignalR.Protocol;
using Microsoft.Azure.SignalR.Protocol;
Expand Down Expand Up @@ -283,10 +284,18 @@ protected static bool IsInvalidArgument(IReadOnlyList<object> list)
return list == null;
}

protected IDictionary<string, ReadOnlyMemory<byte>> SerializeAllProtocols(string method, object[] args)
protected IDictionary<string, ReadOnlyMemory<byte>> SerializeAllProtocols(string method, object[] args, string invocationId = null)
{
var payloads = new Dictionary<string, ReadOnlyMemory<byte>>();
var message = new InvocationMessage(method, args);
InvocationMessage message;
if (invocationId == null)
{
message = new InvocationMessage(method, args);
}
else
{
message = new InvocationMessage(invocationId, method, args);
}
var serializedHubMessages = _messageSerializer.SerializeMessage(message);
foreach (var serializedMessage in serializedHubMessages)
{
Expand All @@ -298,6 +307,9 @@ protected IDictionary<string, ReadOnlyMemory<byte>> SerializeAllProtocols(string
protected ReadOnlyMemory<byte> SerializeProtocol(string protocol, string method, object[] args) =>
_messageSerializer.SerializeMessage(protocol, new InvocationMessage(method, args));

protected ReadOnlyMemory<byte> SerializeCompletionMessage(CompletionMessage message, string protocol) =>
_messageSerializer.SerializeMessage(protocol, message);

protected virtual T AppendMessageTracingId<T>(T message) where T : ServiceMessage, IMessageWithTracingId
{
return message.WithTracingId();
Expand Down
Loading