unitycontainer / microsoft-dependency-injection

Unity.Microsoft.DependencyInjection package
Apache License 2.0
63 stars 36 forks source link

The ServiceProvider swallows all errors and gives the impression that the service does not exist. #53

Closed cduivis closed 4 years ago

cduivis commented 5 years ago

https://github.com/unitycontainer/microsoft-dependency-injection/blob/8a16285e9ec72078ee97d1b9880feaa11a52627b/src/ServiceProvider.cs#L37

This makes resolving mis-configured dependencies extremely hard to find, as it doesn't even point you in the right direction, making you believe there is something wrong with the setup..

If you do figure out where the error is due to a mis-configuration it can take you quite some time to find the correct one (especially when your dependency graph goes several layers deep).

ENikS commented 5 years ago

How do you suggest it should be improved? Could you provide an example?

rjgotten commented 5 years ago

Chiming in. @cduivis is a colleague of mine and we talked about this earlier today.

The motivation for the catch-all is ofcourse in the fact that many core framework-related services may be queried from the container once hooked up via the service providers. You need to return null for those if they're not registered, to ensure applications don't break.

On the other hand, like @cduivis mentioned; it makes mis-configured dependencies extremely hard to detect, because user-generated errors thrown due to failing build-up of user-supplied classes will also be caught and swallowed.

One way to possibly resolve this, is to only swallow those exceptions that indicate Unity was unable to find a compatible type and to not swallow exceptions that indicate failure during resolution or build up of a type.

Another would be to supply a means to hook in a logger. But that would mean exceptions thrown when failing to resolve framework services would also flow into logging, even when those services are intentionally not registered in the unity container. And that involves a lo----t of log spam, so it would be a (very) distant second best, I think.


[EDIT]

I wonder if it can be as easy as adding a simple check using IUnityContainer.IsRegistered. E.g. something like:

return _container.IsRegistered(serviceType))
  ? _container.Resolve(serviceType)
  : null;

Not sure though, if that will also report otherwise auto-resolved concrete classes as registered, so it may need further padding with a check that allows non-interface types to pass through to Resolve immediately.

ENikS commented 5 years ago

How about we try to create a list of requirements for the diagnostic extension for this project? The regular Diagnostic extension is too restrictive, but is you could help me to specify minimum working set of requirements for this particular project I could implement it in September.

Hieronymus1 commented 5 years ago

I tried to change the code to use container.IsRegistered to replace the empty try catch before but this didn't work though, probably because it didn't detect if other sub dependencies were also properly registered or not.

In any case, checking if container.IsRegistered(serviceType) to call Resolve when true is probably the best solution as it will bubble up the unity exception with the the relevant diagnostic message when it fails to resolve because of missing sub-dependencies.

As long as it doesn't silently ignore exceptions.

rjgotten commented 5 years ago

In any case, checking if container.IsRegistered(serviceType) to call Resolve when true is probably the best solution as it will bubble up the unity exception with the the relevant diagnostic message when it fails to resolve because of missing sub-dependencies.

As long as it doesn't silently ignore exceptions.

That was exactly my reasoning as well. :)

If an interface-based service is not registered (and iirc all the core framework services are interface based with .NET Core right?) then it should skip and return null. But if an interface is registered (or an explicit class is auto-resolved) and fails for whatever reason then obviously the intention was for it to succeed, so the exception should not be swallowed.

Hieronymus1 commented 5 years ago

This would be perfect. Unlike the IUnityContainer that systematically throws an exception when failing to resolve the IServiceProvider API is designed to return null if there is no registration. There is also another GetRequiredService extension designed to throw an exception when there is no registration for the requested type.

ENikS commented 4 years ago

Fixed in 5.11.0 Service provider now supports ISupportRequiredService and throws real exception when fails to resolve.