SignalR ConnectionToken/ConnectionAddress feature #13773

mikaelm12 · 2019-09-06T16:28:20Z

Adding the connectionToken connectionAddress split.

src/SignalR/common/Http.Connections.Common/src/NegotiationResponse.cs

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs

src/SignalR/clients/csharp/Client/test/UnitTests/HttpConnectionTests.Negotiate.cs

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs

src/SignalR/common/Http.Connections/test/NegotiateProtocolTests.cs

…version

mikaelm12 · 2019-09-11T18:34:21Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=349911

mikaelm12 · 2019-09-11T20:07:03Z

So we an old client connects to a new server, we're just setting the connectionId and connectionToken to the same value. This keeps up from having to handle separate code paths on the server for old and new clients.

BrennanConroy

I think a majority of the tests in HttpConnectionDispatcherTests are using negotiate version 0 still

src/SignalR/clients/csharp/Client/test/UnitTests/HttpConnectionTests.Negotiate.cs

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

src/SignalR/clients/csharp/Client/test/UnitTests/HttpConnectionTests.Negotiate.cs

src/SignalR/common/Http.Connections/test/HttpConnectionManagerTests.cs

aspnet-hello · 2019-09-12T17:51:41Z

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2765
https://github.com/aspnet/AspNetCore-Internal/issues/2617
https://github.com/aspnet/AspNetCore-Internal/issues/2730

halter73 · 2019-09-13T01:03:14Z

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs

@@ -75,7 +77,7 @@ public HttpConnectionContext(string id, ILogger logger)
        }

        public HttpConnectionContext(string id, IDuplexPipe transport, IDuplexPipe application, ILogger logger = null)
-            : this(id, logger)
+            : this(id, null, logger)


Is this only used for tests? If so, can we add a comment or something? Maybe make it internal.

Since we plan to bring this into 3.1 I don't think we can make it internal, but I agree that it should have been internal to start

Internal class, make all the changes you want!

halter73 · 2019-09-13T01:06:41Z

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

+                var queryStringVersionValue = qsVersion.ToString();
+                int.TryParse(queryStringVersionValue, out var clientProtocolVersion);
+                return _manager.CreateConnection(transportPipeOptions, appPipeOptions, clientProtocolVersion);
+


super nit:

Suggested change

halter73 · 2019-09-13T01:10:05Z

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs


            Log.CreatedNewConnection(_logger, id);
            var connectionTimer = HttpConnectionsEventSource.Log.ConnectionStart(id);
-            var connection = new HttpConnectionContext(id, _connectionLogger);
+            var connection = new HttpConnectionContext(id, connectionKey, _connectionLogger);


I would prefer if we stuck with the connectionId and connectionToken naming throughout. I'd replaces all usages of connectionKey, privateId, etc.. with connectionToken. I'd even rename id to connectionId just to make things super clear.

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

BrennanConroy

After looking at this for awhile, I realized I strongly dislike us writing a connectionToken for negotiate version 0. We should just omit it.

It will also conveniently force all our tests to be correct.

BrennanConroy · 2019-09-12T23:12:57Z

src/SignalR/clients/csharp/Client/test/UnitTests/HttpConnectionTests.Negotiate.cs

+
+                Assert.Equal("0rge0d00-0040-0030-0r00-000q00r00e00", connectionId);
+                Assert.Equal("http://fakeuri.org/negotiate?negotiateVersion=1", testHttpHandler.ReceivedRequests[0].RequestUri.ToString());
+                Assert.Equal("http://fakeuri.org/?negotiateVersion=1&id=0rge0d00-0040-0030-0r00-000q00r00e00", testHttpHandler.ReceivedRequests[1].RequestUri.ToString());


Oh no, this is ugly. Don't have to do it in this PR, but we might want to remove the version from the query string after negotiate

Will file a follow up issue.

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs

src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs

mikaelm12 · 2019-09-16T21:21:36Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=356374

aspnet-hello · 2019-09-16T21:22:04Z

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/3130

Start connection address change

254ec88

Eilon added the area-signalr Includes: SignalR clients and servers label Sep 6, 2019

some more tests

335c2dd

mikaelm12 commented Sep 9, 2019

View reviewed changes

src/SignalR/common/Http.Connections.Common/src/NegotiationResponse.cs Show resolved Hide resolved

src/SignalR/common/Http.Connections.Common/src/NegotiationResponse.cs Outdated Show resolved Hide resolved

mikaelm12 added 2 commits September 9, 2019 10:30

HttpConnectionManagerTest

b85eeba

renames and other updates

c802290

mikaelm12 marked this pull request as ready for review September 10, 2019 00:26

mikaelm12 requested review from analogrelay, BrennanConroy and halter73 as code owners September 10, 2019 00:26

BrennanConroy reviewed Sep 10, 2019

View reviewed changes

mikaelm12 added 3 commits September 10, 2019 17:43

update and fix all the tests

ba9cd32

update

54cb5ba

code check

d544d21

mikaelm12 mentioned this pull request Sep 11, 2019

Add support for negotiateVersion query string parameter #13880

Merged

add logic to decide which id to store based on the clients negotiate …

0de5189

…version

update

f5bb148

mikaelm12 mentioned this pull request Sep 12, 2019

Update SignalR transport protocol spec #13916

Merged

BrennanConroy reviewed Sep 12, 2019

View reviewed changes

mikaelm12 added 2 commits September 12, 2019 09:30

fix tests

eb01ffe

update

deb2901

mikaelm12 added 3 commits September 12, 2019 13:27

add negotiateVersion to HttpConnectionDispatcherTests

f6137d0

whitespace

6b347a8

comment

43c7bb3

halter73 reviewed Sep 13, 2019

View reviewed changes

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs Outdated Show resolved Hide resolved

BrennanConroy reviewed Sep 13, 2019

View reviewed changes

pr fb

d80bc8b

halter73 reviewed Sep 14, 2019

View reviewed changes

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs Outdated Show resolved Hide resolved

halter73 approved these changes Sep 14, 2019

View reviewed changes

BrennanConroy approved these changes Sep 16, 2019

View reviewed changes

pr fb

80d3089

mikaelm12 merged commit d7e19c4 into master Sep 16, 2019

mikaelm12 deleted the mikaelm12/ConnectionAddress branch September 16, 2019 21:22

analogrelay added this to the 5.0.0-preview1 milestone Sep 17, 2019

analogrelay added the 3.1-candidate label Sep 17, 2019

BrennanConroy pushed a commit that referenced this pull request Sep 24, 2019

SignalR ConnectionToken/ConnectionAddress feature (#13773)

7d59ae1

BrennanConroy mentioned this pull request Sep 24, 2019

Negotiate protocol versioning and ConnectionID split #14402

Merged

18 tasks

SignalR ConnectionToken/ConnectionAddress feature #13773

SignalR ConnectionToken/ConnectionAddress feature #13773

Uh oh!

Conversation

mikaelm12 commented Sep 6, 2019 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikaelm12 commented Sep 11, 2019

Uh oh!

mikaelm12 commented Sep 11, 2019

Uh oh!

BrennanConroy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aspnet-hello commented Sep 12, 2019

Uh oh!

halter73 Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

mikaelm12 Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

BrennanConroy Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

halter73 Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

halter73 Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BrennanConroy left a comment

Choose a reason for hiding this comment

Uh oh!

BrennanConroy Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

mikaelm12 Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikaelm12 commented Sep 16, 2019

Uh oh!

aspnet-hello commented Sep 16, 2019

Uh oh!

Uh oh!

mikaelm12 commented Sep 6, 2019 •

edited

Loading