Run hub invocations serially #2086

BrennanConroy · 2018-04-18T23:54:47Z

#2084

muratg · 2018-04-19T00:13:56Z

src/Microsoft.AspNetCore.SignalR.Core/HubConnectionHandler.cs

@@ -184,7 +184,7 @@ public class HubConnectionHandler<THub> : ConnectionHandler where THub : Hub
                            {
                                // Don't wait on the result of execution, continue processing other
                                // incoming messages on this connection.


Not valid, remove.

JamesNK · 2018-04-19T00:35:49Z

src/Microsoft.AspNetCore.SignalR.Core/HubConnectionHandler.cs

@@ -184,7 +184,7 @@ public class HubConnectionHandler<THub> : ConnectionHandler where THub : Hub
                            {
                                // Don't wait on the result of execution, continue processing other
                                // incoming messages on this connection.
-                                _ = _dispatcher.DispatchMessageAsync(connection, message);
+                                await _dispatcher.DispatchMessageAsync(connection, message);


Does this mean only one message can be invoked at a time for a connection? What happens if a hub method takes 5 seconds to complete because it is calling a slow DB? Do all other messages wait for it to finish?

Scenario: I am connected to jabbr Core. I send David a private message containing a link to github. On the server jabbr parses the message and sees the github link, makes a request to github to include more details.

At the same time I am sending messages in another chat room. Will those other messages only be invoked and sent out to other users in the room once the private message has finished being processed?

It does, but I believe this is also how ASP.NET SignalR works. This helps propagate back-pressure on the server back up through the client. If the server is backed up waiting to send data (in response to received messages), it slows down processing received messages, which eventually slows down the client, etc...

If you want to invoke a long-running thing in the background, you can fire and forget a task in the body of your Hub method and use IHubContext<T> to send a response to the Client via a Client invocation when the long-running task is complete.

Ok, as long as everyone who knows more about SignalR understands and expects that behavior. It doesn't seem like something can can easily be undone. If V1 runs messages serially then V2 won't be able to change to run in parallel without breaking some hearts.

I think it should be added as a comment here, and also spelled out in documentation. Devs are use to firing 10 requests at MVC and having them run in parallel.

Although I guess if there is load balancing and multiple instances of the SignalR server then messages could still end up running in parallel so people shouldn't rely on that logic.

I agree that we should make sure we understand the behavior. @davidfowl can you confirm how ASP.NET SignalR did dispatching? Last we talked I think you said it did it serially, like this.

Yes this is how old SignalR worked when you used websockets. Regardless this is a better way to handle dispatching. Running multiple calls to the hub in parallel means there's no self throttling and another rate limiting mechanism would have to be introduced to make this reasonable.

JamesNK · 2018-04-19T07:35:02Z

I'd like a couple of unit tests

Sequentially dispatching invoke messages
Multiple streams returning results and not blocking other messages being dispatched

davidfowl · 2018-04-19T17:34:07Z

test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs

+            var connectionHandler = serviceProvider.GetService<HubConnectionHandler<BlockingHub>>();
+
+            // Because we use PipeScheduler.Inline the hub invocations will run inline until they wait, which happens inside the BlockingMethod call
+            using (var client = new TestClient(new JsonHubProtocol()))


Isn't JsonHubProtocol the default?

davidfowl · 2018-04-19T17:35:43Z

test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTestUtils/Hubs.cs

@@ -491,6 +493,34 @@ public class SimpleTypedHub : Hub<ITypedHubClient>
        }
    }

+    public class BlockingHub : Hub
+    {
+        private TcsService _tcs;


_tcsService not tcs

davidfowl · 2018-04-19T17:35:59Z

test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTestUtils/Hubs.cs

+            _tcs = tcs;
+        }
+
+        public async Task<int> BlockingMethod()


This isn't blocking...

davidfowl · 2018-04-19T18:09:41Z

test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs

+                await tcsService.StartedMethod.Task.OrTimeout();
+
+                // Invoke another hub method which will wait for the first method to finish
+                var nonBlockingTask = client.SendInvocationAsync(nameof(LongRunningHub.SimpleMethod), nonBlocking: false);


Assert.False(nonBlockingTask.IsCompleted)

davidfowl

Add the assert

muratg

shiproom approved 👍

JamesNK · 2018-04-19T20:14:54Z

src/Microsoft.AspNetCore.SignalR.Core/HubConnectionHandler.cs

-                                // Don't wait on the result of execution, continue processing other
-                                // incoming messages on this connection.
-                                _ = _dispatcher.DispatchMessageAsync(connection, message);
+                                await _dispatcher.DispatchMessageAsync(connection, message);


