Adopt Bedrock client abstractions in SignalR client #11484

halter73 · 2019-06-24T03:35:58Z

This add IConnectionFactory and HttpEndPoint to Connections.Abtractions and implements it with the SignalR .NET Client's HttpConnectionFactory.

So far I've been pretty conservative with these changes trying to break as few people as possible while still removing the old SignalR.Client.IConnectionFactory.

Going forward we might want to consider something like adding a HubConnection.StartAsync overload that takes an EndPoint.

Addresses #10872

src/SignalR/clients/csharp/Client/src/HttpConnectionFactory.cs

davidfowl · 2019-06-24T04:10:59Z

src/Servers/Connections.Abstractions/src/HttpEndPoint.cs

+
+namespace Microsoft.AspNetCore.Connections
+{
+    public class HttpEndPoint : EndPoint


I wonder if this should be in abstractions or in the https.connections.common assembly.

I followed the pattern you used for FileHandleEndPoint down to the namespace and lack of virtual properties.

davidfowl · 2019-06-24T04:12:44Z

src/SignalR/clients/csharp/Client.Core/src/HubConnectionBuilder.cs

@@ -48,6 +52,12 @@ public HubConnection Build()
                throw new InvalidOperationException($"Cannot create {nameof(HubConnection)} instance. An {nameof(IConnectionFactory)} was not configured.");
            }

+            var endPoint = serviceProvider.GetService<EndPoint>();


I'm not a fan of having the EndPoint be directly in the container, it needs to hang off some option that gets passed to the HubConnection instead.

I figured you might say that. Do you want to add a new HubConnectionOptions type with and EndPoint property?

I was thinking making a HubConnection.StartAsync take an EndPoint might be even better if we're willing to make that hard of a break

That's a pretty awful API though. It would be more flexible though.

Actually what you have isn’t so bad it gets bad only when you add this to a bigger service collection.

I could see value in an IOptions<HubConnectionOptions> given the earlier discussions re constructor breaking changes... might be a safer approach.

src/SignalR/clients/csharp/Client/src/HttpConnectionFactory.cs

davidfowl · 2019-06-24T04:15:03Z

src/SignalR/clients/csharp/Client/src/HttpConnectionFactory.cs

