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

IServiceProvider ObjectDisposedException fix. #102

Closed
wants to merge 2 commits into from
Closed

IServiceProvider ObjectDisposedException fix. #102

wants to merge 2 commits into from

Conversation

tash649
Copy link

@tash649 tash649 commented Sep 28, 2018

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

…is restarts. BinanceHttpClient.Initializer.IsValueCreate true, so we dont recreate BinanceHttpClient, but is needed, because the old IServiceProvider (which uses in ApiRateLimiterProvider, RateLimiterProvider) already disposed
@tash649
Copy link
Author

tash649 commented Sep 28, 2018

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

@tash649
Copy link
Author

tash649 commented Sep 28, 2018

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

@sonvister
Copy link
Owner

sonvister commented Sep 28, 2018

@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
Copy link
Author

tash649 commented Sep 28, 2018

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
Copy link
Author

tash649 commented Sep 28, 2018

can you explain why you are checking the isValueCreated value?

@tash649
Copy link
Author

tash649 commented Sep 28, 2018

from my point of view, this check is redundant

@sonvister
Copy link
Owner

@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
Copy link
Owner

@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
Copy link
Author

tash649 commented Sep 29, 2018

Thank you very much @sonvister

@tash649 tash649 closed this Sep 29, 2018
@sonvister
Copy link
Owner

@tash649, 0.2.0-beta7 is released.

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

@tash649
Copy link
Author

tash649 commented Sep 29, 2018

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
Copy link
Owner

@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
Copy link
Author

tash649 commented Sep 29, 2018

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

@sonvister
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants