Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Commit 18f770e

Browse files
moozzykmoozzyk
authored
Late parameter binding (#1049)
Late parameter binding Storing exception thrown during parameter binding and rethrowing when the method is about to throw. This allows completing invocations with a HubException and keeping the connection open. We will also no longer close the connection if parameters for client side methods cannot be bound. We will log and continue. Fixes: #818 (Also fixing #1005 because I was just touching this line)
1 parent eec6b4f commit 18f770e

File tree

15 files changed

+351
-128
lines changed

15 files changed

+351
-128
lines changed

‎src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
using Microsoft.AspNetCore.Sockets.Internal;
2020
using Microsoft.Extensions.Logging;
2121
using Microsoft.Extensions.Logging.Abstractions;
22-
using Newtonsoft.Json;
2322

2423
namespace Microsoft.AspNetCore.SignalR.Client
2524
{
@@ -122,7 +121,7 @@ private async Task DisposeAsyncCore()
122121
public IDisposable On(string methodName, Type[] parameterTypes, Func<object[], object, Task> handler, object state)
123122
{
124123
var invocationHandler = new InvocationHandler(parameterTypes, handler, state);
125-
var invocationList = _handlers.AddOrUpdate(methodName, _ => new List<InvocationHandler> { invocationHandler },
124+
var invocationList = _handlers.AddOrUpdate(methodName, _ => new List<InvocationHandler> { invocationHandler },
126125
(_, invocations) =>
127126
{
128127
lock (invocations)
@@ -135,7 +134,7 @@ public IDisposable On(string methodName, Type[] parameterTypes, Func<object[], o
135134
return new Subscription(invocationHandler, invocationList);
136135
}
137136

138-
public async Task<ReadableChannel<object>> StreamAsync(string methodName, Type returnType, object[] args, CancellationToken cancellationToken = default(CancellationToken))
137+
public async Task<ReadableChannel<object>> StreamAsync(string methodName, Type returnType, object[] args, CancellationToken cancellationToken = default)
139138
{
140139
return await StreamAsyncCore(methodName, returnType, args, cancellationToken).ForceAsync();
141140
}
@@ -174,7 +173,7 @@ private async Task<ReadableChannel<object>> StreamAsyncCore(string methodName, T
174173
return channel;
175174
}
176175

177-
public async Task<object> InvokeAsync(string methodName, Type returnType, object[] args, CancellationToken cancellationToken = default(CancellationToken)) =>
176+
public async Task<object> InvokeAsync(string methodName, Type returnType, object[] args, CancellationToken cancellationToken = default) =>
178177
await InvokeAsyncCore(methodName, returnType, args, cancellationToken).ForceAsync();
179178

180179
private async Task<object> InvokeAsyncCore(string methodName, Type returnType, object[] args, CancellationToken cancellationToken)
@@ -184,7 +183,7 @@ private async Task<object> InvokeAsyncCore(string methodName, Type returnType, o
184183
return await task;
185184
}
186185

187-
public async Task SendAsync(string methodName, object[] args, CancellationToken cancellationToken = default(CancellationToken)) =>
186+
public async Task SendAsync(string methodName, object[] args, CancellationToken cancellationToken = default) =>
188187
await SendAsyncCore(methodName, args, cancellationToken).ForceAsync();
189188

190189
private Task SendAsyncCore(string methodName, object[] args, CancellationToken cancellationToken)
@@ -206,7 +205,8 @@ private Task InvokeCore(string methodName, InvocationRequest irq, object[] args,
206205
}
207206

208207
// Create an invocation descriptor. Client invocations are always blocking
209-
var invocationMessage = new InvocationMessage(irq.InvocationId, nonBlocking, methodName, args);
208+
var invocationMessage = new InvocationMessage(irq.InvocationId, nonBlocking, methodName,
209+
argumentBindingException: null, arguments: args);
210210

211211
// We don't need to track invocations for fire an forget calls
212212
if (!nonBlocking)
@@ -252,7 +252,8 @@ private async Task OnDataReceivedAsync(byte[] data)
252252
switch (message)
253253
{
254254
case InvocationMessage invocation:
255-
_logger.ReceivedInvocation(invocation.InvocationId, invocation.Target, invocation.Arguments);
255+
_logger.ReceivedInvocation(invocation.InvocationId, invocation.Target,
256+
invocation.ArgumentBindingException != null ? null : invocation.Arguments);
256257
await DispatchInvocationAsync(invocation, _connectionActive.Token);
257258
break;
258259
case CompletionMessage completion:
@@ -344,7 +345,7 @@ private async Task DispatchInvocationAsync(InvocationMessage invocation, Cancell
344345
}
345346
catch (Exception ex)
346347
{
347-
_logger.ExceptionThrownFromCallback(nameof(On), ex);
348+
_logger.ErrorInvokingClientSideMethod(invocation.Target, ex);
348349
}
349350
}
350351
}

‎src/Microsoft.AspNetCore.SignalR.Client.Core/Internal/SignalRClientLoggerExtensions.cs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -112,8 +112,8 @@ internal static class SignalRClientLoggerExtensions
112112
private static readonly Action<ILogger, string, Exception> _streamItemOnNonStreamInvocation =
113113
LoggerMessage.Define<string>(LogLevel.Error, new EventId(4, nameof(StreamItemOnNonStreamInvocation)), "Invocation {invocationId} received stream item but was invoked as a non-streamed invocation.");
114114

115-
private static readonly Action<ILogger, string, Exception> _exceptionThrownFromCallback =
116-
LoggerMessage.Define<string>(LogLevel.Error, new EventId(5, nameof(ExceptionThrownFromCallback)), "An exception was thrown from the '{callback}' callback");
115+
private static readonly Action<ILogger, string, Exception> _errorInvokingClientSideMethod =
116+
LoggerMessage.Define<string>(LogLevel.Error, new EventId(5, nameof(ErrorInvokingClientSideMethod)), "Invoking client side method '{methodName}' failed.");
117117

118118
private static readonly Action<ILogger, string, string, Exception> _receivedUnexpectedMessageTypeForInvokeCompletion =
119119
LoggerMessage.Define<string, string>(LogLevel.Error, new EventId(6, nameof(ReceivedUnexpectedMessageTypeForInvokeCompletion)), "Invocation {invocationId} was invoked as a non-streaming hub method but completed with '{messageType}' message.");
@@ -137,7 +137,7 @@ public static void IssueInvocation(this ILogger logger, string invocationId, str
137137
{
138138
if (logger.IsEnabled(LogLevel.Trace))
139139
{
140-
var argsList = string.Join(", ", args.Select(a => a.GetType().FullName));
140+
var argsList = args == null ? string.Empty : string.Join(", ", args.Select(a => a?.GetType().FullName ?? "(null)"));
141141
_issueInvocation(logger, invocationId, returnType, methodName, argsList, null);
142142
}
143143
}
@@ -161,7 +161,7 @@ public static void ReceivedInvocation(this ILogger logger, string invocationId,
161161
{
162162
if (logger.IsEnabled(LogLevel.Trace))
163163
{
164-
var argsList = string.Join(", ", args.Select(a => a.GetType().FullName));
164+
var argsList = args == null ? string.Empty : string.Join(", ", args.Select(a => a?.GetType().FullName ?? "(null)"));
165165
_receivedInvocation(logger, invocationId, methodName, argsList, null);
166166
}
167167
}
@@ -286,9 +286,9 @@ public static void StreamItemOnNonStreamInvocation(this ILogger logger, string i
286286
_streamItemOnNonStreamInvocation(logger, invocationId, null);
287287
}
288288

289-
public static void ExceptionThrownFromCallback(this ILogger logger, string callbackName, Exception exception)
289+
public static void ErrorInvokingClientSideMethod(this ILogger logger, string methodName, Exception exception)
290290
{
291-
_exceptionThrownFromCallback(logger, callbackName, exception);
291+
_errorInvokingClientSideMethod(logger, methodName, exception);
292292
}
293293

294294
public static void ReceivedUnexpectedMessageTypeForStreamCompletion(this ILogger logger, string invocationId, string messageType)

‎src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/InvocationMessage.cs

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,42 @@
33

44
using System;
55
using System.Linq;
6+
using System.Runtime.ExceptionServices;
67

78
namespace Microsoft.AspNetCore.SignalR.Internal.Protocol
89
{
910
public class InvocationMessage : HubMessage
1011
{
12+
private readonly ExceptionDispatchInfo _argumentBindingException;
13+
private readonly object[] _arguments;
14+
1115
public string Target { get; }
1216

13-
public object[] Arguments { get; }
17+
public object[] Arguments
18+
{
19+
get
20+
{
21+
if (_argumentBindingException != null)
22+
{
23+
_argumentBindingException.Throw();
24+
}
25+
26+
return _arguments;
27+
}
28+
}
29+
30+
public Exception ArgumentBindingException
31+
{
32+
get
33+
{
34+
return _argumentBindingException?.SourceException;
35+
}
36+
}
1437

1538
public bool NonBlocking { get; }
1639

17-
public InvocationMessage(string invocationId, bool nonBlocking, string target, params object[] arguments) : base(invocationId)
40+
public InvocationMessage(string invocationId, bool nonBlocking, string target, ExceptionDispatchInfo argumentBindingException, params object[] arguments)
41+
: base(invocationId)
1842
{
1943
if (string.IsNullOrEmpty(invocationId))
2044
{
@@ -26,13 +50,14 @@ public InvocationMessage(string invocationId, bool nonBlocking, string target, p
2650
throw new ArgumentNullException(nameof(target));
2751
}
2852

29-
if (arguments == null)
53+
if ((arguments == null && argumentBindingException == null) || (arguments?.Length > 0 && argumentBindingException != null))
3054
{
31-
throw new ArgumentNullException(nameof(arguments));
55+
throw new ArgumentException($"'{nameof(argumentBindingException)}' and '{nameof(arguments)}' are mutually exclusive");
3256
}
3357

3458
Target = target;
35-
Arguments = arguments;
59+
_arguments = arguments;
60+
_argumentBindingException = argumentBindingException;
3661
NonBlocking = nonBlocking;
3762
}
3863

‎src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/JsonHubProtocol.cs

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33

44
using System;
55
using System.Collections.Generic;
6-
using System.Diagnostics;
76
using System.IO;
7+
using System.Runtime.ExceptionServices;
88
using Microsoft.AspNetCore.SignalR.Internal.Formatters;
99
using Newtonsoft.Json;
1010
using Newtonsoft.Json.Linq;
@@ -242,22 +242,40 @@ private InvocationMessage BindInvocationMessage(JObject json, IInvocationBinder
242242
var args = JsonUtils.GetRequiredProperty<JArray>(json, ArgumentsPropertyName, JTokenType.Array);
243243

244244
var paramTypes = binder.GetParameterTypes(target);
245+
246+
try
247+
{
248+
var arguments = BindArguments(args, paramTypes);
249+
return new InvocationMessage(invocationId, nonBlocking, target, argumentBindingException: null, arguments: arguments);
250+
}
251+
catch (Exception ex)
252+
{
253+
return new InvocationMessage(invocationId, nonBlocking, target, ExceptionDispatchInfo.Capture(ex));
254+
}
255+
}
256+
257+
private object[] BindArguments(JArray args, Type[] paramTypes)
258+
{
245259
var arguments = new object[args.Count];
246260
if (paramTypes.Length != arguments.Length)
247261
{
248262
throw new FormatException($"Invocation provides {arguments.Length} argument(s) but target expects {paramTypes.Length}.");
249263
}
250264

251-
for (var i = 0; i < paramTypes.Length; i++)
265+
try
252266
{
253-
var paramType = paramTypes[i];
267+
for (var i = 0; i < paramTypes.Length; i++)
268+
{
269+
var paramType = paramTypes[i];
270+
arguments[i] = args[i].ToObject(paramType, _payloadSerializer);
271+
}
254272

255-
// TODO(anurse): We can add some DI magic here to allow users to provide their own serialization
256-
// Related Bug: https://github.com/aspnet/SignalR/issues/261
257-
arguments[i] = args[i].ToObject(paramType, _payloadSerializer);
273+
return arguments;
274+
}
275+
catch (Exception ex)
276+
{
277+
throw new FormatException("Error binding arguments. Make sure that the types of the provided values match the types of the hub method being invoked.", ex);
258278
}
259-
260-
return new InvocationMessage(invocationId, nonBlocking, target, arguments);
261279
}
262280

263281
private StreamItemMessage BindResultMessage(JObject json, IInvocationBinder binder)

‎src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/MessagePackHubProtocol.cs

Lines changed: 48 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.Collections.Generic;
66
using System.Diagnostics;
77
using System.IO;
8+
using System.Runtime.ExceptionServices;
89
using Microsoft.AspNetCore.SignalR.Internal.Formatters;
910
using MsgPack;
1011
using MsgPack.Serialization;
@@ -55,24 +56,26 @@ public bool TryParseMessages(ReadOnlyBuffer<byte> input, IInvocationBinder binde
5556

5657
private static HubMessage ParseMessage(Stream input, IInvocationBinder binder)
5758
{
58-
var unpacker = Unpacker.Create(input);
59-
var arraySize = ReadArrayLength(unpacker, "elementCount");
60-
var messageType = ReadInt32(unpacker, "messageType");
61-
62-
switch (messageType)
59+
using (var unpacker = Unpacker.Create(input))
6360
{
64-
case InvocationMessageType:
65-
return CreateInvocationMessage(unpacker, binder);
66-
case StreamItemMessageType:
67-
return CreateStreamItemMessage(unpacker, binder);
68-
case CompletionMessageType:
69-
return CreateCompletionMessage(unpacker, binder);
70-
case StreamCompletionMessageType:
71-
return CreateStreamCompletionMessage(unpacker, arraySize, binder);
72-
case CancelInvocationMessageType:
73-
return CreateCancelInvocationMessage(unpacker);
74-
default:
75-
throw new FormatException($"Invalid message type: {messageType}.");
61+
var arraySize = ReadArrayLength(unpacker, "elementCount");
62+
var messageType = ReadInt32(unpacker, "messageType");
63+
64+
switch (messageType)
65+
{
66+
case InvocationMessageType:
67+
return CreateInvocationMessage(unpacker, binder);
68+
case StreamItemMessageType:
69+
return CreateStreamItemMessage(unpacker, binder);
70+
case CompletionMessageType:
71+
return CreateCompletionMessage(unpacker, binder);
72+
case StreamCompletionMessageType:
73+
return CreateStreamCompletionMessage(unpacker, arraySize, binder);
74+
case CancelInvocationMessageType:
75+
return CreateCancelInvocationMessage(unpacker);
76+
default:
77+
throw new FormatException($"Invalid message type: {messageType}.");
78+
}
7679
}
7780
}
7881

@@ -81,22 +84,43 @@ private static InvocationMessage CreateInvocationMessage(Unpacker unpacker, IInv
8184
var invocationId = ReadInvocationId(unpacker);
8285
var nonBlocking = ReadBoolean(unpacker, "nonBlocking");
8386
var target = ReadString(unpacker, "target");
84-
var argumentCount = ReadArrayLength(unpacker, "arguments");
8587
var parameterTypes = binder.GetParameterTypes(target);
8688

89+
try
90+
{
91+
var arguments = BindArguments(unpacker, parameterTypes);
92+
return new InvocationMessage(invocationId, nonBlocking, target, argumentBindingException: null, arguments: arguments);
93+
}
94+
catch (Exception ex)
95+
{
96+
return new InvocationMessage(invocationId, nonBlocking, target, ExceptionDispatchInfo.Capture(ex));
97+
}
98+
}
99+
100+
private static object[] BindArguments(Unpacker unpacker, Type[] parameterTypes)
101+
{
102+
var argumentCount = ReadArrayLength(unpacker, "arguments");
103+
87104
if (parameterTypes.Length != argumentCount)
88105
{
89106
throw new FormatException(
90-
$"Target method expects {parameterTypes.Length} arguments(s) but invocation has {argumentCount} argument(s).");
107+
$"Invocation provides {argumentCount} argument(s) but target expects {parameterTypes.Length}.");
91108
}
92109

93-
var arguments = new object[argumentCount];
94-
for (var i = 0; i < argumentCount; i++)
110+
try
95111
{
96-
arguments[i] = DeserializeObject(unpacker, parameterTypes[i], "argument");
97-
}
112+
var arguments = new object[argumentCount];
113+
for (var i = 0; i < argumentCount; i++)
114+
{
115+
arguments[i] = DeserializeObject(unpacker, parameterTypes[i], "argument");
116+
}
98117

99-
return new InvocationMessage(invocationId, nonBlocking, target, arguments);
118+
return arguments;
119+
}
120+
catch (Exception ex)
121+
{
122+
throw new FormatException("Error binding arguments. Make sure that the types of the provided values match the types of the hub method being invoked.", ex);
123+
}
100124
}
101125

102126
private static StreamItemMessage CreateStreamItemMessage(Unpacker unpacker, IInvocationBinder binder)

‎src/Microsoft.AspNetCore.SignalR.Core/DefaultHubLifetimeManager.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ public override Task InvokeGroupAsync(string groupName, string methodName, objec
125125

126126
private InvocationMessage CreateInvocationMessage(string methodName, object[] args)
127127
{
128-
return new InvocationMessage(GetInvocationId(), nonBlocking: true, target: methodName, arguments: args);
128+
return new InvocationMessage(GetInvocationId(), nonBlocking: true, target: methodName, argumentBindingException: null, arguments: args);
129129
}
130130

131131
public override Task InvokeUserAsync(string userId, string methodName, object[] args)

0 commit comments

Comments
 (0)