Refactor streaming from client to server #4559

BrennanConroy · 2018-12-10T22:43:04Z

Fixes https://github.com/aspnet/SignalR/issues/2592
Fixes #4401

TODO:

Couple more tests
Update spec with new behavior

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

src/SignalR/src/Microsoft.AspNetCore.SignalR.Common/Protocol/HubMethodInvocationMessage.cs

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

JamesNK · 2018-12-11T04:22:00Z

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

                                if (descriptor.OriginalParameterTypes[parameterPointer] == typeof(CancellationToken))
                                {
                                    cts = CancellationTokenSource.CreateLinkedTokenSource(connection.ConnectionAborted);
                                    arguments[parameterPointer] = cts.Token;
                                }
+                                else if (ReflectionHelper.IsStreamingType(descriptor.OriginalParameterTypes[parameterPointer], mustBeDirectType: true))


The reflection here and line 242 is happening for every stream item right? Should cache it on the descriptor to pay the price once.

davidfowl · 2018-12-11T05:21:13Z

I think we should also support accepting a channel writer for sever to client streaming. This is something we discussed briefly but didn’t go into details on. It’s worth looking at as it could simplify the programming model.

halter73 · 2018-12-11T19:51:02Z

src/SignalR/clients/ts/signalr/src/IHubProtocol.ts

-    CloseMessage |
-    StreamCompleteMessage |
-    StreamDataMessage;
+    CloseMessage;


Should InvocationMessage and StreamInvocationMessage also be combined?

No, those are how we distinguish between regular invocation and invocation that returns a stream to the client.

Couldn't that be determined based on the signature of the hub method being invoked? Or the presence of the stream/streamIds property in the InvocationMessage?

Current design of the streamIds property is that it is only for streams from client to server.

And we can't rely on the signature of the Hub method to determine if the client intended to wait for a normal invocation response (via CompletionMessage) or a streamed response (via StreaItems + CompletionMessage). So we use two different invocation messages, StreamInvocationMessage and InvocationMessage

src/SignalR/src/Microsoft.AspNetCore.SignalR.Common/Protocol/HubMethodInvocationMessage.cs

mikaelm12

Still reviewing. I think some of my comments are already out of date with the commit you just made seconds ago

mikaelm12 · 2018-12-10T22:47:57Z

src/SignalR/clients/ts/signalr/src/IHubProtocol.ts

@@ -155,16 +141,6 @@ export interface CancelInvocationMessage extends HubInvocationMessage {
    readonly invocationId: string;
 }

