-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Changes from all commits
85db70e
7bbdea1
67c3c85
c68bf43
51b83a4
9161f57
179dcf8
831b1e1
1cb2859
36fa09d
7e6301d
6d5501e
e732726
3ef4f84
50c08c9
d5ebf76
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
---|---|---|
@@ -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 |
---|---|---|
|
@@ -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; | ||
|
@@ -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 | ||
{ | ||
|
@@ -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. | ||
|
@@ -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"> | ||
|
@@ -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) | ||
: this(connectionFactory, protocol, endPoint, serviceProvider, loggerFactory) | ||
{ | ||
_reconnectPolicy = reconnectPolicy; | ||
} | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>(); | ||
|
@@ -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 | ||
|
@@ -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. | ||
|
This file was deleted.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 beinternal
. I'm kinda 🤷♂ on breaking changes in this kind of thing.There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.