+
+            if (_httpConnectionOptions.Url != null && _httpConnectionOptions.Url != castedHttpEndPoint.Url)
+            {
+                throw new InvalidOperationException($"If {nameof(HttpConnectionOptions)}.{nameof(HttpConnectionOptions.Url)} was set, it must match the {nameof(HttpEndPoint)}.{nameof(HttpEndPoint.Url)} passed to {nameof(ConnectAsync)}.");


How would this happen?

If the IConnectionFactory gets resolved directly from the ServiceProvider anyone can call ConnectAsync with any EndPoint.

I'd be OK with having it silently override the option though. I was thinking maybe we do that and log a warning.

src/SignalR/perf/Microbenchmarks/HubConnectionStartBenchmark.cs

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

davidfowl · 2019-06-24T04:31:04Z

src/SignalR/samples/ClientSample/RawSample.cs

@@ -34,10 +34,12 @@ public static async Task<int> ExecuteAsync(string baseUrl)
            baseUrl = string.IsNullOrEmpty(baseUrl) ? "http://localhost:5000/chat" : baseUrl;

            Console.WriteLine($"Connecting to {baseUrl}...");
-            var connection = new HttpConnection(new Uri(baseUrl));
+
+            var connection = new HttpConnection(new HttpConnectionOptions() { Url = new Uri(baseUrl) }, TransferFormat.Text, loggerFactory: null);


Is this the simplest way to construct the HttpConnection now? Can we add some defaults (like the TransferFormat) so this sort of code doesn't break:

https://github.com/Azure/azure-signalr/blob/7257fda52f219601f8efc34dd240728a9c4ea31c/src/Microsoft.Azure.SignalR.Common/ServiceConnections/ConnectionFactory.cs#L32-L44

src/Servers/Connections.Abstractions/src/HttpEndPoint.cs

jkotalik · 2019-06-24T15:23:14Z

src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs

@@ -181,8 +185,8 @@ public partial class HubConnection
        /// <remarks>
        /// The <see cref="IServiceProvider"/> used to initialize the connection will be disposed when the connection is disposed.
        /// </remarks>
-        public HubConnection(IConnectionFactory connectionFactory, IHubProtocol protocol, IServiceProvider serviceProvider, ILoggerFactory loggerFactory, IRetryPolicy reconnectPolicy)
-            : this(connectionFactory, protocol, serviceProvider, loggerFactory)
+        public HubConnection(IConnectionFactory connectionFactory, IHubProtocol protocol, EndPoint endPoint, IServiceProvider serviceProvider, ILoggerFactory loggerFactory, IRetryPolicy reconnectPolicy)


I assume keeping the original constructors (to avoid the breaking change) is the entire point of this change.

Users shouldn't be constructing HubConnection. If it weren't for DI this would be internal. I'm kinda 🤷‍♂ on breaking changes in this kind of thing.

Funnily enough, HubConnection has always been difficult/impossible to construct manually. Now that the HubConnection needs to specify an EndPoint that previously was just an implementation detail of IConnectionFactory, HubConnection has to break at least a little.

The main thing I was worried about was keeping the HubConnectionBuilder backwards-compatible.

~~Stupid question, can we make HubConnection internal?~~ nvrm, didn't read comments closely enough above.

We can't make the class internal as it's the class users interact with to send and receive messages. We would like the constructors to be internal but like Andrew said we can't because of DI.

src/SignalR/clients/csharp/Client/src/HttpConnectionFactory.cs

src/SignalR/clients/csharp/Client.Core/src/HubConnectionBuilder.cs

jkotalik · 2019-06-24T15:30:24Z

src/SignalR/clients/csharp/Client/src/HubConnectionBuilderHttpExtensions.cs

            hubConnectionBuilder.Services.AddSingleton<IConnectionFactory, HttpConnectionFactory>();
            return hubConnectionBuilder;
        }
+
+        private class HttpConnectionOptionsDerivedHttpEndPoint : HttpEndPoint


This name is interesting... maybe HttpEndPointWithConnectionOptions?

Meh, it's a private class. That's why I went with the overly-descriptive name.

That's why I went with the overly-descriptive name.

False, you do this in general 😄

jkotalik · 2019-06-24T15:30:53Z

src/SignalR/clients/csharp/Client/src/Microsoft.AspNetCore.SignalR.Client.csproj

@@ -11,4 +11,7 @@
    <Reference Include="Microsoft.AspNetCore.Http.Connections.Client" />
  </ItemGroup>

+  <ItemGroup>
+    <InternalsVisibleTo Include="Microsoft.AspNetCore.SignalR.Client.Tests" />


Wait this is a thing now? You don't need to put this in assembly attributes 😱

It IS a thing! You still need to have the full public key for things that aren't built by the repo, but if it's built here you can just use the simple assembly name and the rest is automated!

Yeah, I convert SignalR to use this and removed the AssemblyInfo files

src/SignalR/clients/csharp/Client/test/FunctionalTests/HubProtocolVersionTests.cs

BrennanConroy · 2019-06-24T17:17:47Z

src/SignalR/clients/csharp/Client/test/UnitTests/HttpConnectionFactoryTests.cs

