DI Considerations

[dazinator]

Describe the bug
Hello!,
If I am reading the following code correctly, I think this could be a bug:

https://github.com/MarimerLLC/csla/blob/master/Source/Csla.Shared/ApplicationContext.cs#L861

In the case that a scope is not present in local context, a new scope is created and added. However I can't see that the scope ever gets disposed. Due to this, if there are any services resolved via this scope that are transient and implement IDisposable (i.e this would mean HttpClient being registered as transient, or an @Inject'ed paramater happening to be a transient IDisposable, those instances will build up over time in this DI scopes internal "tracking" list (it keeps track of transient disposables), they won't be ever be released until the scope is disposed. This could result in memory usage growing over time.

I think this is solved relatively easily (as per some of the things we have discussed already) and I am happy to submit a PR:

• CSLA to allow a scoped IServiceProvider to be set in local context (rather than an IServiceScope as currently)
  • Caller is responsible for creating a DI Scope to begin with, and giving CSLA its IServiceProvider
  • Caller can do this at the start of a request by setting CSLA's scoped IServiceProvider from HttpContext.RequestServices (which gets disposed at the end of the request). In background jobs, user would create a new IServiceScope, set the CSLA IServiceProvider to that scopes IServiceProvider, then the caller would hang on to the service scope and dispose of it at the appropriate time (for example after the job had finished etc).

I noted that certain code allows the scoped provider to be null, and will work by reverting to ordinary type activation (non DI):

https://github.com/MarimerLLC/csla/blob/master/Source/Csla.Shared/Server/ObjectFactoryLoader.cs#L57

However with the following code, it looks like for an injectable method, do you think it would be desirable perhaps to throw an exception if the service provider was null, and injectable parameters were encountered - rather than injecting null parameters (which I am guessing is what happens in that case but I could be wrong - in fact as scoped IServiceProvider is currently created by CSLA on the fly if it doesn't exist, this should never be the case currently)

https://github.com/MarimerLLC/csla/blob/master/Source/Csla.Shared/Reflection/ServiceProviderMethodCaller.cs#L413

Would you be up for me submitting a PR with some discreet changes, for consideration?

Version and Platform
CSLA version: master branch
OS: Windows, Linux, iOS, Android, etc.
Platform: WinForms, WPF, ASP.NET Core, MVC, Xamarin, etc.

Code that Fails
Provide the failing code, ideally an isolated repro of the issue you are encountering.

Stack Trace or Exception Detail
Provide a stack trace, or at least detailed information about the exception.

Additional context
Add any other context about the problem here.

[dazinator]

For additional info on transient disposal issue: aspnet/DependencyInjection#456

[dazinator]

The one thing I am not certain about is when the following code comes into play that disposes the scope.. does this only run on a remote server? or only when there is no remote server - i.e on the client side? If its running on a remote server, it should be possible to have the DI scope created and disposed for each request / response (similar to what asp.net core already does for each request). If its only running on the client then.. if a user has set a scope specifically, having csla dispose of that is dangerous as csla will be disposing of the callers DI scope which the caller may still be using - however with my changes suggested above it would mean that as CSLA no longer creates scopes, it no longer needs to worry about disposing of them either..

https://github.com/MarimerLLC/csla/blob/master/Source/Csla.Shared/Server/DataPortal.cs#L745

[rockfordlhotka]

@dazinator I think you have a good point about throwing an exception for an injectable parameter when the service provider is null. That would probably help avoid some otherwise obscure bugs. I'd welcome a PR on that!

[rockfordlhotka]

@dazinator Disposing the scoped service provider might be a little tricky, as there are two scenarios.

First, the scoped provider might be created by the data portal, in the absence of one having been provided by your code (this is the most common scenario I'm sure). In this case the data portal could dispose the scoped provider as the workflow exists the server-side data portal call.

Second, you might have provided the scoped provider to the data portal, in which case I think it would be a bad idea for the data portal to unilaterally dispose the object, because you should retain control.

Right now the data portal doesn't know which scenario applies, because it just asks ApplicationContext for the provider and it gets one.

[dazinator]

@rockfordlhotka have submitted the PR which hopefully encapsulates my take on addressing these issues.

[rockfordlhotka]

@dazinator I left this issue open for the work to throw an exception on Inject parameters if the provider is null. Or do you want to open a new work item for that task?

[dazinator]

Im happy either way, whatever is best for you. I was unable to add a resx entry to the netstandard project without appveyor build failing so from my perspective that work / tweak is currently blocked.

[dazinator]

Im happy either way, whatever is best for you. I was unable to add a resx entry to the netstandard project without appveyor build failing so from my perspective that work / tweak is currently blocked.

Sorry, have just seen your comment about throwing an NRE to avoid a new resx entry. That should unblock this. I'll likely submit a new PR for that on Monday if no one beats me to it.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.