Skip to content

Adopt Bedrock client abstractions in SignalR client #11484

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Jun 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ public partial interface IConnectionBuilder
Microsoft.AspNetCore.Connections.ConnectionDelegate Build();
Microsoft.AspNetCore.Connections.IConnectionBuilder Use(System.Func<Microsoft.AspNetCore.Connections.ConnectionDelegate, Microsoft.AspNetCore.Connections.ConnectionDelegate> middleware);
}
public partial interface IConnectionFactory
{
System.Threading.Tasks.ValueTask<Microsoft.AspNetCore.Connections.ConnectionContext> ConnectAsync(System.Net.EndPoint endPoint, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken));
}
public partial interface IConnectionListener : System.IAsyncDisposable
{
System.Net.EndPoint EndPoint { get; }
Expand All @@ -125,6 +129,11 @@ public enum TransferFormat
Binary = 1,
Text = 2,
}
public partial class UriEndPoint : System.Net.EndPoint
{
public UriEndPoint(System.Uri uri) { }
public System.Uri Uri { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
}
}
namespace Microsoft.AspNetCore.Connections.Features
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ public partial interface IConnectionBuilder
Microsoft.AspNetCore.Connections.ConnectionDelegate Build();
Microsoft.AspNetCore.Connections.IConnectionBuilder Use(System.Func<Microsoft.AspNetCore.Connections.ConnectionDelegate, Microsoft.AspNetCore.Connections.ConnectionDelegate> middleware);
}
public partial interface IConnectionFactory
{
System.Threading.Tasks.ValueTask<Microsoft.AspNetCore.Connections.ConnectionContext> ConnectAsync(System.Net.EndPoint endPoint, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken));
}
public partial interface IConnectionListener : System.IAsyncDisposable
{
System.Net.EndPoint EndPoint { get; }
Expand All @@ -125,6 +129,11 @@ public enum TransferFormat
Binary = 1,
Text = 2,
}
public partial class UriEndPoint : System.Net.EndPoint
{
public UriEndPoint(System.Uri uri) { }
public System.Uri Uri { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
}
}
namespace Microsoft.AspNetCore.Connections.Features
{
Expand Down
25 changes: 25 additions & 0 deletions src/Servers/Connections.Abstractions/src/IConnectionFactory.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Net;
using System.Threading;
using System.Threading.Tasks;

namespace Microsoft.AspNetCore.Connections
{
/// <summary>
/// A factory abstraction for creating connections to an endpoint.
/// </summary>
public interface IConnectionFactory
{
/// <summary>
/// Creates a new connection to an endpoint.
/// </summary>
/// <param name="endPoint">The <see cref="EndPoint"/> to connect to.</param>
/// <param name="cancellationToken">The token to monitor for cancellation requests. The default value is <see cref="CancellationToken.None" />.</param>
/// <returns>
/// A <see cref="ValueTask{TResult}" /> that represents the asynchronous connect, yielding the <see cref="ConnectionContext" /> for the new connection when completed.
/// </returns>
ValueTask<ConnectionContext> ConnectAsync(EndPoint endPoint, CancellationToken cancellationToken = default);
}
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Net;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

Expand Down
28 changes: 28 additions & 0 deletions src/Servers/Connections.Abstractions/src/UriEndPoint.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Net;

namespace Microsoft.AspNetCore.Connections
{
/// <summary>
/// An <see cref="EndPoint"/> defined by a <see cref="System.Uri"/>.
/// </summary>
public class UriEndPoint : EndPoint
{
/// <summary>
/// Initializes a new instance of the <see cref="UriEndPoint"/> class.
/// </summary>
/// <param name="uri">The <see cref="System.Uri"/> defining the <see cref="EndPoint"/>.</param>
public UriEndPoint(Uri uri)
{
Uri = uri ?? throw new ArgumentNullException(nameof(uri));
}

/// <summary>
/// The <see cref="System.Uri"/> defining the <see cref="EndPoint"/>.
/// </summary>
public Uri Uri { get; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ public partial class HubConnection
public static readonly System.TimeSpan DefaultHandshakeTimeout;
public static readonly System.TimeSpan DefaultKeepAliveInterval;
public static readonly System.TimeSpan DefaultServerTimeout;
public HubConnection(Microsoft.AspNetCore.SignalR.Client.IConnectionFactory connectionFactory, Microsoft.AspNetCore.SignalR.Protocol.IHubProtocol protocol, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory) { }
public HubConnection(Microsoft.AspNetCore.SignalR.Client.IConnectionFactory connectionFactory, Microsoft.AspNetCore.SignalR.Protocol.IHubProtocol protocol, System.IServiceProvider serviceProvider, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory) { }
public HubConnection(Microsoft.AspNetCore.SignalR.Client.IConnectionFactory connectionFactory, Microsoft.AspNetCore.SignalR.Protocol.IHubProtocol protocol, System.IServiceProvider serviceProvider, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory, Microsoft.AspNetCore.SignalR.Client.IRetryPolicy reconnectPolicy) { }
public HubConnection(Microsoft.AspNetCore.Connections.IConnectionFactory connectionFactory, Microsoft.AspNetCore.SignalR.Protocol.IHubProtocol protocol, System.Net.EndPoint endPoint, System.IServiceProvider serviceProvider, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory) { }
public HubConnection(Microsoft.AspNetCore.Connections.IConnectionFactory connectionFactory, Microsoft.AspNetCore.SignalR.Protocol.IHubProtocol protocol, System.Net.EndPoint endPoint, System.IServiceProvider serviceProvider, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory, Microsoft.AspNetCore.SignalR.Client.IRetryPolicy reconnectPolicy) { }
public string ConnectionId { get { throw null; } }
public System.TimeSpan HandshakeTimeout { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
public System.TimeSpan KeepAliveInterval { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
Expand Down Expand Up @@ -145,11 +144,6 @@ public enum HubConnectionState
Connecting = 2,
Reconnecting = 3,
}
public partial interface IConnectionFactory
{
System.Threading.Tasks.Task<Microsoft.AspNetCore.Connections.ConnectionContext> ConnectAsync(Microsoft.AspNetCore.Connections.TransferFormat transferFormat, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken));
System.Threading.Tasks.Task DisposeAsync(Microsoft.AspNetCore.Connections.ConnectionContext connection);
}
public partial interface IHubConnectionBuilder : Microsoft.AspNetCore.SignalR.ISignalRBuilder
{
Microsoft.AspNetCore.SignalR.Client.HubConnection Build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ public partial class HubConnection
public static readonly System.TimeSpan DefaultHandshakeTimeout;
public static readonly System.TimeSpan DefaultKeepAliveInterval;
public static readonly System.TimeSpan DefaultServerTimeout;
public HubConnection(Microsoft.AspNetCore.SignalR.Client.IConnectionFactory connectionFactory, Microsoft.AspNetCore.SignalR.Protocol.IHubProtocol protocol, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory) { }
public HubConnection(Microsoft.AspNetCore.SignalR.Client.IConnectionFactory connectionFactory, Microsoft.AspNetCore.SignalR.Protocol.IHubProtocol protocol, System.IServiceProvider serviceProvider, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory) { }
public HubConnection(Microsoft.AspNetCore.SignalR.Client.IConnectionFactory connectionFactory, Microsoft.AspNetCore.SignalR.Protocol.IHubProtocol protocol, System.IServiceProvider serviceProvider, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory, Microsoft.AspNetCore.SignalR.Client.IRetryPolicy reconnectPolicy) { }
public HubConnection(Microsoft.AspNetCore.Connections.IConnectionFactory connectionFactory, Microsoft.AspNetCore.SignalR.Protocol.IHubProtocol protocol, System.Net.EndPoint endPoint, System.IServiceProvider serviceProvider, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory) { }
public HubConnection(Microsoft.AspNetCore.Connections.IConnectionFactory connectionFactory, Microsoft.AspNetCore.SignalR.Protocol.IHubProtocol protocol, System.Net.EndPoint endPoint, System.IServiceProvider serviceProvider, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory, Microsoft.AspNetCore.SignalR.Client.IRetryPolicy reconnectPolicy) { }
public string ConnectionId { get { throw null; } }
public System.TimeSpan HandshakeTimeout { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
public System.TimeSpan KeepAliveInterval { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
Expand Down Expand Up @@ -145,11 +144,6 @@ public enum HubConnectionState
Connecting = 2,
Reconnecting = 3,
}
public partial interface IConnectionFactory
{
System.Threading.Tasks.Task<Microsoft.AspNetCore.Connections.ConnectionContext> ConnectAsync(Microsoft.AspNetCore.Connections.TransferFormat transferFormat, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken));
System.Threading.Tasks.Task DisposeAsync(Microsoft.AspNetCore.Connections.ConnectionContext connection);
}
public partial interface IHubConnectionBuilder : Microsoft.AspNetCore.SignalR.ISignalRBuilder
{
Microsoft.AspNetCore.SignalR.Client.HubConnection Build();
Expand Down
35 changes: 17 additions & 18 deletions src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Globalization;
using System.IO;
using System.Linq;
using System.Net;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Threading;
Expand All @@ -21,6 +22,7 @@
using Microsoft.AspNetCore.SignalR.Protocol;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using Microsoft.Extensions.Options;

namespace Microsoft.AspNetCore.SignalR.Client
{
Expand Down Expand Up @@ -59,6 +61,7 @@ public partial class HubConnection
private readonly IServiceProvider _serviceProvider;
private readonly IConnectionFactory _connectionFactory;
private readonly IRetryPolicy _reconnectPolicy;
private readonly EndPoint _endPoint;
private readonly ConcurrentDictionary<string, InvocationHandlerList> _handlers = new ConcurrentDictionary<string, InvocationHandlerList>(StringComparer.Ordinal);

// Holds all mutable state other than user-defined handlers and settable properties.
Expand Down Expand Up @@ -172,6 +175,7 @@ public partial class HubConnection
/// </summary>
/// <param name="connectionFactory">The <see cref="IConnectionFactory" /> used to create a connection each time <see cref="StartAsync" /> is called.</param>
/// <param name="protocol">The <see cref="IHubProtocol" /> used by the connection.</param>
/// <param name="endPoint">The <see cref="EndPoint"/> to connect to.</param>
/// <param name="serviceProvider">An <see cref="IServiceProvider"/> containing the services provided to this <see cref="HubConnection"/> instance.</param>
/// <param name="loggerFactory">The logger factory.</param>
/// <param name="reconnectPolicy">
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jkotalik jkotalik Jun 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

: this(connectionFactory, protocol, endPoint, serviceProvider, loggerFactory)
{
_reconnectPolicy = reconnectPolicy;
}
Expand All @@ -192,27 +196,22 @@ public HubConnection(IConnectionFactory connectionFactory, IHubProtocol protocol
/// </summary>
/// <param name="connectionFactory">The <see cref="IConnectionFactory" /> used to create a connection each time <see cref="StartAsync" /> is called.</param>
/// <param name="protocol">The <see cref="IHubProtocol" /> used by the connection.</param>
/// <param name="endPoint">The <see cref="EndPoint"/> to connect to.</param>
/// <param name="serviceProvider">An <see cref="IServiceProvider"/> containing the services provided to this <see cref="HubConnection"/> instance.</param>
/// <param name="loggerFactory">The logger factory.</param>
/// <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)
: this(connectionFactory, protocol, loggerFactory)
{
_serviceProvider = serviceProvider ?? throw new ArgumentNullException(nameof(serviceProvider));
}

/// <summary>
/// Initializes a new instance of the <see cref="HubConnection"/> class.
/// </summary>
/// <param name="connectionFactory">The <see cref="IConnectionFactory" /> used to create a connection each time <see cref="StartAsync" /> is called.</param>
/// <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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mikaelm12 mikaelm12 Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

IHubProtocol protocol,
EndPoint endPoint,
IServiceProvider serviceProvider,
ILoggerFactory loggerFactory)
{
_connectionFactory = connectionFactory ?? throw new ArgumentNullException(nameof(connectionFactory));
_protocol = protocol ?? throw new ArgumentNullException(nameof(protocol));
_endPoint = endPoint ?? throw new ArgumentException(nameof(endPoint));
_serviceProvider = serviceProvider ?? throw new ArgumentNullException(nameof(serviceProvider));

_loggerFactory = loggerFactory ?? NullLoggerFactory.Instance;
_logger = _loggerFactory.CreateLogger<HubConnection>();
Expand Down Expand Up @@ -424,7 +423,7 @@ private async Task StartAsyncCore(CancellationToken cancellationToken)
Log.Starting(_logger);

// Start the connection
var connection = await _connectionFactory.ConnectAsync(_protocol.TransferFormat, cancellationToken);
var connection = await _connectionFactory.ConnectAsync(_endPoint, cancellationToken);
var startingConnectionState = new ConnectionState(connection, this);

// From here on, if an error occurs we need to shut down the connection because
Expand All @@ -450,9 +449,9 @@ private async Task StartAsyncCore(CancellationToken cancellationToken)
Log.Started(_logger);
}

private Task CloseAsync(ConnectionContext connection)
private ValueTask CloseAsync(ConnectionContext connection)
{
return _connectionFactory.DisposeAsync(connection);
return connection.DisposeAsync();
}

// This method does both Dispose and Start, the 'disposing' flag indicates which.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@

using System;
using System.ComponentModel;
using System.Linq;
using System.Net;
using Microsoft.AspNetCore.Connections;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Options;

namespace Microsoft.AspNetCore.SignalR.Client
{
Expand Down Expand Up @@ -42,11 +46,11 @@ public HubConnection Build()
// The service provider is disposed by the HubConnection
var serviceProvider = Services.BuildServiceProvider();

var connectionFactory = serviceProvider.GetService<IConnectionFactory>();
if (connectionFactory == null)
{
var connectionFactory = serviceProvider.GetService<IConnectionFactory>() ??
throw new InvalidOperationException($"Cannot create {nameof(HubConnection)} instance. An {nameof(IConnectionFactory)} was not configured.");
}

var endPoint = serviceProvider.GetService<EndPoint>() ??
throw new InvalidOperationException($"Cannot create {nameof(HubConnection)} instance. An {nameof(EndPoint)} was not configured.");

return serviceProvider.GetService<HubConnection>();
}
Expand Down
35 changes: 0 additions & 35 deletions src/SignalR/clients/csharp/Client.Core/src/IConnectionFactory.cs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>Client for ASP.NET Core SignalR</Description>
Expand Down
Loading