-                HttpMessageHandlerFactory = _ => testHandler
-            }), NullLoggerFactory.Instance);
+            var factory = new HttpConnectionFactory(
+                Mock.Of<IHubProtocol>(p => p.TransferFormat == TransferFormat.Text),


Please try to avoid Moq where possible. This should easily be replaceable by new JsonHubProtocol()

I considered using new JsonHubProtocol() when I changed this, but decided to go with Mock.Of<IHubProtocol>(p => p.TransferFormat == TransferFormat.Text) because it's more self-documenting. It shows what the test is actually using the IHubProtocol for.

I'm mildly torn betweeen my preference to avoid Moq as much as possible and the fact that I do agree with @halter73 's self-documenting argument. I guess I'd consider just making a TestTextHubProtocol concrete class (that throws NotImplementedException except in the TransferFormat property) but I could go either way.

I find the mock more self-documenting than the TestTextHubProtocol for something as trivial as this, because I don't have to scroll to TestTextHubProtocol to see what it's doing.

And when TestTextHubProtocol inevitably starts getting used by other tests that started life as a copy of this one, you won't be able to see the implementation for TestTextHubProtocol in the new PRs that use it. And when those tests inevitably add more requirements, you get more methods/properties in TestTextHubProtocol that don't throw a NotImplementedException, so you don't know what's really being called by any given test even if you can find the TestTextHubProtocol implementation.

All of this for what should be a one-liner.

I'm over Moq

Are you suggesting I rewrite this? If so, can I at least get a reason? Even if it's something completely irrelevant like not working in environments that don't support IL emit.

No I don't really care, we've been gradually moving away from Moq as a team. Leave it the way it is if you want but generally I've been trying to avoid Moq as well when the situation is trivial. As we move towards support more AOT scenarios Moq won't work well (or it'll work but be really slow). Though that's a future thing, nothing to worry about yet 😄

BrennanConroy · 2019-06-24T17:23:49Z

src/SignalR/clients/csharp/Client/src/HubConnectionBuilderHttpExtensions.cs

+
+        private class HttpConnectionOptionsDerivedHttpEndPoint : HttpEndPoint
+        {
+            public HttpConnectionOptionsDerivedHttpEndPoint(IOptions<HttpConnectionOptions> httpConnectionOptions)


Couldn't we put the transferformat on HttpEndPoint and resolve it here through the IHubProtocol and avoid the IHubProtocol stuff you do in HttpConnectionFactory?

HttpEndPoint is not a solely SignalR concept. Other things that might want to connect to an HttpEndPoint probably don't care about SignalR's TransferFormat. The project bedrock API all deal with bytes/binary anyway.

We could create a SignalREndPoint that derives from HttpEndPoint to contain extra options, but at that point what options go into HttpConnectionOptions and what options go into SignalREndPoint?

TransferFormat is in Connection.Abstractions as well as it would be fine to add it to the HttpEndPoint

But what does a Text TransferFormat mean to anyone other than SignalR? The ConnectionContext exposes Pipes which are all binary. The TransferFormat seems like a protocol concern, not a transport/endpoint concern.

When using the lower HttpConnection layer, it implies that the transport cannot do binary, e.g. server sent events.

If we're going to put a TransferFormat on HttpEndPoint, shouldn't we add an Encoding as well? In SignalR's case, TransferFormat.Text implies that all bytes sent over the pipe is valid UTF-8, but what if a non-SignalR EndPoint only accepts ASCII or something else? Do you set TransferFormat.Text on the Endpoint, but use an options object for the encoding? The TransferFormat and encoding should be configured together IMHO.

Also, I looked at the long polling transport and it just completely ignores the TransferFormat. SSE validates the TransferFormat at the beginning of StartAsync, but f assumes TransferFormat.Text from there on out. If the code calling the ITransport simply wrote valid UTF-8 to the IDuplexPipe, everything would work fine with SSE without a need for ever passing in a TransferFormat.

So it's really only the WebSocket transport that needs to know up front whether you're sending binary or text data, and if it weren't for the F12 browser tools being crappy with binary WebSockets, we'd probably just force WebSockets to binary all the time, even for text-based protocols.

Since this is the .NET-client, maybe forcing WebSocket to use the binary message mode even for text protocols is more reasonable. I'd also be OK with adding a TransferFormat to a WebSocketEndPoint which could b derived from HttpEndPoint.

If we're going to put a TransferFormat on HttpEndPoint, shouldn't we add an Encoding as well? In SignalR's case, TransferFormat.Text implies that all bytes sent over the pipe is valid UTF-8, but what if a non-SignalR EndPoint only accepts ASCII or something else? Do you set TransferFormat.Text on the Endpoint, but use an options object for the encoding? The TransferFormat and encoding should be configured together IMHO.

Sounds good add a TextEncoding property that should be used if TransferFormat is Text?

That raises the question of whether the Encoding is really a property of the HttpEndPoint. If Encoding is part and parcel of the HttpEndPoint, what configuration isn't. Why don't we consider the CloseTimeout part of the HttpEndPoint? Or IWebProxy? Or UseDefaultCredentials? I could see the argument for Headers or Cookies, but where's the line?

And why is TranferFormat and Encoding a thing specific to HttpEndPoints? Why don't other EndPoints have these properties? Why not the file handle EndPoint?

This is similar to socket options for IPEndPoint, we had this discussion remember? Either we need to abandon Endpoint and have something that is more loose that allows passing binding specific information (like all of the useful properties you describe) or we say those come in via a side channel.

You can specify any EndPoint you want if you supply an IConnectionFactory that supports it.
Just look at the sample project. It uses an IPEndPoint with HubConnection.

I'm fine with putting a bunch of SignalR specific stuff on an EndPoint type (though people will still likely get confused which options go where unless we put either all or none of the options on the EndPoint type). Just call it SignalREndPoint or something and move it out of the Connection.Abstractions assembly.

BrennanConroy · 2019-06-24T17:32:03Z

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

@@ -212,7 +211,7 @@ private async Task StartAsyncCore(TransferFormat transferFormat, CancellationTok

                Log.Starting(_logger);

-                await SelectAndStartTransport(transferFormat, cancellationToken);
+                await SelectAndStartTransport(_transferFormat, cancellationToken);


All public API needs .ForceAsync()

Also, you've removed the logger scope

I'm surprised no tests failed after removing the logger scope. It'd be nice if we tested the ForceAsync() stuff too.

src/Servers/Connections.Abstractions/src/IConnectionFactory.cs

src/Servers/Connections.Abstractions/src/HttpEndPoint.cs

src/Servers/Connections.Abstractions/src/IConnectionFactory.cs

analogrelay · 2019-06-24T20:36:36Z

src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs

@@ -181,8 +185,8 @@ public partial class HubConnection
        /// <remarks>
        /// The <see cref="IServiceProvider"/> used to initialize the connection will be disposed when the connection is disposed.
        /// </remarks>
-        public HubConnection(IConnectionFactory connectionFactory, IHubProtocol protocol, IServiceProvider serviceProvider, ILoggerFactory loggerFactory, IRetryPolicy reconnectPolicy)
-            : this(connectionFactory, protocol, serviceProvider, loggerFactory)
+        public HubConnection(IConnectionFactory connectionFactory, IHubProtocol protocol, EndPoint endPoint, IServiceProvider serviceProvider, ILoggerFactory loggerFactory, IRetryPolicy reconnectPolicy)


Users shouldn't be constructing HubConnection. If it weren't for DI this would be internal. I'm kinda 🤷‍♂ on breaking changes in this kind of thing.

analogrelay · 2019-06-24T20:38:00Z

src/SignalR/clients/csharp/Client.Core/src/HubConnectionBuilder.cs

@@ -48,6 +52,12 @@ public HubConnection Build()
                throw new InvalidOperationException($"Cannot create {nameof(HubConnection)} instance. An {nameof(IConnectionFactory)} was not configured.");
            }

+            var endPoint = serviceProvider.GetService<EndPoint>();


I could see value in an IOptions<HubConnectionOptions> given the earlier discussions re constructor breaking changes... might be a safer approach.

analogrelay · 2019-06-24T20:50:35Z

src/SignalR/clients/csharp/Client/test/UnitTests/HttpConnectionFactoryTests.cs

-                HttpMessageHandlerFactory = _ => testHandler
-            }), NullLoggerFactory.Instance);
+            var factory = new HttpConnectionFactory(
+                Mock.Of<IHubProtocol>(p => p.TransferFormat == TransferFormat.Text),


I'm mildly torn betweeen my preference to avoid Moq as much as possible and the fact that I do agree with @halter73 's self-documenting argument. I guess I'd consider just making a TestTextHubProtocol concrete class (that throws NotImplementedException except in the TransferFormat property) but I could go either way.

mikaelm12 · 2019-06-24T20:38:24Z

src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs

-        /// <param name="protocol">The <see cref="IHubProtocol" /> used by the connection.</param>
-        /// <param name="loggerFactory">The logger factory.</param>
-        public HubConnection(IConnectionFactory connectionFactory, IHubProtocol protocol, ILoggerFactory loggerFactory)
+        public HubConnection(IConnectionFactory connectionFactory,


I don't think we format ctors like this any where in SignalR

There's a first time for everything. Seriously though, I prefer this for DI-invoked ctors with a lot of arguments. In fact, it looks like we already do this in our HubConnectionHandler.

I also prefer this for more than 3 args. I was angling more for consistency.

mikaelm12 · 2019-06-24T20:51:26Z

src/SignalR/clients/csharp/Client/src/HttpConnectionFactory.cs

        {
+            if (hubProtocol == null)


Are we gonna use throw expressions throughout?

I would, but throw expressions don't look as nice once you start trying to pull out a specific property like IHubProtocol.TransferFormat and store that in the field instead of the entire argument. You end up needing a ternary expression or something. I assume this is the same reason we didn't use a throw expression for options and loggerFactory.

mikaelm12 · 2019-06-24T20:58:17Z

src/SignalR/clients/csharp/Client/src/Microsoft.AspNetCore.SignalR.Client.csproj

@@ -11,4 +11,7 @@
    <Reference Include="Microsoft.AspNetCore.Http.Connections.Client" />
  </ItemGroup>

+  <ItemGroup>
+    <InternalsVisibleTo Include="Microsoft.AspNetCore.SignalR.Client.Tests" />


Yeah, I convert SignalR to use this and removed the AssemblyInfo files

halter73 · 2019-06-27T05:42:58Z

⬆️📅

jkotalik · 2019-06-27T16:19:46Z

src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs

@@ -452,7 +451,7 @@ private async Task StartAsyncCore(CancellationToken cancellationToken)

        private Task CloseAsync(ConnectionContext connection)
        {
-            return _connectionFactory.DisposeAsync(connection);
+            return connection.DisposeAsync().AsTask();


Can this return ValueTask?

BrennanConroy

Looking good

BrennanConroy · 2019-06-27T17:36:58Z

src/SignalR/clients/csharp/Client/src/HttpConnectionFactory.cs

        {
-            return connection.DisposeAsync().AsTask();
+            return new HttpConnectionOptions


Is there anyway for us to make sure additions to the options get updated here too?

Like make a clone method on HttpConnectionOptions? Or a comment in that file mentioning this?

Ah, I see you use reflection in a test to go over all properties 😄

BrennanConroy · 2019-06-27T17:44:22Z

src/SignalR/clients/csharp/Client/src/HttpConnectionFactory.cs

+
+            if (!(endPoint is UriEndPoint uriEndPoint))
+            {
+                throw new NotSupportedException($"The provided {nameof(EndPoint)} must be of type {nameof(UriEndPoint)}.");


nit:

Suggested change

throw new NotSupportedException($"The provided {nameof(EndPoint)} must be of type {nameof(UriEndPoint)}.");

throw new NotSupportedException($"The provided {nameof(EndPoint)} must be of type '{nameof(UriEndPoint)}'.");

halter73 · 2019-06-27T20:49:53Z

/azp run AspNetCore-ci

azure-pipelines · 2019-06-27T20:50:02Z

Azure Pipelines successfully started running 1 pipeline(s).

- Remove using for nonexistent Connections.Abstractions namespace

davidfowl · 2019-06-28T00:20:08Z

👍

halter73 requested review from analogrelay, davidfowl, mikaelm12 and BrennanConroy June 24, 2019 03:35

halter73 requested review from jkotalik and Tratcher as code owners June 24, 2019 03:35

Eilon added the area-signalr Includes: SignalR clients and servers label Jun 24, 2019