Skip to content
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

Add singleton factories to create instance per hub in management SDK #1070

Merged
merged 13 commits into from
Nov 17, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System;
using System.Net.Http;
using Microsoft.Extensions.DependencyInjection;

namespace Microsoft.Azure.SignalR
{
Expand All @@ -13,21 +12,13 @@ internal class RestClientFactory
private readonly string _userAgent;
private readonly string _serverName;

internal RestClientFactory(string userAgent)
public RestClientFactory(string userAgent, IHttpClientFactory httpClientFactory)
{
var serviceCollection = new ServiceCollection()
.AddHttpClient();
_httpClientFactory = serviceCollection.BuildServiceProvider().GetRequiredService<IHttpClientFactory>();
_httpClientFactory = httpClientFactory;
_userAgent = userAgent;
_serverName = RestApiAccessTokenGenerator.GenerateServerName();
}

protected RestClientFactory(string userAgent, IHttpClientFactory httpClientFactory)
{
_userAgent = userAgent;
_httpClientFactory = httpClientFactory;
}

protected virtual HttpClient CreateHttpClient() => _httpClientFactory.CreateClient();

internal SignalRServiceRestClient Create(ServiceEndpoint endpoint)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
using Microsoft.Extensions.Options;
using Microsoft.Extensions.Primitives;

namespace Microsoft.Azure.SignalR.Management.Configuration
namespace Microsoft.Azure.SignalR.Management
{
internal class ServiceManagerOptionsSetup : IConfigureOptions<ServiceManagerOptions>, IOptionsChangeTokenSource<ServiceManagerOptions>
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
using System;
using System.ComponentModel;
using System.Net.Http;
using System.Reflection;
using Microsoft.Azure.SignalR.Management.Configuration;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Connections;
using Microsoft.AspNetCore.SignalR;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Options;

Expand All @@ -16,17 +19,7 @@ namespace Microsoft.Azure.SignalR.Management
/// </summary>
public static IServiceCollection AddSignalRServiceManager(this IServiceCollection services)
{
services.AddSingleton<ServiceManagerOptionsSetup>()
.AddSingleton<IConfigureOptions<ServiceManagerOptions>>(sp => sp.GetService<ServiceManagerOptionsSetup>())
.AddSingleton<IOptionsChangeTokenSource<ServiceManagerOptions>>(sp => sp.GetService<ServiceManagerOptionsSetup>());
services.PostConfigure<ServiceManagerOptions>(o => o.ValidateOptions());
services.AddSingleton<ServiceManagerContextSetup>()
.AddSingleton<IConfigureOptions<ServiceManagerContext>>(sp => sp.GetService<ServiceManagerContextSetup>())
.AddSingleton<IOptionsChangeTokenSource<ServiceManagerContext>>(sp => sp.GetService<ServiceManagerContextSetup>());
services.AddSingleton<ServiceOptionsSetup>()
.AddSingleton<IConfigureOptions<ServiceOptions>>(sp => sp.GetService<ServiceOptionsSetup>())
.AddSingleton<IOptionsChangeTokenSource<ServiceOptions>>(sp => sp.GetService<ServiceOptionsSetup>());
return services.TrySetProductInfo();
return services.AddSignalRServiceManager<ServiceManagerOptionsSetup>();
}

/// <summary>
Expand All @@ -38,6 +31,43 @@ public static IServiceCollection AddSignalRServiceManager(this IServiceCollectio
return services.AddSignalRServiceManager();
}

/// <summary>
/// Adds the essential SignalR Service Manager services to the specified services collection.
/// </summary>
/// <remarks>Designed for Azure Function extension where the setup of <see cref="ServiceManagerOptions"/> is different from SDK</remarks>
/// <typeparam name="TOptionsSetup">The type of class used to setup <see cref="ServiceManagerOptions"/>. </typeparam>
[EditorBrowsable(EditorBrowsableState.Never)]
public static IServiceCollection AddSignalRServiceManager<TOptionsSetup>(this IServiceCollection services) where TOptionsSetup : class, IConfigureOptions<ServiceManagerOptions>, IOptionsChangeTokenSource<ServiceManagerOptions>
{
//cascade options setup
services.AddSingleton<TOptionsSetup>()
.AddSingleton<IConfigureOptions<ServiceManagerOptions>>(sp => sp.GetService<TOptionsSetup>())
.AddSingleton<IOptionsChangeTokenSource<ServiceManagerOptions>>(sp => sp.GetService<TOptionsSetup>());
services.PostConfigure<ServiceManagerOptions>(o => o.ValidateOptions());
services.AddSingleton<ServiceManagerContextSetup>()
.AddSingleton<IConfigureOptions<ServiceManagerContext>>(sp => sp.GetService<ServiceManagerContextSetup>())
.AddSingleton<IOptionsChangeTokenSource<ServiceManagerContext>>(sp => sp.GetService<ServiceManagerContextSetup>());

services.AddSignalR()
.AddAzureSignalR<ServiceOptionsSetup>();

//add dependencies for persistent mode only
services
.AddSingleton<ConnectionFactory>()
.AddSingleton<IConnectionFactory,ManagementConnectionFactory>()
.AddSingleton<ConnectionDelegate>((connectionContext) => Task.CompletedTask)
.AddSingleton<IServiceConnectionFactory, ServiceConnectionFactory>()
.AddSingleton<MultiEndpointConnectionContainerFactory>()
.AddSingleton<IConfigureOptions<HubOptions>, ManagementHubOptionsSetup>();

services.AddLogging()
Copy link
Member

Choose a reason for hiding this comment

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

shall we let users decide if they AddLogging ?

Copy link
Member Author

Choose a reason for hiding this comment

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

How about replace AddLogging() with services.TryAdd(ServiceDescriptor.Singleton<ILoggerFactory, LoggerFactory>()); to make sure we have a default impl of ILoggerFctory?

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 follow the general practice for a library package, for example, does SignalR pakage add default logger?

Copy link
Member Author

Choose a reason for hiding this comment

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

SignalR package doesn't addLogger. But since we already use ILoggerFactory, why we don't add default logger for user? If no default loggerFactory is set, program throws error when users don't specify logger. User can add their own loggerFactory and the default one won't take effect.

.AddSingleton<ServiceHubContextFactory>()
.AddSingleton<ServiceHubLifetimeManagerFactory>();
services.AddSingleton<IServiceManager, ServiceManager>();
services.AddRestClientFactory();
return services.TrySetProductInfo();
}

/// <summary>
/// Adds product info to <see cref="ServiceManagerContext"/>
/// </summary>
Expand All @@ -53,7 +83,17 @@ private static IServiceCollection TrySetProductInfo(this IServiceCollection serv
{
var assembly = Assembly.GetExecutingAssembly();
var productInfo = ProductInfo.GetProductInfo(assembly);
return services.Configure<ServiceManagerContext>(o => o.ProductInfo = o.ProductInfo ?? productInfo);
return services.Configure<ServiceManagerContext>(o => o.ProductInfo ??= productInfo);
}

private static IServiceCollection AddRestClientFactory(this IServiceCollection services) => services
.AddHttpClient()
.AddSingleton(sp =>
{
var options = sp.GetRequiredService<IOptions<ServiceManagerContext>>().Value;
var productInfo = options.ProductInfo;
var httpClientFactory = sp.GetRequiredService<IHttpClientFactory>();
return new RestClientFactory(productInfo, httpClientFactory);
});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using Microsoft.Azure.SignalR.Common;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;

namespace Microsoft.Azure.SignalR.Management
{
internal class MultiEndpointConnectionContainerFactory
{
private readonly IServiceConnectionFactory _connectionFactory;
private readonly ILoggerFactory _loggerFactory;
private readonly IServiceEndpointManager _endpointManager;
private readonly int _connectionCount;
private readonly IEndpointRouter _router;

public MultiEndpointConnectionContainerFactory(IServiceConnectionFactory connectionFactory, ILoggerFactory loggerFactory, IServiceEndpointManager serviceEndpointManager, IOptions<ServiceManagerContext> options, IEndpointRouter router = null)
{
_connectionFactory = connectionFactory;
_loggerFactory = loggerFactory;
_endpointManager = serviceEndpointManager;
_connectionCount = options.Value.ConnectionCount;
_router = router;
}

public MultiEndpointServiceConnectionContainer Create(string hubName, ILoggerFactory loggerFactoryPerHub = null)
{
var loggerFactory = loggerFactoryPerHub ?? _loggerFactory;
return new MultiEndpointServiceConnectionContainer(
hubName,
endpoint => new WeakServiceConnectionContainer(_connectionFactory, _connectionCount, endpoint, loggerFactory.CreateLogger<WeakServiceConnectionContainer>()),
_endpointManager,
_router,
loggerFactory);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.SignalR;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;

namespace Microsoft.Azure.SignalR.Management
{
internal class ServiceHubContextFactory
{
private readonly ServiceHubLifetimeManagerFactory _managerFactory;

public ServiceHubContextFactory(ServiceHubLifetimeManagerFactory managerFactory)
{
_managerFactory = managerFactory;
}

public async Task<IServiceHubContext> CreateAsync(string hubName, ILoggerFactory loggerFactory = null, CancellationToken cancellationToken = default)
{
var manager = await _managerFactory.CreateAsync(hubName, cancellationToken, loggerFactory);
var servicesPerHub = new ServiceCollection();
Copy link
Member

Choose a reason for hiding this comment

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

why the service collection is for each hub? isn't it enough for each service manager?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because IHubContext<Hub> is for each hub, and one ServiceManager could have multiple HubContext

Copy link
Member

Choose a reason for hiding this comment

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

yeah I am also confused here, why DI ctor not working ? public ServiceHubContextFactory(ServiceHubLifetimeManagerFactory managerFactory, IHubContext<Hub> context)

Copy link
Member Author

Choose a reason for hiding this comment

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

For two reasons:

  1. IHubContext<Hub> is not singleton. And its impl HubContext<> is an internal class, so we can't do services.AddTransient(typeof(IHubContext<>),typeof(HubContext<>));.
  2. HubContext<Hub> depends on HubLiftTimeManager<>, which depends on hub name in our impl.

servicesPerHub.AddSignalRCore();
servicesPerHub.AddSingleton((HubLifetimeManager<Hub>)manager);
var serviceProviderPerHub = servicesPerHub.BuildServiceProvider();
var hubContext = serviceProviderPerHub.GetRequiredService<IHubContext<Hub>>();// The impl of IHubContext<Hub> we want is an internal class. We can only get it by this way.
return new ServiceHubContext(hubContext, manager, serviceProviderPerHub);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.ComponentModel;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.SignalR;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;

namespace Microsoft.Azure.SignalR.Management
{
internal class ServiceHubLifetimeManagerFactory
{
private readonly IServiceProvider _serviceProvider;
private readonly MultiEndpointConnectionContainerFactory _connectionContainerFactory;
private readonly ServiceManagerContext _context;
public ServiceHubLifetimeManagerFactory(IServiceProvider sp, IOptions<ServiceManagerContext> context, MultiEndpointConnectionContainerFactory connectionContainerFactory)
{
_serviceProvider = sp;
_connectionContainerFactory = connectionContainerFactory;
_context = context.Value;
}public async Task<IServiceHubLifetimeManager> CreateAsync(string hubName, CancellationToken cancellationToken, ILoggerFactory loggerFactoryPerHub = null)
{
switch (_context.ServiceTransportType)
{
case ServiceTransportType.Persistent:
{
var container = _connectionContainerFactory.Create(hubName, loggerFactoryPerHub);
var connectionManager = new ServiceConnectionManager<Hub>();
connectionManager.SetServiceConnection(container);
_ = connectionManager.StartAsync();
Copy link
Member

Choose a reason for hiding this comment

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

This class is a factory for creating ServiceHubLifetimeManager, but it also start connection manager here. start should be called in other place

Copy link
Member Author

Choose a reason for hiding this comment

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

Strictly speaking, CreateServiceHubContext() is a method for creating ServiceHubContext, but we start connection manager inside it, and this is unavoidable. Given this, I don't think starting connection manager in another factory is unacceptable?

Copy link
Member

Choose a reason for hiding this comment

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

How about moving the preparison for connection manager into WebSocketsHubLifetimeManager? So that WebSocketsHubLifetimeManager is no knowledge of hub name, and is also able to put into service collection, and make it transient? And set the connection container and start the container in ServiceHubContextFactory.CreateAsync like the old version? @vicancy

                        var containerFactory = _serviceProvider.GetRequiredService<MultiEndpointConnectionContainerFactory>();
                        var container = containerFactory.Create(hubName,loggerFactoryPerHub);
                        var connectionManager = new ServiceConnectionManager<Hub>();
                        connectionManager.SetServiceConnection(container);

Copy link
Member Author

@Y-Sindo Y-Sindo Nov 12, 2020

Choose a reason for hiding this comment

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

If I understand correctly, WebSocketsHubLifetimeManager is transient means that IServiceHubContext is transient too?

Copy link
Member

Choose a reason for hiding this comment

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

you plan to get a new one for each request?

Copy link
Member Author

Choose a reason for hiding this comment

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

The key is that HubContext<> is an internal class, and we can't add it as transient.

await container.ConnectionInitializedTask.OrTimeout(cancellationToken);
return loggerFactoryPerHub == null ? ActivatorUtilities.CreateInstance<WebSocketsHubLifetimeManager<Hub>>(_serviceProvider, connectionManager) : ActivatorUtilities.CreateInstance<WebSocketsHubLifetimeManager<Hub>>(_serviceProvider, connectionManager, loggerFactoryPerHub);
Copy link
Member

Choose a reason for hiding this comment

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

since you already put most of the class as a service into service collection, why not put WebSocketsHubLifetimeManager and RestHubLifetimeManager into service collection as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

WebSocketsHubLifetimeManager and RestHubLifetimeManager are not singletons, they are for hub. And we don't know the hub name until CreateServiceHubContext(...) is called, while hub name is bound to ServiceHubLifetimeManager.

Copy link
Member

Choose a reason for hiding this comment

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

I thinks it's better prepare all the thing before actually start the container, so put the startAsync to ServiceHubContextFactory.CreateAsync is better

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the actual difference between starting connection manager in ServiceHubLifetimeManagerFactory and ServiceHubContextFactory? Now ServiceHubContextFactory doesn't need to know the actual impl of IServiceHubLifetimeManager . I think to put connectionManager.StartAsync() in ServiceHubContextFactory breaks the hierarchy of the code.

Copy link
Member

Choose a reason for hiding this comment

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

what I mean is to put the used classes into service collections if possible, when everything is ready, start the connections

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not singleton and we can't create connectionManager until we know hub name?

}
case ServiceTransportType.Transient:
{
return new RestHubLifetimeManager(hubName, _context.ServiceEndpoints.Single(), _context.ProductInfo, _context.ApplicationName);
}
default:throw new InvalidEnumArgumentException(nameof(ServiceManagerContext.ServiceTransportType),(int)_context.ServiceTransportType,typeof(ServiceTransportType));
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,18 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Connections;
using Microsoft.Extensions.Options;

namespace Microsoft.Azure.SignalR.Management
{
internal class ManagementConnectionFactory : IConnectionFactory
{
private readonly string _productInfo;
private readonly IConnectionFactory _connectionFactory;
private readonly ConnectionFactory _connectionFactory;

public ManagementConnectionFactory(string productInfo, IConnectionFactory connectionFactory)
public ManagementConnectionFactory(IOptions<ServiceManagerContext> context, ConnectionFactory connectionFactory)
{
_productInfo = productInfo;
_productInfo = context.Value.ProductInfo;
_connectionFactory = connectionFactory;
}

Expand Down
Loading