-/** A hub message sent to indicate the end of stream items for a streaming parameter. */
-export interface StreamCompleteMessage extends HubMessageBase {


mikaelm12 · 2018-12-10T22:47:59Z

src/SignalR/clients/ts/signalr/src/IHubProtocol.ts

@@ -97,18 +95,6 @@ export interface StreamItemMessage extends HubInvocationMessage {
    readonly item?: any;
 }

-/** A hub message representing a single stream item, transferred through a streaming parameter. */
-export interface StreamDataMessage extends HubMessageBase {


src/SignalR/src/Microsoft.AspNetCore.SignalR.Common/Protocol/HubMethodInvocationMessage.cs

.../Microsoft.AspNetCore.SignalR.Protocols.NewtonsoftJson/Protocol/NewtonsoftJsonHubProtocol.cs

JamesNK · 2018-12-13T22:48:57Z

src/SignalR/src/Microsoft.AspNetCore.SignalR.Common/Protocol/HubMethodInvocationMessage.cs

@@ -112,7 +112,7 @@ public override string ToString()

            try
            {
-                streamIds = string.Join(", ", StreamIds?.Select(id => id?.ToString()));
+                streamIds = string.Join(", ", StreamIds != null ? StreamIds.Select(id => id?.ToString()) : Array.Empty<string>());


Should an error be thrown for a null streamId?

This place might not be the right place but null stream ids off the wire are something that should be guarded against.

I don't think we should care if stream ids is null, it's similar to how we don't care about extra data in the protocol

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

src/SignalR/clients/ts/signalr-protocol-msgpack/src/MessagePackHubProtocol.ts

...lR/src/Microsoft.AspNetCore.SignalR.Protocols.MessagePack/Protocol/MessagePackHubProtocol.cs

.../Microsoft.AspNetCore.SignalR.Protocols.NewtonsoftJson/Protocol/NewtonsoftJsonHubProtocol.cs

src/SignalR/test/Microsoft.AspNetCore.SignalR.Tests.Utils/TestClient.cs

JamesNK

...icrosoft.AspNetCore.SignalR.Common.Tests/Internal/Protocol/TestHubMessageEqualityComparer.cs

BrennanConroy · 2019-01-02T18:29:25Z

Ping, anymore code comments while I finish up the spec?

mikaelm12

Some comments on the spec. Let's get this in

src/SignalR/specs/HubProtocol.md

BrennanConroy · 2019-01-05T00:12:24Z

Should we put the streaming from client feature behind a check for the MinorVersion from the server? That way we can fail with a nice clean error on the client if a newer client tries to stream to an older server.

halter73 · 2019-01-05T00:24:48Z

Should we put the streaming from client feature behind a check for the MinorVersion from the server? That way we can fail with a nice clean error on the client if a newer client tries to stream to an older server.

I think the error should occur when a newer client attempts to connect to an older server instead of waiting for the client to attempt stream.

AFAIK, SignalR clients have never selectively supported features based on server protocol version. It's always been all or nothing. That way you don't have to worry about providing a feature detection API. If you want to start using a new feature, you have to upgrade all your servers. In the past, we considered requiring server upgrades a smaller ask than requiring client upgrades anyway.

BrennanConroy · 2019-01-10T19:20:52Z

Started doing some microbenchmarking with the changes:

Method	Mean	Error	StdDev	Op/s	Gen 0	Allocated
InvocationAsync	1.344 us	0.0267 us	0.0625 us	744,175.5	0.0057	696 B
UploadStream_One	44.187 us	0.1145 us	0.0894 us	22,630.9	-	3496 B

This shows that there is greater up-front costs for client->server streaming

Method	Mean	Error	StdDev	Op/s	Gen 0	Allocated
UploadStream_Thousand	318.673 us	1.1018 us	0.9767 us	3,138.0	-	49074 B
Invoke_Thousand	1,349.203 us	21.8902 us	20.4761 us	741.2	5.8594	728000 B

And lower per-item cost when comparing sending 1000 of the same messages in streaming vs invoking.

...alR/benchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks/DefaultHubDispatcherBenchmark.cs

BrennanConroy added the area-signalr Includes: SignalR clients and servers label Dec 10, 2018

BrennanConroy requested review from halter73, davidfowl and mikaelm12 December 10, 2018 22:43

JamesNK reviewed Dec 11, 2018

View reviewed changes

halter73 reviewed Dec 11, 2018

View reviewed changes

src/SignalR/src/Microsoft.AspNetCore.SignalR.Common/Protocol/HubMethodInvocationMessage.cs Outdated Show resolved Hide resolved

mikaelm12 reviewed Dec 11, 2018

View reviewed changes

JamesNK reviewed Dec 13, 2018

View reviewed changes

JamesNK approved these changes Dec 13, 2018

View reviewed changes

mikaelm12 reviewed Dec 14, 2018

View reviewed changes

...icrosoft.AspNetCore.SignalR.Common.Tests/Internal/Protocol/TestHubMessageEqualityComparer.cs Show resolved Hide resolved

BrennanConroy force-pushed the brecon/streamRefactor branch from ee0f461 to 93df7af Compare December 14, 2018 01:46

BrennanConroy force-pushed the brecon/streamRefactor branch from af6dc8f to 1d350ab Compare January 2, 2019 21:11

mikaelm12 approved these changes Jan 2, 2019

View reviewed changes

src/SignalR/specs/HubProtocol.md Outdated Show resolved Hide resolved

src/SignalR/specs/HubProtocol.md Outdated Show resolved Hide resolved

halter73 approved these changes Jan 10, 2019

View reviewed changes

BrennanConroy force-pushed the brecon/streamRefactor branch 2 times, most recently from 339716d to 9b62ba4 Compare January 11, 2019 00:52

davidfowl reviewed Jan 11, 2019

View reviewed changes

...alR/benchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks/DefaultHubDispatcherBenchmark.cs Outdated Show resolved Hide resolved

Refactor streaming from client to server

bc3af25

BrennanConroy force-pushed the brecon/streamRefactor branch from 9b62ba4 to bc3af25 Compare January 11, 2019 04:42

BrennanConroy merged commit 3640182 into master Jan 11, 2019

BrennanConroy deleted the brecon/streamRefactor branch January 11, 2019 05:52

This was referenced Jan 11, 2019

StreamItem InvocationId cleanup #5315

Closed

Update HubProtocol spec with new message types and changes #5319

Closed

Refactor streaming from client to server #4559

Refactor streaming from client to server #4559

Uh oh!

Conversation

BrennanConroy commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Dec 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikaelm12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JamesNK Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JamesNK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BrennanConroy commented Jan 2, 2019

Uh oh!

mikaelm12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BrennanConroy commented Jan 5, 2019

Uh oh!

halter73 commented Jan 5, 2019

Uh oh!

BrennanConroy commented Jan 10, 2019

Uh oh!

Uh oh!

Uh oh!

BrennanConroy commented Dec 10, 2018 •

edited

Loading

JamesNK Dec 13, 2018 •

edited

Loading