search

unitycontainer / microsoft-dependency-injection

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

Missing resolution error details in ServiceProvider #60

Closed carmeloanthony015 closed 4 years ago

carmeloanthony015 commented 4 years ago

When, for example, we have an error during the resolution of a Controller constructor, we receive a very generic error message

Unable to resolve service for type Type2 while attempting to activate Type1

The error could be in a following resolution, but we'll never know... I noticed that at line 37 of ServiceProvider class, the exceptions thrown by _container.Resolve are "suppressed" and it just returns null. I think that throwing the exception here would give the user much more details about the error. Is this a good idea or is there a specific reason behind the particular exception handling in use currently?

Hieronymus1 commented 4 years ago

We should definitely not swallow thrown exceptions to return null because exceptions are not meant to control the execution flow but it is legitimate to return null in IServiceProvider.GetService because there is another IServiceProvider.GetRequiredService extension that can't return null.

This issue is a duplicate of #53 though.

carmeloanthony015 commented 4 years ago

Maybe I'm not getting entirely your answer, but MVC just uses IServiceProvider.GetService to invoke controllers' constructors, so I don't know how could I call IServiceProvider.GetRequiredService. Also, if I'm checking the correct code, the ServiceProvider must implement ISupportRequiredService interface, that is not implemented by Unity's ServiceProvider class

Hieronymus1 commented 4 years ago

It looks like the ISupportRequiredService implementation is optional because it still calls GetService to throw an explicit exception when it can't resolve the requested type.

Otherwise I'm not sure about the MVC integration but there must be a way to customize how the resolution is done.

carmeloanthony015 commented 4 years ago

It's optional, but if not implemented we get a generic exception, not the verbose Unity logging. Considered this, even if we find a way to customize MVC integration, the issue would still not be resolved

Hieronymus1 commented 4 years ago

That's why there is still an open issue for this bug.

I was just mentioning it can be legitimate to return null in GetRequiredService but it should throw an explicit exception with diagnostic details if the service is registered but the resolution fails because of another sub dependency that can't be resolved.

ENikS commented 4 years ago

Duplicate of #53