Add "allowReconnect" to SignalR CloseMessages #14908

halter73 · 2019-10-11T01:55:12Z

Add "allowReconnect" to SignalR CloseMessages so the Azure SignalR Service can disconnect clients with a reason while still allowing the automatic reconnect features of the C# and TS/JS client to automatically reconnect.

For backwards compatibility, received CloseMessages with "allowReconnect" completely missing is interpreted the same as if it were false.

Compat Testing:

Tested 3.0 JS Client receiving close message with this new flag from 3.1 Server with JSON protocol
Tested 3.0 JS Client receiving close message with this new flag from 3.1 Server with MessagePack protocol
Tested 3.0 .NET Client receiving close message with this new flag from 3.1 Server with JSON protocol
Tested 3.0 .NET Client receiving close message with this new flag from 3.1 Server with MessagePack protocol
Tested 3.1 JS Client receiving close message without this new flag from 3.0 Server with MessagePack protocol
Tested 3.1 .NET Client receiving close message without this new flag from 3.0 Server with MessagePack protocol

src/SignalR/server/Core/src/HubConnectionContext.cs

halter73 · 2019-10-11T02:16:46Z

src/SignalR/common/SignalR.Common/ref/Microsoft.AspNetCore.SignalR.Common.netcoreapp.cs

        public static readonly Microsoft.AspNetCore.SignalR.Protocol.CloseMessage Empty;
        public CloseMessage(string error) { }
+        public CloseMessage(string error, bool allowAutomaticReconnect) { }
+        public bool AllowAutomaticReconnect { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }


Note the new API @davidfowl

analogrelay · 2019-10-11T16:19:09Z

As a reminder: Do not merge until I post back indicating we have approval from the release QB

BrennanConroy

We should also update the spec doc

Tests should be added to https://github.com/aspnet/AspNetCore/blob/9557630c0a470618b30679e8effcf019b1c74164/src/SignalR/common/SignalR.Common/test/Internal/Protocol/JsonHubProtocolTestsBase.cs#L36 and https://github.com/aspnet/AspNetCore/blob/9557630c0a470618b30679e8effcf019b1c74164/src/SignalR/common/SignalR.Common/test/Internal/Protocol/MessagePackHubProtocolTestBase.cs#L63

src/SignalR/clients/csharp/Client/test/UnitTests/HubConnectionTests.Reconnect.cs

src/SignalR/clients/ts/signalr/tests/HubConnection.test.ts

src/SignalR/common/Protocols.MessagePack/src/Protocol/MessagePackHubProtocol.cs

src/SignalR/common/Shared/JsonUtils.cs

BrennanConroy · 2019-10-14T16:16:33Z

src/SignalR/common/Shared/SystemTextJsonExtensions.cs

+        {
+            reader.Read();
+
+            return reader.TokenType switch


woah, is this that new pattern matching feature

+Pranav Points

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

src/SignalR/server/Core/src/HubConnectionContext.cs

src/SignalR/server/Core/src/HubConnectionHandler.cs

analogrelay · 2019-10-15T17:11:23Z

I put the "NO MERGE" label on just to make clear that we're pending approval for 3.1.

analogrelay · 2019-10-15T19:47:49Z

Approved for 3.1. Please explcitly confirm that cross-version compat testing of this functionality has been done (3.0 client to 3.1 server, 3.1 client to 3.0 server). I've added a checklist you can use to do that to the main description.

Approval confirmation: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1001918/

BrennanConroy

I'd like the protocol spec to be updated too.

src/SignalR/clients/ts/signalr/src/HubConnection.ts

halter73 · 2019-10-16T17:18:31Z

/azp run AspNetCore-ci

azure-pipelines · 2019-10-16T17:18:41Z

Azure Pipelines successfully started running 1 pipeline(s).

halter73 · 2019-10-16T20:57:56Z

src/SignalR/common/SignalR.Common/ref/Microsoft.AspNetCore.SignalR.Common.netcoreapp.cs

        public static readonly Microsoft.AspNetCore.SignalR.Protocol.CloseMessage Empty;
        public CloseMessage(string error) { }
+        public CloseMessage(string error, bool allowReconnect) { }
+        public bool AllowReconnect { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }


@aspnet/asp-net-api-reviews @davidfowl What do I need to do to get the api-approved label on this PR? I want to merge this soon since it for 3.1.

halter73 · 2019-10-16T20:58:23Z

/azp run AspNetCore-ci

azure-pipelines · 2019-10-16T20:58:33Z

Azure Pipelines successfully started running 1 pipeline(s).

Co-Authored-By: Brennan <[email protected]>

analogrelay · 2019-10-17T16:00:00Z

Compat Testing:

<chef-kiss.gif> ;). Excellent, thanks!

