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

Commit 72d1f7a

Browse files
author
Pawel Kadluczka
committed
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 30e7422 commit 72d1f7a

File tree

15 files changed

+266
-129
lines changed

15 files changed

+266
-129
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 = 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 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 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)