sonvister / Binance

A .NET Standard Binance API library.
MIT License
229 stars 77 forks source link

ApiRateLimiter Error with the latest API release #103

Closed evo11x closed 6 years ago

evo11x commented 6 years ago

After I have updated the API with the latest release I get this error when trying to download the chart Error | Cannot access a disposed object. Object name: 'ApiRateLimiter'.

I had to revert back to the previous version which works

evo11x commented 6 years ago

If I change this code from ServiceCollectionExtensions.cs back to previous edit, I no longer get the error

`if (BinanceHttpClient.Initializer.IsValueCreated)
        {
            services.AddSingleton<IBinanceHttpClient>(s =>
            {
                // Replace initializer.
                BinanceHttpClient.Initializer = new Lazy<BinanceHttpClient>(() =>
                    new BinanceHttpClient(
                        s.GetService<ITimestampProvider>(),
                        s.GetService<IApiRateLimiter>(),
                        s.GetService<IOptions<BinanceApiOptions>>(),
                        s.GetService<ILogger<BinanceHttpClient>>()), true);

                return BinanceHttpClient.Instance;
            });
        }`
sonvister commented 6 years ago

The code was:

if (!BinanceHttpClient.Initializer.IsValueCreated)
{
    // Replace initializer.
    BinanceHttpClient.Initializer = new Lazy<BinanceHttpClient>(() =>
        new BinanceHttpClient(
            s.GetService<ITimestampProvider>(),
            s.GetService<IApiRateLimiter>(),
            s.GetService<IOptions<BinanceApiOptions>>(),
            s.GetService<ILogger<BinanceHttpClient>>()), true);
}
sonvister commented 6 years ago

@evo11x, can you elaborate on "trying to download the chart" with some example code?

Are you, perhaps, using both new BinanceApi() and DI?

evo11x commented 6 years ago

I have 2 threads, one running with Binance API with getcandlestickasync and another one with BinanceAPI for order processor which is using another api key for orders, and I found that in the order processor I also had these 2 lines

       var api = services.GetService<IBinanceApi>();            
        var manager = services.GetService<IUserDataWebSocketManager>();  

and I think the second line destroyed the binanceapi object also for the thread which was downloading the candlesticks?

the manager object was never used in the code

evo11x commented 6 years ago

there is something wrong with the latest update, I have downloaded again the api and compiled and now the app is blocking without any error, switching back to previous version works perfectly. It does not even reach the binance api code and it's blocking the entire app...I am still trying to find where it's blocking

sonvister commented 6 years ago

@evo11x, it seems the line of code in question (the disposing of BinanceHttpClient) is being called after BinanceApi is initialized and before it is used. By using multiple threads, I was not able to reach that line of code to duplicate the issue (I suspect ServiceProvider is thread-safe).

However, I was able to reproduce both issues you mention if I initialize a ServiceProvider with each thread (instead of sharing one instance). Depending on the timing (added delays) I would either reach the dispose line and get an error or not or the test would hang.

So, are you using multiple ServiceProvider instances in your application?

evo11x commented 6 years ago

Yes, I have this in each thread

var services = new ServiceCollection()
                .AddBinance()
                .AddLogging(builder => builder.SetMinimumLevel(Microsoft.Extensions.Logging.LogLevel.Trace))
                .BuildServiceProvider();

you suggest to share this accross threads? But then I will be able to have two sets of api keys used, one for each thread?

evo11x commented 6 years ago

I am initializing the serviceprovider in each thread because I have a monitor for the threads when there is no internet connection so it restarts the threads, if I declare it static in main and initialize it at startup will it work if the threads will restart?

sonvister commented 6 years ago

Yes, @evo11x, you should have only one ServiceProvider in your application. A second one is redundant and it invalidates singleton usage.

This library was designed to support multiple sets of API keys (multiple users) with the singleton API client. If an API key is required, an IBinanceApiUser instance will be a parameter. The public market data does not require an API key. If you want to use multiple API keys, you would manage them with one or more IBinanceApiUser instances.

var user = services
    .GetService<IBinanceApiUserProvider>()
    .CreateUser("<API Key>", "<API Secret>");
evo11x commented 6 years ago

Ok, thank you! and can I use the GetServices method from different threads without a lock? I already use the public market data for the charts without API keys, but there is another thread which uses API keys which reads the account ballances and another one with API keys for the orders, so 3 in total.

sonvister commented 6 years ago

No problem. I haven't seen any official documentation stating ServiceProvider is thread-safe (and I didn't look at the source code), but I would expect you can do that without a lock.