Closed peterbozso closed 2 years ago
This statement is a bit misleading.
A singleton on a server would be shared across all users, but scoped services are isolated.
Your statement is true for Blazor WASM apps, but Fluxor is UI agnostic.
You could say that effects instances are created once per store instance and share a lifetime with the store.
Sorry, you are right, I was looking at it strictly from the point of Blazor WASM. How about this?
Important: effects instances are created once per store instance and share a lifetime with the store. In case of Blazor WASM, this means that effect classes are singletons, thus any services injected into them become singletons as well. If you want your effects to create new instances of services each time they are executed, you have to inject an IServiceScopeFactory
into them and create a new scope manually.
I don't like suggesting a solution because it comes with its own problems. I.e. dependencies of the new service will be new too, so won't have access to the same service instances with the same state as the user's main scope....
Important: Effects instances are created once per store instance and share a lifetime with the store.
This means that service instances injected into effects also share a lifetime with the store which, for long-lived scopes, means they will live for the life of the user's session. For example, Blazor apps keep one long-lived injection scope per browser window.
Then what do you suggest? :)
The reason why I opened #261 was to emphasize the issue with injecting typed HttpClients into Effects which makes them singletons, essentially stopping them from working properly. I think suggesting the use of IServiceScopeFactory
and creating scopes manually is a good solution to those, but maybe I am missing something.
The text I wrote was my suggestion :)
It should be safe to inject HttpClient and keep it.
Sorry, but I don't get it! As far as I know, you lose all the added benefit of IHttpClientFactory if you hold into the HttpClient instances.
Not at all. HttpClient is thread safe and a single instance is supposed to be reused. In fact, you'll get the same instance each time from IHttpClientFactory.
The only exception to this is when you need to put HttpHandlers into the request chain. For example, if you want to add the public certificate of the party you want to contact, or add your own certificate to verify your own identity.
Because the handler pipeline is not added per request but onto an HttpClient instance itself you can then only use that client for that purpose. So if you want to also call a different server you'll need a different HttpClient instance that isn't expecting the host to use that same server certificate.
So, IHttpClientFactory gives you a single instance per unique scenario to get around this limitation.
If you aren't using handlers, then a singleton HttpClient is fine (recommended).
This is true, but in case of a singleton HttpClient, how about the DNS change problems mentioned here?
Therefore, HttpClient is intended to be instantiated once and reused throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads. That issue will result in SocketException errors. Possible approaches to solve that problem are based on the creation of the HttpClient object as singleton or static, as explained in this Microsoft article on HttpClient usage. This can be a good solution for short-lived console apps or similar, that run a few times a day.
Another issue that developers run into is when using a shared instance of HttpClient in long-running processes. In a situation where the HttpClient is instantiated as a singleton or a static object, it fails to handle the DNS changes as described in this issue of the dotnet/runtime GitHub repository.
I can totally imagine having a Blazor WASM app built with Fluxor to be open for hours continuously in a workplace setting - either in a browser or packed as a desktop application with Electron. In that case, I think the issues outlined above could occur.
The above referenced documentation also explicitly says about keeping a singleton HttpClient:
This can be a good solution for short-lived console apps or similar, that run a few times a day.
I hadn't thought of DNS changes, thanks :)
I think IHttpClientFactory is registered as a singleton. Is using that causing you a problem?
That in itself is fine, but the specific scenario I am worried about here is typed clients. These are registered as transient, but when injecting them into Fluxor effects, they become singletons. :(
Of course I think it's totally fine to use just IHttpClientFactory
(which IS a singleton anyway, as mentioned here: "This extension method registers the internal DefaultHttpClientFactory
class to be used as a singleton for the interface IHttpClientFactory.
"), but it'd be important to make users of Fluxor aware of the fact that if they use typed HttpClients
in effects, they could end up in an unexpected situation.
You can create a typed client from IHttpClientFactory too before each use.
How about...
Important: Effects instances are created once per store instance and share a lifetime with the store.
This means that service instances injected into effects also share a lifetime with the store which, for long-lived scopes, means they will live for the life of the user's session. For example, Blazor apps keep one long-lived injection scope per browser window.
When injecting
HttpClient
and you anticipate a possible DNS change you could consider instead usingIHttpClientFactory
and requesting aHttpClient
per execution of the effect code. If you need a new instance of a service or each execution (e.g. aUnitOfWork
orDbContext
) you could consider usingIServiceScopeFactory
to create a new instance of those services.
You can create a typed client from IHttpClientFactory too before each use.
Good point!
The quoted part is perfect, I have nothing to add. :) Thanks for having such a fruitful discussion about this! Will you push these changes directly. Then we can close this PR.
If you put those changes into your PR, I'll happily approve them.
I knew that our two brains combined could come up with at least most of a single competent brain ;)
Haha, right! :D Changes pushed!
Thank you so much for sticking this out through to the end!
I really appreciate your contribution!
Thanks for taking my feedback into account! :) I think this library is great and I am happy I was able to help.
Updating documentation according to #261.