sonvister / Binance

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

IServiceProvider ObjectDisposedException fix. #102

Closed tash649 closed 6 years ago

tash649 commented 6 years ago

Error occures when app is restarts. BinanceHttpClient.Initializer.IsValueCreate is true, so we dont recreate BinanceHttpClient, but is needed, because the old IServiceProvider (which uses in ApiRateLimiterProvider, RateLimiterProvider) already disposed

.net core 2.1.1

tash649 commented 6 years ago

and can you create a new release after merge? btw, have a nice day

tash649 commented 6 years ago

You can revert my changes in IServiceCollectionExtension and run test BinanceHttpClientTest.ServiceProviderDisposeNoThrows @sonvister

sonvister commented 6 years ago

@tash649, could you help me to better understand your use case? I see from your test that you are disposing of and recreating a ServiceProvider although you mention "when app is restart[ed]." To me this is a contradiction since restarting an app usually means restarting the process.

Assuming your test resembles your use case (and you are not restarting app), this is not the intended/expected use of IServiceProvider. The IServiceProvider used to initialize the BinanceHttpClient is intended to exist during the entire scope of an application.

If you are needing to dispose of and create a new ServiceProvider within the scope of your application, could you reconsider the usage of services (assuming other managed singletons) or use a separate application-level IServiceProvider for the Binance services?

tash649 commented 6 years ago

Sorry for my English, the restart it is recreation of the WebHost (.net core). Main issue is when I doing this, lazy value of BinanceHttpClient contains ApiRateLimiter which disposed already. @sonvister

I don't really understand why the check I removed is needed, because the singletone service is initialized once. I understand why you use lazy.

tash649 commented 6 years ago

can you explain why you are checking the isValueCreated value?

tash649 commented 6 years ago

from my point of view, this check is redundant

sonvister commented 6 years ago

@tash649, I understand the cause of the error. I do not understand why you are not using an application-level static IServiceProvider interface instance. Regardless, I think we can find a middle-ground....

From your perspective, the check is unnecessary. However, other users (seemingly a majority) did not want to use dependency injection, so the API needed to also support instantiation of BinanceApi. I believe the IsValueCreated check was just to ensure there aren't multiple non-disposed BinanceHttpClient instances in case of both DI and direct instantiation methods are used.

Ignoring any potential race conditions, it should be fine to use this:

if (BinanceHttpClient.Initializer.IsValueCreated)
    BinanceHttpClient.Initializer.Value.Dispose();
sonvister commented 6 years ago

@tash649, instead of merging this pull request, I added the modifications as mentioned in the previous comment with similar unit test cases. Let me know if these changes work for you.

tash649 commented 6 years ago

Thank you very much @sonvister

sonvister commented 6 years ago

@tash649, 0.2.0-beta7 is released.

Donations (DCR, LTC, BTC, other...) appreciated 😉

tash649 commented 6 years ago

Thank you.

(not recommended usage). NOTE: An application-scoped IServiceProvider is still recommended (to maintain singleton behavior

That's what I'm doing. I use application-scopes IServiceProvider. In my case I just can recreate this scope. Iam stopping Webhost, then I create a new one and start again. @sonvister

sonvister commented 6 years ago

@tash649, I am referring to using a single instance of IServiceProvider for the lifetime of your application (process) where one instance is either made available as a public static or injected into a constructor or method. Based on your unit test (all that you gave me to understand your use case) and this (assuming you are rebuilding the WebHost pipeline, etc.), you are not doing that.

I may better understand your use case now, but I would still recommend restarting your application by restarting the process (instead of rebuilding the WebHost from within). Have you considered/tried IApplicationLifetime?.

Regardless, I don't mind that this library supports (the odd case of) re-initializing IServiceProvider. Although it does smell and does not seem like the intended use of the DI framework, etc.

tash649 commented 6 years ago

WebHost has StopAsync method, which uses IApplicationLifetime as far as I understand. image

sonvister commented 6 years ago

I have not had much recent experience with ASP.NET, perhaps I will experiment with this sometime....

Also, I found this: "Allow overriding the hosting service provider". Seems others have had issues with the WebHost being the DI universe. The example usage: ExternalContainerInstanceCanBeUsedForEverything.