Late parameter binding #1049

moozzyk · 2017-10-24T00:39:37Z

Soliciting early feedback.

moozzyk · 2017-10-24T00:40:01Z

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

+        {
+            get
+            {
+                if (_bindingError != null)


How bad is this?

davidfowl · 2017-10-24T04:15:24Z

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

@@ -8,13 +8,29 @@ namespace Microsoft.AspNetCore.SignalR.Internal.Protocol
 {
    public class InvocationMessage : HubMessage
    {
+        // Use ExceptionDispatchInfo ?
+        private readonly Exception _bindingError;


Yes use ExceptionDispatchInfo

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)

moozzyk · 2017-10-25T00:17:46Z

Ready for review.

BrennanConroy · 2017-10-25T16:20:49Z

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

@@ -137,7 +137,7 @@ public static void IssueInvocation(this ILogger logger, string invocationId, str
        {
            if (logger.IsEnabled(LogLevel.Trace))
            {
-                var argsList = string.Join(", ", args.Select(a => a.GetType().FullName));
+                var argsList = string.Join(", ", args.Select(a => a?.GetType().FullName?? "(null)"));


space after FullName

Add null check for args

Didn't do because it never happens but I think it won't hurt either.

BrennanConroy · 2017-10-25T16:25:00Z

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


-                // TODO(anurse): We can add some DI magic here to allow users to provide their own serialization
-                // Related Bug: https://github.com/aspnet/SignalR/issues/261


Do we want to keep this comment in?

We allow setting serialization settings in HubOptions so this is covered (#261 is closed).

BrennanConroy · 2017-10-25T16:26:11Z

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

+            }
+            catch (Exception ex)
+            {
+                throw new FormatException("Error binding arguments. Make sure that types of the provided values match the types of the hub method being invoked.", ex);


"that the types"

BrennanConroy · 2017-10-25T19:03:47Z

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

            if (parameterTypes.Length != argumentCount)
            {
                throw new FormatException(
-                    $"Target method expects {parameterTypes.Length} arguments(s) but invocation has {argumentCount} argument(s).");
+                    $"Invocation provides {argumentCount} argument(s) but target expects {parameterTypes.Length}.");


Do we log on the server for any binding failures?

Yes - it now fails when we are invoking the method so it is treated as any other hub method invocation error

BrennanConroy · 2017-10-25T19:04:30Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

@@ -556,7 +556,7 @@ public class RedisExcludeClientsMessage : InvocationMessage
            public IReadOnlyList<string> ExcludedIds;

            public RedisExcludeClientsMessage(string invocationId, bool nonBlocking, string target, IReadOnlyList<string> excludedIds, params object[] arguments)
-                : base(invocationId, nonBlocking, target, arguments)
+                : base(invocationId, nonBlocking, target, null, arguments)


Wouldn't hurt to use named params here

BrennanConroy · 2017-10-25T19:04:45Z

test/Common/TestClient.cs

@@ -136,7 +136,7 @@ public Task<string> SendInvocationAsync(string methodName, params object[] args)
        public Task<string> SendInvocationAsync(string methodName, bool nonBlocking, params object[] args)
        {
            var invocationId = GetInvocationId();
-            return SendHubMessageAsync(new InvocationMessage(invocationId, nonBlocking, methodName, args));
+            return SendHubMessageAsync(new InvocationMessage(invocationId, nonBlocking, methodName, null, args));


named param

dnfclas added the cla-already-signed label Oct 24, 2017

moozzyk commented Oct 24, 2017

View reviewed changes

davidfowl reviewed Oct 24, 2017

View reviewed changes

moozzyk force-pushed the pawelka/lateparambinding branch from 327d672 to 72d1f7a Compare October 25, 2017 00:06

moozzyk force-pushed the pawelka/lateparambinding branch from 72d1f7a to 2e0d99f Compare October 25, 2017 00:17

moozzyk changed the title ~~Late parameter binding - Prototype~~ Late parameter binding Oct 25, 2017

BrennanConroy reviewed Oct 25, 2017

View reviewed changes

PR feedback

62577b0

BrennanConroy approved these changes Oct 26, 2017

View reviewed changes

moozzyk merged commit 18f770e into dev Oct 30, 2017

moozzyk deleted the pawelka/lateparambinding branch November 14, 2017 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Late parameter binding #1049

Late parameter binding #1049

moozzyk commented Oct 24, 2017

moozzyk Oct 24, 2017

davidfowl Oct 24, 2017

moozzyk commented Oct 25, 2017

BrennanConroy Oct 25, 2017

BrennanConroy Oct 25, 2017

moozzyk Oct 25, 2017

BrennanConroy Oct 25, 2017

moozzyk Oct 25, 2017

BrennanConroy Oct 25, 2017

BrennanConroy Oct 25, 2017

moozzyk Oct 25, 2017

BrennanConroy Oct 25, 2017

BrennanConroy Oct 25, 2017


		// TODO(anurse): We can add some DI magic here to allow users to provide their own serialization
		// Related Bug: https://github.com/aspnet/SignalR/issues/261

Late parameter binding #1049

Late parameter binding #1049

Conversation

moozzyk commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moozzyk commented Oct 25, 2017

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