Add a comment like:
Messages are dispatched sequentially and will block others until complete (except streaming invocations). Note that developers can't depend on messages running one after another after scaling out

JamesNK · 2018-04-19T20:20:01Z

src/Microsoft.AspNetCore.SignalR.Core/Internal/DefaultHubDispatcher.cs

@@ -88,7 +88,8 @@ public partial class DefaultHubDispatcher<THub> : HubDispatcher<THub> where THub

                case StreamInvocationMessage streamInvocationMessage:
                    Log.ReceivedStreamHubInvocation(_logger, streamInvocationMessage);
-                    await ProcessInvocation(connection, streamInvocationMessage, isStreamedInvocation: true);
+                    // Fire-and-forget stream invocations, otherwise they would block other hub invocations from being able to run
+                    _ = ProcessInvocation(connection, streamInvocationMessage, isStreamedInvocation: true);


Do we truly want to fire and forget the entire streaming invocation like this? I wonder if streaming invocations should run sequentially like other messages until the IAsyncEnumerator<T> is returned from the method, and it is only iterating over the IAsyncEnumerator<T> that is fire and forget. That would make them more consistent with other invocations.

Using the current logic in this PR I think it would still be possible to wreak havoc doing something like this:

public class BroadcastHub : Hub { public async Task<ChannelReader<int>> Broadcast(string message) { await Clients.All.SendAsync(message); var channel = Channel.CreateUnbounded<int>(); channel.Writer.Complete(); return channel; } }

I mostly agree with @JamesNK here. Though to be fair, this isn't a full defense against bad code, there's still nothing stopping a poorly-written Hub method from Task.Runing stuff. Still, if we await invocation of the method and then only fire-and-forget the logic to drain the stream, it would fit user expectations more I think.

BrennanConroy · 2018-04-19T23:21:45Z

Re-review

analogrelay · 2018-04-20T02:22:26Z

src/Microsoft.AspNetCore.SignalR.Core/Internal/DefaultHubDispatcher.cs

+            }
+            finally
+            {
+                if (disposeScope)


You forgot to rename this but meh...

Run hub invocations serially

86fe332

davidfowl added the SHP: Approval pending label Apr 19, 2018

muratg reviewed Apr 19, 2018

View reviewed changes

JamesNK reviewed Apr 19, 2018

View reviewed changes

tests

99d3468

davidfowl reviewed Apr 19, 2018

View reviewed changes

naming

186a445

davidfowl reviewed Apr 19, 2018

View reviewed changes

davidfowl approved these changes Apr 19, 2018

View reviewed changes

test

a746c66

BrennanConroy added SHP: Approved and removed SHP: Approval pending labels Apr 19, 2018

muratg approved these changes Apr 19, 2018

View reviewed changes

JamesNK reviewed Apr 19, 2018

View reviewed changes

BrennanConroy added 2 commits April 19, 2018 16:15

whew

1e71429

comment

55ee2f0

BrennanConroy merged commit 1957655 into release/2.1 Apr 19, 2018

BrennanConroy deleted the brecon/serialHub branch April 19, 2018 23:59

analogrelay reviewed Apr 20, 2018

View reviewed changes

BrennanConroy mentioned this pull request Jan 3, 2019

Non-serialized hub invocations dotnet/aspnetcore#5351

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run hub invocations serially #2086

Run hub invocations serially #2086

BrennanConroy commented Apr 18, 2018

muratg Apr 19, 2018

JamesNK Apr 19, 2018

JamesNK Apr 19, 2018 •

edited

analogrelay Apr 19, 2018

JamesNK Apr 19, 2018

JamesNK Apr 19, 2018

analogrelay Apr 19, 2018

davidfowl Apr 19, 2018 •

edited

JamesNK Apr 19, 2018

JamesNK commented Apr 19, 2018

davidfowl Apr 19, 2018

davidfowl Apr 19, 2018

davidfowl Apr 19, 2018

davidfowl Apr 19, 2018

davidfowl left a comment

muratg left a comment

JamesNK Apr 19, 2018

JamesNK Apr 19, 2018 •

edited

analogrelay Apr 19, 2018

BrennanConroy commented Apr 19, 2018

analogrelay Apr 20, 2018

BrennanConroy Apr 20, 2018

Run hub invocations serially #2086

Run hub invocations serially #2086

Conversation

BrennanConroy commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK Apr 19, 2018 • edited

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

davidfowl Apr 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK commented Apr 19, 2018

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

davidfowl left a comment

Choose a reason for hiding this comment

muratg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK Apr 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrennanConroy commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK Apr 19, 2018 •

edited

davidfowl Apr 19, 2018 •

edited

JamesNK Apr 19, 2018 •

edited