Add singleton factories to create instance per hub in management SDK

[Y-Sindo]

Summary of the changes

• Fully apply DI
• Add new methods GenerateClientAccessToken to get client endpoint and access token.
• Breaking changes: ILoggerFactory specified in CreateHubContextAsync is only passed to per hub instance. Global singleton instances shares one ILoggerFactory.

Usage example

Method1: use global service collection

public void ConfigureServices(IServiceCollection services)
{
      services.AddSignalRServiceManager();
      services.AddSingleton(endpointRouter);  //optional
      services.AddSingleton(loggerFactory);//optional
      services.WithAssembly(Assembly.GetCallingAssembly());//optional
}

Method2: use ServiceManagerBuilder

In this way, dynamic configuration is not supported.

    var builder =new ServiceManagerBuilder();
    builder.WithOptions(...);
    builder.WithConfiguration(...);
    builder.WithRouter(...);
    builder.WithLoggerFactory(...);
    var serviceManager=builder.Build();

[wanlwanl]

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

[Y-Sindo]

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

[vicancy]

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

[Y-Sindo]

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.

[wanlwanl]

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

[Y-Sindo]

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?

[wanlwanl]

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);

[Y-Sindo]

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

[wanlwanl]

you plan to get a new one for each request?

[Y-Sindo]

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

[wanlwanl]

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?

[Y-Sindo]

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.

[wanlwanl]

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

[Y-Sindo]

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.

[wanlwanl]

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

[Y-Sindo]

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

[vicancy]

shall we let users decide if they AddLogging ?

[Y-Sindo]

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

[vicancy]

We can follow the general practice for a library package, for example, does SignalR pakage add default logger?

[Y-Sindo]

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.

[vicancy]

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

[vicancy]

remove this //TODO comments with little meanings, or add more details into it about what todo

[Y-Sindo]

This test is for reconnection function and doesn't work anymore, because connectionContainer isn't in the service provider of a ServiceHubContext and it's not convenient to add it to the service collection per hub. I think ideally this test should do for connection container class. I intend to add a unit test for connection container separately latter.

[vicancy]

LGTM