halter73 · 2019-10-17T20:53:51Z

src/Components/Server/src/BlazorPack/BlazorPackHubProtocol.cs

                    return true;
                case HubProtocolConstants.CloseMessageType:
-                    message = CreateCloseMessage(ref reader);
+                    message = CreateCloseMessage(ref reader, itemCount);


@rynowak @pranavkm @mkArtakMSFT FYI I just merged this PR into release/3.1 which changes BlazorPackHubProtocol.

Oof, probably should've gotten sign off from @pranavkm before the merge. Next time :). Can you take a quick look @pranavkm and see if this seems problematic?

halter73 added the area-signalr Includes: SignalR clients and servers label Oct 11, 2019

halter73 requested a review from BrennanConroy October 11, 2019 01:55

halter73 requested review from analogrelay and mikaelm12 as code owners October 11, 2019 01:55

halter73 commented Oct 11, 2019

View reviewed changes

src/SignalR/server/Core/src/HubConnectionContext.cs Outdated Show resolved Hide resolved

halter73 commented Oct 11, 2019

View reviewed changes

analogrelay added this to the 3.1.0-preview1 milestone Oct 11, 2019

analogrelay added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Oct 11, 2019

analogrelay modified the milestones: 3.1.0-preview1, 3.1.0-preview2 Oct 14, 2019

BrennanConroy reviewed Oct 14, 2019

View reviewed changes

halter73 force-pushed the halter73/closemessage-3.1 branch from aad9b2a to b4c3a38 Compare October 15, 2019 02:32

analogrelay added the * NO MERGE * Do not merge this PR as long as this label is present. label Oct 15, 2019

halter73 force-pushed the halter73/closemessage-3.1 branch from b4c3a38 to 1f00a16 Compare October 15, 2019 18:47

analogrelay added Servicing-approved Shiproom has approved the issue and removed * NO MERGE * Do not merge this PR as long as this label is present. labels Oct 15, 2019

halter73 force-pushed the halter73/closemessage-3.1 branch from 1f00a16 to a0b34d6 Compare October 16, 2019 01:08

halter73 requested a review from SteveSandersonMS as a code owner October 16, 2019 01:08

BrennanConroy approved these changes Oct 16, 2019

View reviewed changes

src/SignalR/clients/ts/signalr/src/HubConnection.ts Show resolved Hide resolved

halter73 commented Oct 16, 2019

View reviewed changes

halter73 added 3 commits October 16, 2019 15:29

Add CloseMessage.PreventAutomaticReconnect

797d62b

PreventAutomaticReconnect -> AllowAutomaticReconnect

a7c7f1e

TS client allowReconnect support

7ba5f79

halter73 and others added 7 commits October 16, 2019 15:29

Add C# unit tests

14c5817

Update ref assemblies

71c0fad

Apply suggestions from code review

c9a6722

Co-Authored-By: Brennan <[email protected]>

AllowAutomaticReconnect -> AllowReconnect

7d8184b

Update BlazorPackHubProtocol to support CloseMessage.AllowReconnect

bd9d8bb

Update HubProtocol.md with allowReconnect

4081038

Move new TS reconnect tests to proper file

4320912

halter73 force-pushed the halter73/closemessage-3.1 branch from a0b34d6 to 4320912 Compare October 16, 2019 22:44

davidfowl approved these changes Oct 17, 2019

View reviewed changes

davidfowl added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Oct 17, 2019

halter73 merged commit a4af618 into release/3.1 Oct 17, 2019

halter73 deleted the halter73/closemessage-3.1 branch October 17, 2019 20:31

halter73 commented Oct 17, 2019

View reviewed changes

Add "allowReconnect" to SignalR CloseMessages #14908

Add "allowReconnect" to SignalR CloseMessages #14908

Uh oh!

Conversation

halter73 commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

halter73 Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

analogrelay commented Oct 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!

BrennanConroy Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

analogrelay commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analogrelay commented Oct 15, 2019

Uh oh!

BrennanConroy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

halter73 commented Oct 16, 2019

Uh oh!

azure-pipelines bot commented Oct 16, 2019

Uh oh!

halter73 Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

halter73 commented Oct 16, 2019

Uh oh!

azure-pipelines bot commented Oct 16, 2019

Uh oh!

analogrelay commented Oct 17, 2019

Uh oh!

halter73 Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

analogrelay Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

halter73 commented Oct 11, 2019 •

edited

Loading

analogrelay commented Oct 15, 2019 •

edited

Loading