UpdateSubscription getting disposed when the BittrexSocketClient is disposed

[daerhiel]

Describe the bug
I'm trying to use the DI to inject the clients into app services that is provided by AddBittrex() and I'm injecting the IBittrexSocketClient as a scoped service to perform the subscription operation. Right after the request handling is completed the client gets disposed and all of the subscriptions created by the client get disposed as well. As I understand, the socket client here shouldn't maintain all subscription dependencies and it's used only to create them and subscription should be handled by the SignalR hub and a calling dependencies themselves. I could switch to the singleton socket client, but it doesn't seem right. The same behavior is observed on Binance clients and Bittrex clients, didn't check the others yet.

To Reproduce

• Register clients with the code:

        services.AddBittrex((client, socket) =>
        {
            client.LogLevel = LogLevel.Trace;
            socket.LogLevel = LogLevel.Trace;
        });

• Inject the IBittrexSocketClient and create the subscriptions with the following code:

        var client = serviceProvider.GetRequiredService<IBittrexSocketClient>();
        _heartbeat = await client.SpotStreams.SubscribeToHeartbeatAsync(OnHeartbeat).ToException(Descriptor);
        _ticker = await client.SpotStreams.SubscribeToTickerUpdatesAsync(OnTicketUpdates).ToException(Descriptor);

• With handlers:

    private void OnTicketUpdates(DataEvent<BittrexTickersUpdate> data)
    {
        _logger.LogInformation("[{timestamp}] Ticker: {data}.", data.Timestamp, data.Topic);
    }

    private void OnHeartbeat(DataEvent<DateTime> data)
    {
        _logger.LogInformation("Heartbeat: {data}.", data);
    }

Expected behavior
There should be the way to leave the subscriptions active when disposing the client. Also it seems that subscriptions should have mechanisms to recover the connections with credentials by themselves.

Debug logging
The logs display the following:

2022/02/22 21:38:29:486 | Trace | Bittrex    | Socket 2 received 221 bytes in single message
2022/02/22 21:38:29:487 | Trace | Bittrex    | Socket 2 received 49 bytes in single message
2022/02/22 21:38:29:491 | Debug | Bittrex    | Disposing socket client, closing all subscriptions
2022/02/22 21:38:29:494 | Debug | Bittrex    | Closing all 2 subscriptions

[JKorf]

Hi, I understand what you're saying, but what is the advantage of keeping the subscriptions and connections alive when you're disposing the socket client? The socket client is nothing but just a manager of subscriptions/connections. Having everything cleaned up upon disposing the socket client is a good way to make sure there are no left over subscriptions or sockets running.

In the Bittrex case there is a bit of redundancy as CryptoExchange.Net and SignalR have some overlap. Normally SignalR would also handle socket reconnecting. This is now handled by the CryptoExchange.Net socket client however, as it does with all exchange implementations who don't rely on SignalR. You can look see the socket client a bit like a SignalR hub, it manages connections.

Now if you want something to have a longer lifetime than a request you'll need to make it a singleton, which makes sense for something that manages long running connections.

I hope I have made it somewhat clear

[daerhiel]

Well, that's a good question. The answer here is probably whether the BittrexSocketClient and other socket clients want to fit the dependency model supported by the DI container. AddBittrex() registers BitrexClient as transient, which is pretty okay meaning that every time you call for dependency, ie injecting it into the controller's or any other dependent's constructor the new instance will be created. But the BittrexSocketClient is registered as Scoped, which makes no sense in ASP.NET DI model, because the scoped dependencies are only valid in the context of the current request which means that controller's or any other dependent's constructor will receive the same object but that's valid only in the scope of a request (for example in the scope of an API call). But once request is executed, the scoped version of BittrexSocketClient will be destroyed.

It's good that you mentioned SignalR Hubs. Actually hubs are not the containers for subscriptions, Hubs are created and destroyed transiently, they're not singletons. As far as I know the SignalR subscription are managed on another level and they work independently of hubs, but Hubs just provide the API to subscription managers. You can inject hub as singleton technically and have access to it for the app whole lifetime but there's a potential for wait locks on the same hub if you have a lot of calls to the same instance.

The point of separating the clients from subscription managers is decoupling the APIs from subscription logic to maintain the single responsibility principle. For example you might want to open 100 connections and all of them should have different credentials. There's no way to play with DefaultConnectionOptions, you can hardly maintain 100 singletons (even though it's possible), but the easier solution is to create the lightweight client, pass the desired connection options to it and the client will interact with subscription manager to set up the reconnection strategy and so on and then gets destroyed as it's not needed anymore. You will have the access to the connection through the subscription object and connection manager will have the responsibility to destroy all of the subscription objects including forgotten ones when application lifetime is over.

I do understand that this approach is probably quite far from the current design and it's not feasible/desireable to change anything, but I just want to emphasize that having the DI registration method AddBittrex is a bit controversial and misleading in terms of the instantiation scope. And I'd say there's a potential scalability problem when you really need a lot of connection to maintain.. If you think that's too much, you can close the issue, I pretty much got my problem resolved tactically, but I think there's a strategic way to improve the architecture a little bit.. :)

[JKorf]

It's an interesting topic. You're obviously more familiar with SignalR than me, so ignore me assuming the SignalR hub is similar.
As for the socket client feeling a bit weird in the DI model I agree, but there is a place for it with Scoped lifetime. If you take a Blazor application, the scope is not based on a request, but based on client lifetime. It runs from when the client enters the site until the user leaves the website. So if you use a scoped socket client, it's a socket client per user lifetime. Which can be fine.
For Asp.Net it is not useful to have it scoped, agree.

As for decoupling the connection and the API subscription logic I see your point. But you're right, at this moment that doesn't fit the architecture. At the moment the socket client is responsible for interpreting messages received by the connections as well. If we'd want to shift this logic out of the socket client we'd need to provide the socket connection with the ability to process the data, probably in the form of some injected interface.

Short term, I think it's wise to have worked around it for now on your end ;)
But for a next major version of the CryptoExchange.Net base library I will surely look into this. I'll open an issue on the CC repository referencing this, as it's good food for thought, so thank you for your input. Feel free to add any thoughts to this issue.

[EricGarnier]

hi @daerhiel
your scenario of 100 clients using DefaultConnectionOptions would be difficult, as BittrexClientOption rely on a static field.

public static BittrexClientOptions Default { get; set; } = new BittrexClientOptions();

So your 100 calls to BittrexClient.SetDefaultOptions(options); as will result from 100 calls to AddBittrex will override each other settings.

        public static IServiceCollection AddBittrex(
            this IServiceCollection services, 
            Action<BittrexClientOptions, BittrexSocketClientOptions>? defaultOptionsCallback = null,
            ServiceLifetime? socketClientLifeTime = null)
        {
            if (defaultOptionsCallback != null)
            {
                var options = new BittrexClientOptions();
                var socketOptions = new BittrexSocketClientOptions();
                defaultOptionsCallback?.Invoke(options, socketOptions);

                BittrexClient.SetDefaultOptions(options); // write a static field
                BittrexSocketClient.SetDefaultOptions(socketOptions); // write a static field
            }

The design of BittrexClient and other client based on CryptoExchange assume that you use only one instance per process, or at least a single set of credentials unless you are very careful when you create the clients .

The rate limiter are an other subject that must be treated if you want to have different clients with different credentials.